GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — test_os, test_io on windows by youknowone · Pull Request #6379 · RustPython/RustPython Conversation
|
WalkthroughRemoved Windows CI skips for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📓 Common learnings⏰ 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)
🔇 Additional comments (7)
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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
81b279c to
ef41136
Compare
ef41136 to
194e2a6
Compare
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
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
putenvfunction incorrectly raisesValueErrorfor invalid keys, but CPython'sos.putenvraisesOSError(likeunsetenvdoes). Change"illegal environment variable name"error on line 483 to matchunsetenv'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.
🧹 Nitpick comments (2)
crates/vm/src/stdlib/os.rs (2)
428-437: Unused helper functioncheck_env_var_len.This helper function is defined but never called. The new Windows
putenvandunsetenvimplementations (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 + 1differs from what this helper would check (size > _MAX_ENV), which could lead to off-by-one discrepancies.
492-520: Duplicated key validation logic withputenv.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(()) +}
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/os.rs(2 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/os.rs
⏰ 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)
- 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
🔇 Additional comments (1)
crates/vm/src/stdlib/os.rs (1)
439-468: Verify_MAX_ENVis in scope for this function.The function references
_MAX_ENVat lines 455 and 458, but the only visible import is insidecheck_env_var_len(line 429), which is scoped to that function. Ensure there's a module-level import for_MAX_ENVor 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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
e3dc792 to
016c575
Compare
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 (3)
crates/vm/src/stdlib/os.rs (3)
425-437: Unusedcheck_env_var_lenhelper and stale doc comment
check_env_var_lenis no longer called, and its comment aboutkey.len() + value.len() + 2doesn’t match the current wide-string-based_MAX_ENVchecks inputenv/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/unsetenvpaths (takingwide.len()or similar), and updating the comment accordingly; or- Dropping the helper entirely and keeping the length checks inline.
495-522: Windowsunsetenv: mirrorsputenvcorrectly; consider sharing logicThis Windows-specific
unsetenvmirrorsputenv’s validation rules and uses_wputenv("KEY=")to remove the variable, which matches typical CRT/CPython behavior. Error handling vianew_value_errorandnew_last_errno_erroralso 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_ENVlength 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_ENVsemantics for your MSVC toolchain version and thatKEY=indeed removes the variable rather than leaving an empty one.
439-469: Windowsputenv: validation logic is duplicated; confirm bytes API change was intentionalThe Windows
putenvimplementation looks correct: it enforces non-empty names, allows a leading=, rejects additional=and embedded NULs, checks the_MAX_ENVlimit using the wide string length, and uses_wputenvto 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 betweenputenv(lines 445–454, 456–460) andunsetenv(lines 477–487, 489–494). Pulling them into small helpers (e.g.validate_env_name(&str, vm)?andvalidate_env_length(&[u16], vm)?) would reduce duplication and keep Windows-specific rules in one place.Regarding the signature change to
PyStrRefonly: the current code already accepts onlystr, notbytes. If this was a recent change fromEither<PyStrRef, PyBytesRef>, confirm that the behavioral change is intentional and acceptable (it aligns with CPython on Windows, which accepts text only).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/os.rs(2 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/os.rs
⏰ 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)
- 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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
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: 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_lenfunction 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, whilecheck_env_var_lenexpectssizeto bekey.len() + value.len() + 2. The function signature would need adjustment to match the actual usage pattern.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/os.rs(4 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/os.rs
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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)
- 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
🔇 Additional comments (6)
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
_wputenvwith an empty value ("key=") to unset environment variables on Windows, matching CPython's approach. The validation logic is consistent with the Windowsputenvimplementation, 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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| #[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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 -20
Repository: 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=rs
Repository: 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 Agents
In 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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
7013528 to
431a043
Compare
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: 3
♻️ Duplicate comments (1)
crates/vm/src/stdlib/os.rs (1)
444-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. Whenkey_str = "=", the checkkey_str.get(1..).is_some_and(|s| s.contains('='))evaluates tofalsebecause 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') {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_ntpath.pyis excluded by!Lib/**Lib/test/test_unittest/testmock/testpatch.pyis excluded by!Lib/**
📒 Files selected for processing (3)
.github/workflows/ci.yaml(0 hunks)crates/vm/src/stdlib/nt.rs(2 hunks)crates/vm/src/stdlib/os.rs(10 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yaml
🧰 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/nt.rscrates/vm/src/stdlib/os.rs
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (1)
crates/vm/src/stdlib/os.rs (2)
crates/common/src/os.rs (2)
bytes_as_os_str(66-69)bytes_as_os_str(72-74)crates/vm/src/function/fspath.rs (1)
bytes_as_os_str(123-126)
⏰ 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)
- 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)
🔇 Additional comments (4)
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))]tobytes_as_os_stris appropriate since Windows uses wide strings for environment variables. The update toenv_bytes_as_bytesto handleEither<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_wputenvcorrectly follows CPython's approach of using the CRT function to update the environment. Thecheck_env_var_lenfunction appropriately validates against Windows'_MAX_ENVlimit, accounting for the null terminator.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"));
}🤖 Prompt for AI Agents
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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"));
}🤖 Prompt for AI Agents
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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| #[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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')
{🤖 Prompt for AI Agents
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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
a8f1e26 to
df899ea
Compare
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.