Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/vm/src/stdlib/winapi.rs (1)

245-249: Consider folding the NUL validation into wstr to 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 fmt and cargo clippy, and confirm the failing test_subprocess case now raises ValueError("embedded null character") for both name and command_line.

Also applies to: 251-269

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75dcf80 and f0899e4.

⛔ Files ignored due to path filters (1)

• Lib/test/test_subprocess.py is 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 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/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 calling CreateProcessW. Returning crate::exceptions::cstring_error(vm) here matches the existing “embedded null character” behavior used elsewhere (e.g., env handling), and avoids constructing invalid wide strings.

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/stdlib/src/locale.rs (1)

239-253: Pre-setlocale validation is well-placed; consider trimming parts for LC_ALL

Running validation before CString::new/libc::setlocale is the right spot. For LC_ALL, you may want to trim() each part (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

📥 Commits

Reviewing files that changed from the base of the PR and between f0899e4 and 752c0f6.

⛔ Files ignored due to path filters (2)

• Lib/test/test_locale.py is excluded by !Lib/**
• Lib/test/test_subprocess.py is 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 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/winapi.rs
• crates/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 strings

This matches the existing cstring_error(vm) behavior elsewhere (e.g., env var validation) and avoids passing potentially truncated strings into CreateProcessW.
One caveat: the if let ... && ... “let-chains” syntax is MSRV-sensitive—please confirm RustPython’s MSRV supports it, or rewrite to a nested if let for 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: Validate MAX_CP_LEN = 15 against Windows CRT/CPython expectations

The helper split/scan looks reasonable, but the specific limit (15) and accepted LC_ALL formatting (semicolon-separated parts, possible LC_*=... 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.