Rework crt_fd to be more aligned with io-safety#5789
Conversation
Sorry, something went wrong.
coolreader18
force-pushed
the
crt_fd-rework
branch
3 times, most recently
from
0a201cd to
cadba1b
Compare
May 22, 2025 03:48
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis update refactors file descriptor handling throughout the codebase. It introduces explicit Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonVM
participant RustAPI
participant OS
User->>PythonVM: Call os.open/close/read/write/stat/etc.
PythonVM->>RustAPI: Convert Python args to Owned/Borrowed fd
RustAPI->>OS: Perform syscall with fd.as_raw()
OS-->>RustAPI: Return result or error code
RustAPI->>PythonVM: Return result or raise exception
PythonVM-->>User: Return value or Python exception
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
vm/src/stdlib/os.rs (1)
1328-1334: ⚠️ Potential issue
copy_file_rangepassesBorroweddirectly tolibc::syscall, breaking the expectedlongABI
libc::syscallexpects its variadic arguments to be integral types widened toc_long.
Passing a zero-sized-phantom wrapper struct (crt_fd::Borrowed) relies onrepr(transparent>and an implicit struct-to-integer cast, which is undefined in the C ABI and currently rejected by LLVM in some optimisation levels.- libc::SYS_copy_file_range, - args.src, + libc::SYS_copy_file_range, + args.src.as_raw() as libc::c_long, @@ - args.dst, + args.dst.as_raw() as libc::c_long,(The explicit
as libc::c_longmakes the intent unmistakable.)Without this change the build may succeed with
--releaseon glibc but fail on musl/clang.
🧹 Nitpick comments (6)
vm/src/stdlib/posix_compat.rs (1)
29-36: Lifetime-annotatedDirFdlooks good, but consider a more expressive lifetime nameAdding
'atoSymlinkArgsand propagating it toDirFdaligns with the newBorrowed/Ownedmodel – no functional issues spotted.
For readability, you might rename'ato something domain-specific ('fdor'dirfd) so that future readers instantly understand the intent.common/src/fileutils.rs (1)
98-105: Windows: minor style – avoid shadowinghtwiceThe double-binding is perfectly valid, but a small
matchkeeps the happy path flatter and setsSetLastErroronly when needed:- 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(); + let h = match crt_fd::as_handle(fd) { + Ok(h) => h, + Err(e) => { + unsafe { SetLastError(ERROR_INVALID_HANDLE) }; + return Err(e); + } + }; + let h = h.as_raw_handle();Pure style – feel free to ignore.
vm/src/stdlib/posix.rs (1)
167-177: Consider strengthening the safety documentation for file descriptor conversions.The safety comment acknowledges that Python's file descriptor API isn't io-safe, but this could be better documented. Consider adding more detailed safety requirements or invariants that callers must uphold.
- // SAFETY: none, really. but, python's os api of passing around file descriptors - // everywhere isn't really io-safe anyway, so, this is passed to the user. + // SAFETY: This conversion is inherently unsafe as we cannot guarantee: + // 1. The file descriptor is valid and open + // 2. The file descriptor won't be closed while this BorrowedFd exists + // 3. No other code will modify the file descriptor's state + // This matches Python's os module behavior which doesn't provide io-safety guarantees. Ok(unsafe { BorrowedFd::borrow_raw(fd) })vm/src/ospath.rs (1)
107-114: Consider documenting the safety requirements for try_borrow_raw.While the unsafe block correctly handles the conversion from a raw file descriptor to a borrowed one, it would be beneficial to document the safety requirements more explicitly.
Some(int) => { let fd = int?.try_to_primitive(vm)?; + // SAFETY: We're creating a borrowed file descriptor from a raw fd provided by Python. + // The safety of this operation depends on the Python code properly managing the fd lifetime. unsafe { crt_fd::Borrowed::try_borrow_raw(fd) } .map(Self::Fd) .map_err(|e| e.into_pyexception(vm)) }vm/src/stdlib/io.rs (1)
3883-3885:fcntl()can takeBorrowedFddirectly — avoid raw FD conversionYou already have a typed
Borrowedin scope:if let Ok(OsPathOrFd::Fd(fd)) = … { nix::fcntl::fcntl(fd.as_raw(), nix::fcntl::F_GETFD) }
nix::fcntl::fcntlis generic overAsRawFd; passingfddirectly removes
the extra syscall-unsafe integer step:- nix::fcntl::fcntl(fd.as_raw(), nix::fcntl::F_GETFD) + nix::fcntl::fcntl(&fd, nix::fcntl::F_GETFD)Minor, but keeps the code consistently io-safe.
common/src/crt_fd.rs (1)
356-359: Preferc::read(alias) for consistency and to avoid double-dispatchThe rest of the module consistently funnels raw libc calls through the
c::*alias, but_readgoes directly tolibc::read. For symmetry (and potential platform shims) it should do the same:- let n = cvt(unsafe { suppress_iph!(libc::read(fd, buf.as_mut_ptr() as _, count as _)) })?; + let n = cvt(unsafe { suppress_iph!(c::read(fd, buf.as_mut_ptr() as _, count as _)) })?;Minor, yet it keeps the abstraction layer uniform.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
common/src/crt_fd.rs(1 hunks)common/src/fileutils.rs(3 hunks)vm/src/ospath.rs(3 hunks)vm/src/stdlib/io.rs(8 hunks)vm/src/stdlib/msvcrt.rs(3 hunks)vm/src/stdlib/nt.rs(5 hunks)vm/src/stdlib/os.rs(25 hunks)vm/src/stdlib/posix.rs(15 hunks)vm/src/stdlib/posix_compat.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
vm/src/stdlib/msvcrt.rs (2)
vm/src/stdlib/os.rs (2)
errno_err(39-41)std(1032-1032)common/src/crt_fd.rs (1)
as_handle(337-341)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (11)
common/src/fileutils.rs (1)
11-21:Details
❌ Incorrect review comment
Unix: verify the public helper is really called
as_raw()
fd.as_raw()compiles only ifBorrowed<'_>exposes that exact method. In the standard library the idiom isas_raw_fd(); drifting from that spelling makes grepping harder for people accustomed to the std traits.- let ret = libc::fstat(fd.as_raw(), stat.as_mut_ptr()); + let ret = libc::fstat(fd.as_raw_fd(), stat.as_mut_ptr());If
as_raw()is the canonical helper incrt_fd, ignore this.Why? A quick search in the repo will confirm which spelling is available.
🏁 Script executed:
#!/bin/bash # list all definitions ending with "fn as_raw" rg -n "fn as_raw" | headLength of output: 721
🏁 Script executed:
#!/bin/bash # Show the impl block for Borrowed<'a> to see which methods it defines rg -n "impl<'a> Borrowed<'a>" -A15 -B5 common/src/crt_fd.rsLength of output: 67
🏁 Script executed:
#!/bin/bash # Locate the struct definition for Borrowed and its impl blocks rg -n "struct Borrowed" -A5 -B5 common/src/crt_fd.rs rg -n "impl Borrowed" -A10 -B0 common/src/crt_fd.rsLength of output: 917
🏁 Script executed:
#!/bin/bash # Show all `as_raw` method definitions with context rg -n "pub fn as_raw" -A5 -B5 common/src/crt_fd.rsLength of output: 1107
No change needed:
Borrowed<'_>defines.as_raw(), not.as_raw_fd()
TheBorrowed<'_>wrapper incommon/src/crt_fd.rsimplements:
pub fn as_raw(&self) -> Rawpub fn as_raw(self) -> RawIt does not expose an
as_raw_fd()method. Callingfd.as_raw_fd()will fail to compile, so the existingfd.as_raw()invocation incommon/src/fileutils.rsis correct.Likely an incorrect or invalid review comment.
vm/src/stdlib/msvcrt.rs (3)
24-26: Public helpers now takeBorrowed<'_>– good upgrade
setmode_binaryno longer risks raw-fd misuse. Implementation stays unchanged – looks correct.
75-76: Safety check: extern_setmoderelies onBorrowedbeing#[repr(transparent)]Passing
Borrowed<'_>directly over FFI is fine only if the wrapper is transparent over anintandCopy. Confirm the type definition:#[repr(transparent)] #[derive(Copy, Clone)] pub struct Borrowed<'a>(c::c_int, PhantomData<&'a ()>);If either attribute is missing, the ABI may break.
103-107: Nice simplification ofget_osfhandleerror handlingDelegating to
crt_fd::as_handleand usingIntoPyExceptionkeeps the function tight and idiomatic.vm/src/stdlib/posix.rs (2)
335-336: Good use of lifetime parameters for borrowed file descriptors.The addition of lifetime parameters to structs and functions that handle borrowed file descriptors improves type safety and makes the borrowing relationships explicit. This aligns well with Rust's ownership model.
Also applies to: 341-342, 345-345, 350-350, 412-412, 441-441, 443-443, 582-582, 594-594, 617-617, 896-896, 915-916, 924-926, 938-939, 957-957, 977-980, 983-983, 1208-1209, 1979-1979, 2200-2221
350-350: Consistent API update from raw file descriptors to safe wrappers.The changes consistently replace raw file descriptor access methods (
.get_opt()) with the new API (.raw_opt(),.as_raw(),.as_raw_fd()), which provides better type safety and clearer semantics.Also applies to: 412-412, 416-417, 594-594, 915-916, 958-958, 1959-1959, 1983-1983
vm/src/stdlib/nt.rs (2)
65-66: Consistent lifetime parameter additions for DirFd.The lifetime parameters added to
DirFdusage maintain consistency with the changes in other modules, ensuring proper tracking of borrowed file descriptors.Also applies to: 71-72, 75-75, 114-115
93-100: Improved type safety for set_inheritable function.The change from accepting a raw
i32tocrt_fd::Borrowed<'_>improves type safety and makes the borrowing relationship explicit. The error handling for the handle conversion is also properly implemented.vm/src/ospath.rs (2)
100-103: Well-structured lifetime parameter addition to OsPathOrFd.The addition of the lifetime parameter
'fdtoOsPathOrFdproperly tracks the lifetime of borrowed file descriptors, maintaining safety invariants across the type system.Also applies to: 105-105, 119-120, 125-125
149-151: Clean API update for IOErrorBuilder to support lifetime parameters.The updates to
IOErrorBuildermethods to acceptimpl Into<OsPathOrFd<'a>>provide a flexible and ergonomic API while maintaining proper lifetime tracking.Also applies to: 154-156, 162-165
vm/src/stdlib/io.rs (1)
4467-4469: Verifycrt_fd::close()does not double-drop the descriptor
close()constructs anOwnedwithfrom_raw(fd)and passes it by value to
crt_fd::close(). Ifclose()internally closes the FD and then allows
Ownedto be dropped (itsDropimpl also closes), the descriptor will be
closed twice.Please confirm the implementation contract:
- If
crt_fd::close()consumes theOwnedandmem::forgets it, current code
is fine.- If it merely forwards to
libc::closeand returns, the subsequentDrop
will execute a secondclose.Unit-test with
dup()plusfcntl(F_GETFD)or auditcrt_fd::close().
Sorry, something went wrong.
coolreader18
force-pushed
the
crt_fd-rework
branch
from
3e7bc95 to
8437b06
Compare
August 29, 2025 16:00
coolreader18
merged commit 7509387
into
RustPython:main
Aug 29, 2025
Summary by CodeRabbit
Refactor
Bug Fixes
Style
Tests