GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix test_subprocess and locale by youknowone · Pull Request #6416 · RustPython/RustPython

Conversation

youknowone commented Dec 12, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Summary by CodeRabbit

Bug Fixes
- Strengthened process creation validation to reject invalid embedded characters in optional parameters, preventing malformed process launches.
- Added stricter Windows locale validation to reject unsupported locale encodings, reducing errors when setting locale and improving stability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Walkthrough

Adds 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

Cohort / File(s)	Summary
CreateProcess null-byte validation `crates/vm/src/stdlib/winapi.rs`	Added runtime checks to reject embedded NUL (`\0`) characters in the optional `name` and `command_line` parameters; returns `cstring_error(vm)` on failure and aborts process creation before constructing C strings.
Windows locale validation for setlocale `crates/stdlib/src/locale.rs`	Added `MAX_CP_LEN` constant and Windows-only helpers `check_locale_name` and `check_locale_name_all`; validate encoding length (post-dot, optional `@` modifier) and iterate semicolon-separated locales for `LC_ALL` before calling `libc::setlocale`, returning an unsupported-locale error on invalid input.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Review null-byte checks in winapi.rs for both Some/None branches and ensure correct early return semantics and error construction.
Validate parsing logic in locale.rs (dot, @ modifier, semicolon splitting) and boundary cases near MAX_CP_LEN.
Confirm platform-conditional compilation and that non-Windows paths remain unchanged.
Test vectors: locales with/without encoding, with @ modifiers, semicolon lists, and embedded NULs in both affected APIs.

Poem

🐰 I hopped through code both neat and bright,
Found sneaky NULs and locales not right,
I nibble the bugs, I tidy the line,
Now CreateProcess and setlocale shine 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'Fix test_subprocess and locale' accurately reflects the main changes: runtime validation additions to CreateProcess for null bytes and Windows-specific locale encoding validation.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone marked this pull request as ready for review

December 12, 2025 11:06

coderabbitai Bot reviewed

Dec 12, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone added 2 commits

December 12, 2025 20:26


          Fix test_subprocess

b9fa405


          fix windows locale

752c0f6

youknowone force-pushed the test-subprocess branch from f0899e4 to 752c0f6 Compare

December 12, 2025 11:26

coderabbitai Bot reviewed

Dec 12, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone changed the title ~~Fix test_subprocess~~ Fix test_subprocess and locale

Dec 12, 2025

youknowone merged commit 35f8860 into RustPython:main

Dec 12, 2025

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

youknowone deleted the test-subprocess branch

December 12, 2025 12:03

coderabbitai Bot mentioned this pull request

Jan 3, 2026

Fix locale.getencoding, overlapped.getresult, chdir for windows #6629

Merged

youknowdot mentioned this pull request

Oct 10, 2026

Count the complete Windows locale encoding suffix #9065

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

youknowone commented Dec 12, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 12, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.