test_os, test_io on windows - #6379
Conversation
Sorry, something went wrong.
WalkthroughRemoved Windows CI skips for test_io and test_os; allowed environment variable names beginning with = in nt.rs; added Windows-specific putenv/unsetenv using CRT _wputenv with wide-string length checks and adjusted non-Windows putenv/unsetenv signatures and ffi/OsStr handling in os.rs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 4a3b77c and df899ea. ⛔ Files ignored due to path filters (2)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
Learnt from: arihant2math Repo: RustPython/RustPython PR: 5790 File: build.rs:2-2 Timestamp: 2025-06-28T16:31:03.991Z Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
crates/vm/src/stdlib/nt.rs (1)crates/vm/src/stdlib/os.rs (6) 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/stdlib/os.rs (1)470-490: Fix error type consistency in non-Windows implementations.
The non-Windows putenv function incorrectly raises ValueError for invalid keys, but CPython's os.putenv raises OSError (like unsetenv does). Change "illegal environment variable name" error on line 483 to match unsetenv's behavior of raising an OSError with errno.
The platform-specific separation with #[cfg(not(windows))] is correctly implemented; only the error type needs alignment with CPython.
crates/vm/src/stdlib/os.rs (2)📜 Review details428-437: Unused helper function check_env_var_len.
This helper function is defined but never called. The new Windows putenv and unsetenv implementations (lines 455, 507) perform inline length checks instead. Consider either:
- Removing this dead code, or
- Refactoring the new functions to use this helper for consistency
Note: The inline check wide.len() > _MAX_ENV + 1 differs from what this helper would check (size > _MAX_ENV), which could lead to off-by-one discrepancies.
492-520: Duplicated key validation logic with putenv.
The key validation (lines 498-503) is identical to putenv (lines 446-452). Consider extracting this into a helper function to reduce duplication and ensure consistent validation across both functions.
+#[cfg(windows)] +fn validate_env_key(key_str: &str, vm: &VirtualMachine) -> PyResult<()> { + if key_str.is_empty() + || key_str.get(1..).is_some_and(|s| s.contains('=')) + || key_str.contains('\0') + { + return Err(vm.new_value_error("illegal environment variable name")); + } + Ok(()) +}
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 194e2a6 and bff45e1.
📒 Files selected for processing (1)- crates/vm/src/stdlib/os.rs (2 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/os.rs
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
crates/vm/src/stdlib/os.rs (1)439-468: Verify _MAX_ENV is in scope for this function.
The function references _MAX_ENV at lines 455 and 458, but the only visible import is inside check_env_var_len (line 429), which is scoped to that function. Ensure there's a module-level import for _MAX_ENV or this won't compile.
#!/bin/bash # Description: Verify _MAX_ENV is imported at module level within the _os module # Looking for use statements that import _MAX_ENV outside of function scope rg -n "_MAX_ENV" crates/vm/src/stdlib/os.rs -B5 -A2
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)crates/vm/src/stdlib/os.rs (3)📜 Review details425-437: Unused check_env_var_len helper and stale doc comment
check_env_var_len is no longer called, and its comment about key.len() + value.len() + 2 doesn’t match the current wide-string-based _MAX_ENV checks in putenv/unsetenv. This is likely to trigger an unused-function lint on Windows and is a bit misleading.
Consider either:
- Wiring this helper into the Windows putenv/unsetenv paths (taking wide.len() or similar), and updating the comment accordingly; or
- Dropping the helper entirely and keeping the length checks inline.
495-522: Windows unsetenv: mirrors putenv correctly; consider sharing logic
This Windows-specific unsetenv mirrors putenv’s validation rules and uses _wputenv("KEY=") to remove the variable, which matches typical CRT/CPython behavior. Error handling via new_value_error and new_last_errno_error also looks consistent.
Given how similar it is to putenv, you could:
- Share the name validation logic between the two functions, and
- Optionally share the _MAX_ENV length check via a small helper,
to avoid future drift between the two paths.
If you want to be sure this matches your target CRT’s behavior, please double-check the _wputenv + _MAX_ENV semantics for your MSVC toolchain version and that KEY= indeed removes the variable rather than leaving an empty one.
439-469: Windows putenv: validation logic is duplicated; confirm bytes API change was intentional
The Windows putenv implementation looks correct: it enforces non-empty names, allows a leading =, rejects additional = and embedded NULs, checks the _MAX_ENV limit using the wide string length, and uses _wputenv to keep the CRT environment in sync.
One follow-up worth addressing:
- The name validity check (empty, = position, NUL chars) and _MAX_ENV-based length check are duplicated between putenv (lines 445–454, 456–460) and unsetenv (lines 477–487, 489–494). Pulling them into small helpers (e.g. validate_env_name(&str, vm)? and validate_env_length(&[u16], vm)?) would reduce duplication and keep Windows-specific rules in one place.
Regarding the signature change to PyStrRef only: the current code already accepts only str, not bytes. If this was a recent change from Either<PyStrRef, PyBytesRef>, confirm that the behavioral change is intentional and acceptable (it aligns with CPython on Windows, which accepts text only).
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between e3dc792 and 016c575.
📒 Files selected for processing (1)- crates/vm/src/stdlib/os.rs (2 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/os.rs
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/stdlib/os.rs (1)429-439: Remove unused function.
The check_env_var_len function is never called. The length validation is performed inline at lines 463 and 516. Either use this function or remove it to avoid dead code.
If you intend to use this function, apply this pattern at lines 463 and 516:
- if wide.len() > _MAX_ENV + 1 { - return Err(vm.new_value_error(format!( - "the environment variable is longer than {} characters", - _MAX_ENV - ))); - } + check_env_var_len(wide.len(), vm)?;However, note that the inline checks use wide.len() which includes the null terminator, while check_env_var_len expects size to be key.len() + value.len() + 2. The function signature would need adjustment to match the actual usage pattern.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 016c575 and 7013528.
📒 Files selected for processing (1)- crates/vm/src/stdlib/os.rs (4 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/os.rs
Learnt from: arihant2math Repo: RustPython/RustPython PR: 5790 File: build.rs:2-2 Timestamp: 2025-06-28T16:31:03.991Z Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
crates/vm/src/stdlib/os.rs (6)115-119: LGTM!
Correct platform gating. Windows uses wide strings (PyStrRef) directly, while non-Windows platforms need byte-to-OsStr conversion.
419-425: LGTM!
Correct platform gating. This helper is only needed for non-Windows implementations that accept Either<PyStrRef, PyBytesRef>.
441-444: LGTM!
Correct Windows CRT function declaration for _wputenv.
478-498: LGTM!
Correct non-Windows implementation. The stricter validation (rejecting '=' anywhere in the key) is appropriate for Unix systems, unlike Windows which allows keys starting with '=' for hidden variables.
500-529: LGTM! (Same edge case as putenv)
The implementation correctly uses _wputenv with an empty value ("key=") to unset environment variables on Windows, matching CPython's approach. The validation logic is consistent with the Windows putenv implementation, including the same edge case where a key of just "=" would be allowed.
531-551: LGTM!
Correct non-Windows implementation with appropriate errno-based error handling for invalid key names.
Sorry, something went wrong.
| #[cfg(windows)] | ||
| #[pyfunction] | ||
| fn putenv(key: PyStrRef, value: PyStrRef, vm: &VirtualMachine) -> PyResult<()> { | ||
| use crate::common::windows::_MAX_ENV; | ||
| let key_str = key.as_str(); | ||
| let value_str = value.as_str(); | ||
| // Search from index 1 because on Windows starting '=' is allowed for | ||
| // defining hidden environment variables. | ||
| if key_str.is_empty() | ||
| || key_str.get(1..).is_some_and(|s| s.contains('=')) | ||
| || key_str.contains('\0') | ||
| || value_str.contains('\0') | ||
| { | ||
| return Err(vm.new_value_error("illegal environment variable name")); | ||
| } | ||
| let env_str = format!("{}={}", key_str, value_str); | ||
| let wide = env_str.to_wide_with_nul(); | ||
| if wide.len() > _MAX_ENV + 1 { | ||
| return Err(vm.new_value_error(format!( | ||
| "the environment variable is longer than {} characters", | ||
| _MAX_ENV | ||
| ))); | ||
| } | ||
|
|
||
| // Use _wputenv like CPython (not SetEnvironmentVariableW) to update CRT environ | ||
| let result = unsafe { suppress_iph!(_wputenv(wide.as_ptr())) }; | ||
| if result != 0 { | ||
| return Err(vm.new_last_errno_error()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
# Search for test cases related to putenv
rg -n "putenv|os.putenv" --type=py crates/Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Check if there are any test files for the os module
fd -e py -p test | rg -i "test.*os|os.*test" | head -20Repository: RustPython/RustPython
Length of output: 186
🏁 Script executed:
# Look at the git context to see if there are comments or commits related to this
rg -n "hidden.*environment\|environment.*variable.*name" --type=rsRepository: RustPython/RustPython
Length of output: 186
🌐 Web query:
CPython putenv Windows environment variable validation
💡 Result:
Short answer: CPython validates environment-variable names for os.putenv / os.environ on Windows — names may not contain '=' except possibly as the first character (Windows "hidden" variables like "=C:..."); CPython rejects names with '=' elsewhere and keeps behavior in sync with the C runtime/Windows APIs. See Python issue tracker discussions and the related CPython changes. [1][2][3]
Sources
- Reject '=' in env var names (issue discussion & PRs). [1]
- Windows parsing/leading '=' (hidden) behavior and tests. [2]
- Changes to os.putenv/os.unsetenv implementation and Windows notes (CRT vs SetEnvironmentVariable). [3]
🌐 Web query:
Windows environment variable naming rules "=" character
💡 Result:
Short answer: Windows environment variable names cannot contain the equals sign ("="). Environment strings are stored as "Name=Value\0" so the '=' is the separator and is not allowed in the name. [1][2]
References:
- Microsoft: Environment Variables (Win32) — "The name of an environment variable cannot include an equal sign (=)." [1]
- POSIX / Open Group: environment strings have the form name=value and "names shall not contain the character '='." (useful for general background). [2]
🌐 Web query:
CPython source code putenv Windows implementation validation
💡 Result:
Short answer: CPython validates Windows putenv names and values in Modules/posixmodule.c (win32_putenv). On Windows it:
- rejects empty names and any name containing '=' at or after index 1 (leading '=' is allowed for hidden vars).
- builds a "NAME=VALUE" wide string and uses PyUnicode_AsWideCharString (which rejects embedded NULs).
- checks the combined length against _MAX_ENV and raises ValueError if too long.
- calls the CRT _wputenv (so CRT updates _wenviron/_environ).
Sources: CPython win32_putenv implementation in Modules/posixmodule.c and the related tracker/commit discussion. [1][2]
References
- win32_putenv implementation (Modules/posixmodule.c). Shows the '=' check, NUL rejection via PyUnicode_AsWideCharString, length check against _MAX_ENV, and use of _wputenv. [1]
- bpo-39406 discussion / commits about using CRT putenv on Windows (context why CPython prefers _wputenv). [2]
[1] Modules/posixmodule.c — win32_putenv implementation.
[2] bpo-39406 / CPython commit discussion (os.putenv() behavior on Windows).
Fix edge case: key consisting of only "=" should be rejected
The validation logic allows a key consisting of just "=", which is invalid. The check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false for this case because the substring after index 1 is empty.
Per Windows environment variable rules and CPython's implementation, "=" alone is not a valid environment variable name (it is the separator character in "NAME=VALUE" format). While the code correctly allows leading "=" for Windows hidden variables (e.g., "=C:"), it should reject a key that is only "=".
Add an additional check: || key_str == "=" to explicitly reject this edge case.
🤖 Prompt for AI AgentsIn crates/vm/src/stdlib/os.rs around lines 446 to 476, the current validation for Windows environment variable keys mistakenly allows a key equal to "="; update the existing conditional that rejects empty, null-containing, or '='-containing keys to also explicitly reject the single-character key "=" (e.g., add a check like key_str == "=" to the OR chain) so that a lone "=" is treated as an invalid environment variable name.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)crates/vm/src/stdlib/os.rs (1)📜 Review details444-468: Critical issue remains unresolved: key consisting of only "=" is still accepted.
The validation at lines 451-456 still allows a key of "=" to pass, despite the previous review comment flagging this exact issue. When key_str = "=", the check key_str.get(1..).is_some_and(|s| s.contains('=')) evaluates to false because the substring after index 1 is empty and does not contain '='.
Per Windows environment variable naming rules and CPython's implementation, a key consisting solely of "=" is invalid (it serves as the separator in "NAME=VALUE" format). Add an explicit check to reject this edge case.
Apply this diff to fix the validation:
if key_str.is_empty() || key_str.get(1..).is_some_and(|s| s.contains('=')) + || key_str == "=" || key_str.contains('\0') || value_str.contains('\0') {
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 7013528 and 431a043.
⛔ Files ignored due to path filters (2)- Lib/test/test_ntpath.py is excluded by !Lib/**
- Lib/test/test_unittest/testmock/testpatch.py is excluded by !Lib/**
- .github/workflows/ci.yaml (0 hunks)
- crates/vm/src/stdlib/nt.rs (2 hunks)
- crates/vm/src/stdlib/os.rs (10 hunks)
- .github/workflows/ci.yaml
📄 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/os.rs
Learnt from: arihant2math Repo: RustPython/RustPython PR: 5790 File: build.rs:2-2 Timestamp: 2025-06-28T16:31:03.991Z Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
crates/common/src/os.rs (2)⏰ 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). (11)crates/vm/src/function/fspath.rs (1)
- bytes_as_os_str (66-69)
- bytes_as_os_str (72-74)
- bytes_as_os_str (123-126)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
crates/vm/src/stdlib/os.rs (4)470-490: Non-Windows implementation looks correct.
The signature change to accept Either<PyStrRef, PyBytesRef> for both key and value aligns with CPython's behavior on Unix-like systems. The validation at line 482 correctly rejects any key containing '=', including the edge case of a key consisting solely of "=".
517-540: Non-Windows implementation looks correct.
The signature change and validation logic are appropriate. The check at line 527 correctly rejects any key containing '=', including a key consisting only of "=".
115-119: Helper function changes look correct.
The addition of #[cfg(not(windows))] to bytes_as_os_str is appropriate since Windows uses wide strings for environment variables. The update to env_bytes_as_bytes to handle Either<PyStrRef, PyBytesRef> correctly supports the new function signatures for non-Windows platforms.
Also applies to: 419-425
428-442: Windows helper functions look correct.
The unsafe extern "C" binding for _wputenv correctly follows CPython's approach of using the CRT function to update the environment. The check_env_var_len function appropriately validates against Windows' _MAX_ENV limit, accounting for the null terminator.
Sorry, something went wrong.
| // Validate: no '=' in key (search from index 1 because on Windows | ||
| // starting '=' is allowed for defining hidden environment variables) | ||
| if key_str.get(1..).is_some_and(|s| s.contains('=')) { | ||
| return Err(vm.new_value_error("illegal environment variable name")); |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Critical issue: key consisting of only "=" is incorrectly accepted.
The validation at line 369 has the same flaw as the corresponding code in os.rs. When key_str = "=", the check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false because the substring after index 1 is empty and does not contain '='.
A key consisting solely of "=" is not a valid environment variable name and should be explicitly rejected.
Apply this diff to fix the validation:
// Validate: no '=' in key (search from index 1 because on Windows
// starting '=' is allowed for defining hidden environment variables)
- if key_str.get(1..).is_some_and(|s| s.contains('=')) {
+ if key_str == "=" || key_str.get(1..).is_some_and(|s| s.contains('=')) {
return Err(vm.new_value_error("illegal environment variable name"));
}In crates/vm/src/stdlib/nt.rs around lines 367 to 370, the current validation misses the case where key_str == "=" so a single "=" is incorrectly accepted; update the validation to explicitly reject a key that is exactly "=" (return the same vm.new_value_error) before the existing substring check (or alter the condition to treat an empty substring after index 1 as invalid), ensuring any key consisting solely of "=" is treated as illegal.
Sorry, something went wrong.
| // Validate: no '=' in key (search from index 1 because on Windows | ||
| // starting '=' is allowed for defining hidden environment variables) | ||
| if key_str.get(1..).is_some_and(|s| s.contains('=')) { | ||
| return Err(vm.new_value_error("illegal environment variable name")); |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Critical issue: key consisting of only "=" is incorrectly accepted.
The validation at line 486 has the same flaw as spawnve above. A key of "=" will incorrectly pass validation because key_str.get(1..).is_some_and(|s| s.contains('=')) returns false when the substring after index 1 is empty.
Apply this diff to fix the validation:
// Validate: no '=' in key (search from index 1 because on Windows
// starting '=' is allowed for defining hidden environment variables)
- if key_str.get(1..).is_some_and(|s| s.contains('=')) {
+ if key_str == "=" || key_str.get(1..).is_some_and(|s| s.contains('=')) {
return Err(vm.new_value_error("illegal environment variable name"));
}crates/vm/src/stdlib/nt.rs around lines 484 to 487: the current check allows a
key of "=" because get(1..) is None and the condition is false; change the
validation to also reject a lone "=" by updating the conditional to fail when
key_str == "=" or when key_str.get(1..).is_some_and(|s| s.contains('=')).
Replace the existing if condition with a compound check that returns Err for
either case.
Sorry, something went wrong.
| #[cfg(windows)] | ||
| #[pyfunction] | ||
| fn unsetenv(key: PyStrRef, vm: &VirtualMachine) -> PyResult<()> { | ||
| let key_str = key.as_str(); | ||
| // Search from index 1 because on Windows starting '=' is allowed for | ||
| // defining hidden environment variables. | ||
| if key_str.is_empty() | ||
| || key_str.get(1..).is_some_and(|s| s.contains('=')) | ||
| || key_str.contains('\0') | ||
| { | ||
| return Err(vm.new_value_error("illegal environment variable name")); | ||
| } | ||
| // "key=" to unset (empty value removes the variable) | ||
| let env_str = format!("{}=", key_str); | ||
| let wide = env_str.to_wide_with_nul(); | ||
| check_env_var_len(wide.len(), vm)?; | ||
|
|
||
| // Use _wputenv like CPython (not SetEnvironmentVariableW) to update CRT environ | ||
| let result = unsafe { suppress_iph!(_wputenv(wide.as_ptr())) }; | ||
| if result != 0 { | ||
| return Err(vm.new_last_errno_error()); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
Critical issue: key consisting of only "=" is incorrectly accepted.
The validation at lines 498-502 has the same flaw as putenv above. When key_str = "=", the check key_str.get(1..).is_some_and(|s| s.contains('=')) returns false because the substring after index 1 is empty.
A key of just "=" is not a valid environment variable name and should be rejected.
Apply this diff to fix the validation:
if key_str.is_empty()
|| key_str.get(1..).is_some_and(|s| s.contains('='))
+ || key_str == "="
|| key_str.contains('\0')
{In crates/vm/src/stdlib/os.rs around lines 492 to 515, the validation allows a
key consisting only of "=" because checking key_str.get(1..) misses the case
where the first character is '='; update the guard to also reject keys that
start with '=' (e.g., key_str.starts_with('=') or key_str.chars().next() ==
Some('=')) so that a lone "=" or any name beginning with '=' is treated as
invalid, while keeping the existing empty and NUL checks and the subsequent
logic unchanged.
Sorry, something went wrong.
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.