Windows execv, spawnv, wait - #6350
Conversation
Sorry, something went wrong.
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)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)crates/vm/src/stdlib/nt.rs (3)📜 Review details435-441: Inconsistent error message format.
The error messages here use a different format ("execve: argv...") compared to execv and spawnv which 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_widestring closure is duplicated across spawnv, spawnve, execv, and execve. 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 between spawnve and execve. A shared helper would reduce duplication.
Also applies to: 449-474
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 2a42535 and 25d77a1.
⛔ Files ignored due to path filters (1)- Lib/test/test_os.py is excluded by !Lib/**
- crates/vm/Cargo.toml (1 hunks)
- crates/vm/src/stdlib/nt.rs (6 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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
crates/vm/src/stdlib/posix.rs (8)🔇 Additional comments (8)crates/vm/src/stdlib/os.rs (3)
- 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/windows.rs (3)
- errno_err (39-41)
- path (485-487)
- std (1060-1060)
- std (407-407)
- std (460-460)
- std (471-471)
crates/vm/Cargo.toml (1)crates/vm/src/stdlib/nt.rs (7)132-132: LGTM!
The Win32_Security_Authorization feature addition is correctly placed and necessary for the new SDDL-based security descriptor functionality in mkdir.
165-176: LGTM!
The cast chain (status as u32) as u64 correctly preserves the full bit pattern for large Windows exit codes (e.g., 0xC000013A), and the left-shift by 8 properly aligns with POSIX waitpid format.
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 argv vector stays alive during the unsafe call. The error messages use correct 1-indexed argument numbers matching CPython convention.
375-413: LGTM!
The execv implementation properly validates arguments and uses OsPath for path handling. Error messages correctly reference "arg 2" for argv.
60-62: TMP_MAX should match MSVC's standard value or document the intentional divergence.
MSVC's <stdio.h> defines TMP_MAX as 32767, not i32::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 LocalFree in all code paths.
Sorry, something went wrong.
| 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.
⚠️ Potential issue | 🟡 Minor
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"));
}‼️ 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")); | |
| } |
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".
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.