Windows execv, spawnv, wait #6350
Conversation
WalkthroughAdded Windows-specific feature flag to Cargo.toml for Windows-sys dependency. Extended nt.rs module with new process spawn/exec functions (spawnv, spawnve, execv, execve), constants for spawn modes and I/O flags, and a restructured mkdir API using MkdirArgs struct. Changed waitpid/wait return types to accommodate larger exit codes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/vm/src/stdlib/nt.rs (3)
435-441: Inconsistent error message format.The error messages here use a different format (
"execve: argv...") compared toexecvandspawnvwhich use"execv() arg 2...". Consider aligning for consistency.let first = argv .first() - .ok_or_else(|| vm.new_value_error("execve: argv must not be empty"))?; + .ok_or_else(|| vm.new_value_error("execve() arg 2 must not be empty"))?; if first.is_empty() { - return Err(vm.new_value_error("execve: argv first element cannot be empty")); + return Err(vm.new_value_error("execve() arg 2 first element cannot be empty")); }
266-267: Consider extracting duplicated helper.The
make_widestringclosure is duplicated acrossspawnv,spawnve,execv, andexecve. Consider extracting it to a shared helper function to reduce duplication.fn make_widestring(s: &str, vm: &VirtualMachine) -> PyResult<widestring::WideCString> { widestring::WideCString::from_os_str(s).map_err(|err| err.to_pyexception(vm)) }Also applies to: 309-310, 384-385, 425-426
333-358: Consider extracting environment building logic.The environment dictionary to wide-string conversion (including validation for null characters and
=in keys) is duplicated betweenspawnveandexecve. A shared helper would reduce duplication.Also applies to: 449-474
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_os.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/Cargo.toml(1 hunks)crates/vm/src/stdlib/nt.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/nt.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/nt.rs (3)
crates/vm/src/stdlib/posix.rs (8)
waitpid(1705-1710)wait(1713-1715)s(1917-1918)s(2311-2311)v(2410-2410)v(2413-2413)v(2419-2419)v(2422-2422)crates/vm/src/stdlib/os.rs (3)
errno_err(39-41)path(485-487)std(1060-1060)crates/vm/src/windows.rs (3)
std(407-407)std(460-460)std(471-471)
🔇 Additional comments (8)
crates/vm/Cargo.toml (1)
132-132: LGTM!The
Win32_Security_Authorizationfeature addition is correctly placed and necessary for the new SDDL-based security descriptor functionality inmkdir.crates/vm/src/stdlib/nt.rs (7)
165-176: LGTM!The cast chain
(status as u32) as u64correctly preserves the full bit pattern for large Windows exit codes (e.g.,0xC000013A), and the left-shift by 8 properly aligns with POSIXwaitpidformat.
178-182: LGTM!Consistent return type with the updated
waitpid.
239-254: LGTM!The extern declarations match the MSVC CRT signatures for wide-string spawn/exec functions.
256-296: LGTM!The implementation correctly validates arguments, builds the null-terminated wide string array, and ensures the
argvvector stays alive during the unsafe call. The error messages use correct 1-indexed argument numbers matching CPython convention.
375-413: LGTM!The
execvimplementation properly validates arguments and usesOsPathfor path handling. Error messages correctly reference "arg 2" forargv.
60-62: TMP_MAX should match MSVC's standard value or document the intentional divergence.MSVC's
<stdio.h>definesTMP_MAXas32767, noti32::MAX(2147483647). If the intent is to match CPython's Windows behavior, add a comment explaining this choice; otherwise, consider aligning with the standard MSVC value to avoid unexpected behavioral differences for code relying on this constant.
695-752: Implementation correctly mirrors CPython's Windows mkdir(mode=0o700) behavior.The SDDL string is accurate: it creates a protected discretionary ACL (D:P) granting Full Access (FA) with inheritance (OICI) to SYSTEM (SY), Administrators (BA), and the directory owner (OW)—matching the security intent of 0o700 mode on Unix systems. Memory management is sound: the security descriptor is properly freed via
LocalFreein all code paths.
| let first = argv | ||
| .first() | ||
| .ok_or_else(|| vm.new_value_error("spawnve() arg 2 cannot be empty"))?; | ||
|
|
||
| if first.is_empty() { | ||
| return Err(vm.new_value_error("spawnve() arg 2 first element cannot be empty")); | ||
| } |
There was a problem hiding this comment.
Incorrect argument index in error messages.
For spawnve(mode, path, argv, env), the argv parameter is argument 3, not argument 2. This is inconsistent with spawnv which correctly uses "arg 3".
let first = argv
.first()
- .ok_or_else(|| vm.new_value_error("spawnve() arg 2 cannot be empty"))?;
+ .ok_or_else(|| vm.new_value_error("spawnve() arg 3 cannot be empty"))?;
if first.is_empty() {
- return Err(vm.new_value_error("spawnve() arg 2 first element cannot be empty"));
+ return Err(vm.new_value_error("spawnve() arg 3 first element cannot be empty"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let first = argv | |
| .first() | |
| .ok_or_else(|| vm.new_value_error("spawnve() arg 2 cannot be empty"))?; | |
| if first.is_empty() { | |
| return Err(vm.new_value_error("spawnve() arg 2 first element cannot be empty")); | |
| } | |
| let first = argv | |
| .first() | |
| .ok_or_else(|| vm.new_value_error("spawnve() arg 3 cannot be empty"))?; | |
| if first.is_empty() { | |
| return Err(vm.new_value_error("spawnve() arg 3 first element cannot be empty")); | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/nt.rs around lines 319-325, the two error messages
mistakenly refer to `argv` as "arg 2" for spawnve(); update both messages to
reference "arg 3" instead so they match the spawnv wording and correctly reflect
the parameter order: change "spawnve() arg 2 cannot be empty" to "spawnve() arg
3 cannot be empty" and "spawnve() arg 2 first element cannot be empty" to
"spawnve() arg 3 first element cannot be empty".
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.