Skip to content

Rework crt_fd to be more aligned with io-safety #5789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
coolreader18 merged 3 commits into from
Aug 29, 2025
Merged

Rework crt_fd to be more aligned with io-safety #5789

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51cbf57
Rework crt_fd to be more aligned with io-safety
coolreader18 May 15, 2025
dc4be47
Windows fixes
coolreader18 Jun 6, 2025
8437b06
Unmark passing tests
coolreader18 Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Windows fixes
  • Loading branch information
@coolreader18
coolreader18 committed Aug 29, 2025
commit dc4be4775101ec413cc0f91905e98bd553eb1f00
5 changes: 3 additions & 2 deletions common/src/crt_fd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,12 +335,13 @@ pub fn ftruncate(fd: Borrowed<'_>, len: Offset) -> io::Result<()> {

#[cfg(windows)]
pub fn as_handle(fd: Borrowed<'_>) -> io::Result<BorrowedHandle<'_>> {
use windows_sys::Win32::Foundation::{HANDLE, INVALID_HANDLE_VALUE};
unsafe extern "C" {
fn _get_osfhandle(fd: Borrowed<'_>) -> c::intptr_t;
}
Comment on lines +338 to +341

@coderabbitai coderabbitai Bot Jun 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

_get_osfhandle FFI signature should take a plain i32 instead of Borrowed

_get_osfhandle is declared in the CRT as intptr_t _get_osfhandle(int fd);.
Passing a Borrowed<'_> (even though repr(transparent)) in the FFI boundary:

  1. Introduces a generic‐parametrised type into the C ABI (discouraged and may be rejected by future rustc versions).
  2. Forces all callers to perform an implicit struct → integer cast, which is less clear than an explicit .as_raw().
-        fn _get_osfhandle(fd: Borrowed<'_>) -> c::intptr_t;
+        fn _get_osfhandle(fd: i32) -> c::intptr_t;

and at the call-site:

-    let handle = unsafe { suppress_iph!(_get_osfhandle(fd)) };
+    let handle = unsafe { suppress_iph!(_get_osfhandle(fd.as_raw())) };

This keeps the ABI crystal-clear and avoids relying on repr(transparent) for safety across the language boundary.

🤖 Prompt for AI Agents 
In common/src/crt_fd.rs around lines 338 to 341, the FFI declaration of
_get_osfhandle currently takes a Borrowed<'_> type, which is incorrect for the C
ABI. Change the function signature to accept a plain i32 instead of Borrowed<'_>
to match the CRT declaration intptr_t _get_osfhandle(int fd). Also, update all
call sites to explicitly convert Borrowed values to i32 using .as_raw() before
passing them to _get_osfhandle, ensuring clear and safe ABI boundaries.

@coolreader18 coolreader18 Jun 6, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, impressive, but irrelevant.

@coderabbitai coderabbitai Bot Jun 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolreader18 Fair enough! You're focused on the bigger io-safety refactoring here, and that FFI detail can certainly be addressed separately if needed. Thanks for the clarification on priorities.

let handle = unsafe { suppress_iph!(_get_osfhandle(fd)) };
if handle == -1 {
Err(io::Error::last_os_error())
if handle as HANDLE == INVALID_HANDLE_VALUE {
Err(crate::os::last_os_error())
} else {
Ok(unsafe { BorrowedHandle::borrow_raw(handle as _) })
}
Expand Down
28 changes: 8 additions & 20 deletions common/src/fileutils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ pub use libc::stat as StatStruct;
pub use windows::{StatStruct, fstat};

#[cfg(not(windows))]
pub fn fstat(fd: libc::c_int) -> std::io::Result<StatStruct> {
pub fn fstat(fd: crate::crt_fd::Borrowed<'_>) -> std::io::Result<StatStruct> {
let mut stat = std::mem::MaybeUninit::uninit();
unsafe {
let ret = libc::fstat(fd, stat.as_mut_ptr());
let ret = libc::fstat(fd.as_raw(), stat.as_mut_ptr());
if ret == -1 {
Err(crate::os::last_os_error())
} else {
Expand All @@ -22,15 +22,15 @@ pub fn fstat(fd: libc::c_int) -> std::io::Result<StatStruct> {

#[cfg(windows)]
pub mod windows {
use crate::suppress_iph;
use crate::crt_fd;
use crate::windows::ToWideString;
use libc::{S_IFCHR, S_IFDIR, S_IFMT};
use std::ffi::{CString, OsStr, OsString};
use std::os::windows::ffi::OsStrExt;
use std::os::windows::io::AsRawHandle;
use std::sync::OnceLock;
use windows_sys::Win32::Foundation::{
BOOL, ERROR_INVALID_HANDLE, ERROR_NOT_SUPPORTED, FILETIME, FreeLibrary, HANDLE,
INVALID_HANDLE_VALUE, SetLastError,
BOOL, ERROR_INVALID_HANDLE, ERROR_NOT_SUPPORTED, FILETIME, FreeLibrary, SetLastError,
};
use windows_sys::Win32::Storage::FileSystem::{
BY_HANDLE_FILE_INFORMATION, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_READONLY,
Expand Down Expand Up @@ -94,26 +94,14 @@ pub mod windows {
}
}

unsafe extern "C" {
fn _get_osfhandle(fd: i32) -> libc::intptr_t;
}

fn get_osfhandle(fd: i32) -> std::io::Result<isize> {
let ret = unsafe { suppress_iph!(_get_osfhandle(fd)) };
if ret as HANDLE == INVALID_HANDLE_VALUE {
Err(crate::os::last_os_error())
} else {
Ok(ret)
}
}

// _Py_fstat_noraise in cpython
pub fn fstat(fd: libc::c_int) -> std::io::Result<StatStruct> {
let h = get_osfhandle(fd);
pub fn fstat(fd: crt_fd::Borrowed<'_>) -> std::io::Result<StatStruct> {
let h = crt_fd::as_handle(fd);
if h.is_err() {
unsafe { SetLastError(ERROR_INVALID_HANDLE) };
}
let h = h?;
let h = h.as_raw_handle();
// reset stat?

let file_type = unsafe { GetFileType(h as _) };
Expand Down
2 changes: 1 addition & 1 deletion vm/src/stdlib/io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4339,7 +4339,7 @@ mod fileio {

Comment on lines +4337 to 4339

@coderabbitai coderabbitai Bot Jun 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consolidate repeated unsafe { borrow_raw(..) } into a single, safe helper

borrow_raw is invoked in several places (init, get_fd, path-vs-fd
resolution, etc.). Centralising the only unsafe block behind a small
fn borrow_fd(raw: i32) -> Borrowed<'_> greatly limits the surface for future
unsoundness and avoids copy-pasted unsafe code.

- let fd = unsafe { crt_fd::Borrowed::borrow_raw(fd) };
+ let fd = borrow_fd(fd);            // new safe helper

This also makes it trivial to add additional validation (e.g. negative FD
check) in exactly one place.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In vm/src/stdlib/io.rs around lines 4249 to 4251, the unsafe call to
borrow_raw(fd) is repeated in multiple places. To fix this, create a single safe
helper function, e.g., borrow_fd(raw: i32) -> Borrowed<'_>, that encapsulates
the unsafe block and performs any necessary validation like checking for
negative FDs. Replace all direct calls to unsafe { borrow_raw(..) } with calls
to this new helper function to centralize unsafe code and reduce duplication.

@youknowone youknowone Jun 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coolreader18 please check this comment

@coderabbitai coderabbitai Bot Jun 15, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@coolreader18 coolreader18 Aug 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not necessarily a benefit to that, since just putting it behind a safe utility function wouldn't actually make it more safe, as each case has its own reason that the unsafe code is okay.

// TODO: _Py_set_inheritable

let fd_fstat = crate::common::fileutils::fstat(fd.as_raw());
let fd_fstat = crate::common::fileutils::fstat(fd);

#[cfg(windows)]
{
Expand Down
27 changes: 11 additions & 16 deletions vm/src/stdlib/msvcrt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,21 @@ mod msvcrt {
use crate::{
PyRef, PyResult, VirtualMachine,
builtins::{PyBytes, PyStrRef},
common::suppress_iph,
common::{crt_fd, suppress_iph},
convert::IntoPyException,
stdlib::os::errno_err,
};
use itertools::Itertools;
use windows_sys::Win32::{
Foundation::{HANDLE, INVALID_HANDLE_VALUE},
System::Diagnostics::Debug,
};
use std::os::windows::io::AsRawHandle;
use windows_sys::Win32::System::Diagnostics::Debug;

#[pyattr]
use windows_sys::Win32::System::Diagnostics::Debug::{
SEM_FAILCRITICALERRORS, SEM_NOALIGNMENTFAULTEXCEPT, SEM_NOGPFAULTERRORBOX,
SEM_NOOPENFILEERRORBOX,
};

pub fn setmode_binary(fd: i32) {
pub fn setmode_binary(fd: crt_fd::Borrowed<'_>) {
unsafe { suppress_iph!(_setmode(fd, libc::O_BINARY)) };
}

Expand Down Expand Up @@ -76,11 +75,11 @@ mod msvcrt {
}

unsafe extern "C" {
fn _setmode(fd: i32, flags: i32) -> i32;
fn _setmode(fd: crt_fd::Borrowed<'_>, flags: i32) -> i32;
}

#[pyfunction]
fn setmode(fd: i32, flags: i32, vm: &VirtualMachine) -> PyResult<i32> {
fn setmode(fd: crt_fd::Borrowed<'_>, flags: i32, vm: &VirtualMachine) -> PyResult<i32> {
let flags = unsafe { suppress_iph!(_setmode(fd, flags)) };
if flags == -1 {
Err(errno_err(vm))
Expand All @@ -91,7 +90,6 @@ mod msvcrt {

unsafe extern "C" {
fn _open_osfhandle(osfhandle: isize, flags: i32) -> i32;
fn _get_osfhandle(fd: i32) -> libc::intptr_t;
}

#[pyfunction]
Expand All @@ -105,13 +103,10 @@ mod msvcrt {
}

#[pyfunction]
fn get_osfhandle(fd: i32, vm: &VirtualMachine) -> PyResult<isize> {
let ret = unsafe { suppress_iph!(_get_osfhandle(fd)) };
if ret as HANDLE == INVALID_HANDLE_VALUE {
Err(errno_err(vm))
} else {
Ok(ret)
}
fn get_osfhandle(fd: crt_fd::Borrowed<'_>, vm: &VirtualMachine) -> PyResult<isize> {
crt_fd::as_handle(fd)
.map(|h| h.as_raw_handle() as _)
.map_err(|e| e.into_pyexception(vm))
}

#[allow(non_snake_case)]
Expand Down
23 changes: 14 additions & 9 deletions vm/src/stdlib/nt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,14 @@ pub(crate) mod module {
use crate::{
PyResult, TryFromObject, VirtualMachine,
builtins::{PyDictRef, PyListRef, PyStrRef, PyTupleRef},
common::{crt_fd::Fd, os::last_os_error, suppress_iph},
common::{crt_fd, os::last_os_error, suppress_iph},
convert::ToPyException,
function::{Either, OptionalArg},
ospath::OsPath,
stdlib::os::{_os, DirFd, FollowSymlinks, SupportFunc, TargetIsDirectory, errno_err},
};
use libc::intptr_t;
use std::os::windows::io::AsRawHandle;
use std::{
env, fs, io,
mem::MaybeUninit,
Expand Down Expand Up @@ -61,17 +62,17 @@ pub(crate) mod module {
}

#[derive(FromArgs)]
pub(super) struct SymlinkArgs {
pub(super) struct SymlinkArgs<'fd> {
src: OsPath,
dst: OsPath,
#[pyarg(flatten)]
target_is_directory: TargetIsDirectory,
#[pyarg(flatten)]
_dir_fd: DirFd<{ _os::SYMLINK_DIR_FD as usize }>,
_dir_fd: DirFd<'fd, { _os::SYMLINK_DIR_FD as usize }>,
}

#[pyfunction]
pub(super) fn symlink(args: SymlinkArgs, vm: &VirtualMachine) -> PyResult<()> {
pub(super) fn symlink(args: SymlinkArgs<'_>, vm: &VirtualMachine) -> PyResult<()> {
use std::os::windows::fs as win_fs;
let dir = args.target_is_directory.target_is_directory
|| args
Expand All @@ -89,9 +90,13 @@ pub(crate) mod module {
}

#[pyfunction]
fn set_inheritable(fd: i32, inheritable: bool, vm: &VirtualMachine) -> PyResult<()> {
let handle = Fd(fd).to_raw_handle().map_err(|e| e.to_pyexception(vm))?;
set_handle_inheritable(handle as _, inheritable, vm)
fn set_inheritable(
fd: crt_fd::Borrowed<'_>,
inheritable: bool,
vm: &VirtualMachine,
) -> PyResult<()> {
let handle = crt_fd::as_handle(fd).map_err(|e| e.to_pyexception(vm))?;
set_handle_inheritable(handle.as_raw_handle() as _, inheritable, vm)
}

#[pyattr]
Expand All @@ -107,7 +112,7 @@ pub(crate) mod module {
#[pyfunction]
fn chmod(
path: OsPath,
dir_fd: DirFd<0>,
dir_fd: DirFd<'_, 0>,
mode: u32,
follow_symlinks: FollowSymlinks,
vm: &VirtualMachine,
Expand Down Expand Up @@ -456,7 +461,7 @@ pub(crate) mod module {
fn mkdir(
path: OsPath,
mode: OptionalArg<i32>,
dir_fd: DirFd<{ _os::MKDIR_DIR_FD as usize }>,
dir_fd: DirFd<'_, { _os::MKDIR_DIR_FD as usize }>,
vm: &VirtualMachine,
) -> PyResult<()> {
let mode = mode.unwrap_or(0o777);
Expand Down
6 changes: 3 additions & 3 deletions vm/src/stdlib/os.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ pub(super) mod _os {
let [] = dir_fd.0;
let name = name.to_wide_cstring(vm)?;
let flags = flags | libc::O_NOINHERIT;
Fd::wopen(&name, flags, mode)
crt_fd::wopen(&name, flags, mode)
};
#[cfg(not(windows))]
let fd = {
Expand Down Expand Up @@ -853,8 +853,8 @@ pub(super) mod _os {

#[cfg(windows)]
fn stat_inner(
file: OsPathOrFd,
dir_fd: DirFd<{ STAT_DIR_FD as usize }>,
file: OsPathOrFd<'_>,
dir_fd: DirFd<'_, { STAT_DIR_FD as usize }>,
follow_symlinks: FollowSymlinks,
) -> io::Result<Option<StatStruct>> {
// TODO: replicate CPython's win32_xstat
Expand Down