Fix test_subprocess and locale #6416
Conversation
WalkthroughAdds runtime input validation: CreateProcess now rejects embedded NUL bytes in optional name and command_line fields; setlocale on Windows gains encoding-length validation for locale strings (including LC_ALL semicolon-separated entries) before calling libc::setlocale. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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: 0
🧹 Nitpick comments (1)
crates/vm/src/stdlib/winapi.rs (1)
245-249: Consider folding the NUL validation intowstrto remove duplication and align errors across all fields (incl.current_dir).let wstr = |s: PyStrRef| { - let ws = widestring::WideCString::from_str(s.as_str()) + let s = s.as_str(); + if s.contains('\0') { + return Err(crate::exceptions::cstring_error(vm)); + } + let ws = widestring::WideCString::from_str(s) .map_err(|err| err.to_pyexception(vm))?; Ok(ws.into_vec_with_nul()) }; - - // Validate no embedded null bytes in command name and command line - if let Some(ref name) = args.name { - if name.as_str().contains('\0') { - return Err(crate::exceptions::cstring_error(vm)); - } - } - if let Some(ref cmd) = args.command_line { - if cmd.as_str().contains('\0') { - return Err(crate::exceptions::cstring_error(vm)); - } - }Verification I’d do locally (per repo guidelines): run
cargo fmtandcargo clippy, and confirm the failingtest_subprocesscase now raisesValueError("embedded null character")for bothnameandcommand_line.Also applies to: 251-269
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_subprocess.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/winapi.rs(1 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/winapi.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/winapi.rs (1)
crates/vm/src/exceptions.rs (1)
cstring_error(1169-1171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
crates/vm/src/stdlib/winapi.rs (1)
251-261: Good: reject embedded NULs before callingCreateProcessW. Returningcrate::exceptions::cstring_error(vm)here matches the existing “embedded null character” behavior used elsewhere (e.g., env handling), and avoids constructing invalid wide strings.
f0899e4 to
752c0f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/stdlib/src/locale.rs (1)
239-253: Pre-setlocalevalidation is well-placed; consider trimming parts for LC_ALLRunning validation before
CString::new/libc::setlocaleis the right spot. For LC_ALL, you may want totrim()eachpart(or document that whitespace is not accepted) since semicolon-separated locale strings can sometimes include spaces in the wild; otherwise validation may diverge from CRT behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_locale.pyis excluded by!Lib/**Lib/test/test_subprocess.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/src/locale.rs(2 hunks)crates/vm/src/stdlib/winapi.rs(1 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/winapi.rscrates/stdlib/src/locale.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/winapi.rs (1)
crates/vm/src/exceptions.rs (1)
cstring_error(1169-1171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (2)
crates/vm/src/stdlib/winapi.rs (1)
251-261: Good hardening: reject embedded NULs before building wide stringsThis matches the existing
cstring_error(vm)behavior elsewhere (e.g., env var validation) and avoids passing potentially truncated strings intoCreateProcessW.
One caveat: theif let ... && ...“let-chains” syntax is MSRV-sensitive—please confirm RustPython’s MSRV supports it, or rewrite to a nestedif letfor broader compatibility.- if let Some(ref name) = args.name - && name.as_str().contains('\0') - { - return Err(crate::exceptions::cstring_error(vm)); - } - if let Some(ref cmd) = args.command_line - && cmd.as_str().contains('\0') - { - return Err(crate::exceptions::cstring_error(vm)); - } + if let Some(name) = args.name.as_ref() { + if name.as_str().contains('\0') { + return Err(crate::exceptions::cstring_error(vm)); + } + } + if let Some(cmd) = args.command_line.as_ref() { + if cmd.as_str().contains('\0') { + return Err(crate::exceptions::cstring_error(vm)); + } + }crates/stdlib/src/locale.rs (1)
201-227: ValidateMAX_CP_LEN = 15against Windows CRT/CPython expectationsThe helper split/scan looks reasonable, but the specific limit (
15) and accepted LC_ALL formatting (semicolon-separated parts, possibleLC_*=...prefixes) should be verified against the behavior you’re targeting (likely CPython/CRT). If this constant is intentional per upstream, consider a short comment citing the source.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.