Merged
Conversation
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] test: cpython/Lib/test/test_mmap.py (TODO: 1) dependencies: dependent tests: (2 tests)
[x] lib: cpython/Lib/subprocess.py dependencies:
dependent tests: (81 tests)
[x] test: cpython/Lib/test/test_sys.py (TODO: 7) dependencies: dependent tests: (225 tests)
Legend:
|
Contributor
📝 WalkthroughWalkthroughThis pull request refines Windows-specific standard library implementations across four files: adjusts exception types in memory mapping operations, fixes environment block serialization for empty mappings, removes deprecated WinSock constants, and prevents direct instantiation of the Windows version struct. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
aaf925a to
68a394f
Compare
Add slot_new to PyWindowsVersion that raises TypeError, matching sys.flags behavior.
WSAHOS was hardcoded as an alias for WSAHOST_NOT_FOUND, but CPython guards it with #ifdef WSAHOS which doesn't exist in modern Windows SDK headers.
Empty env dict produced a single null terminator, but CreateProcessW requires a double null for a valid empty environment block.
mmap resize raises SystemError (not OSError) when mremap is unavailable, matching CPython behavior. test_mmap catches SystemError to skip unsupported resize operations.
…Failure Named mmap resize on Windows should raise OSError (not SystemError). Remove expectedFailure mark from TimeEINTRTest.test_sleep as it now passes.
test_sleep passes on macOS but fails on Linux due to timing.
test_sleep now passes on all platforms.
Contributor
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/_winapi.rs (1)
425-429:⚠️ Potential issue | 🔴 CriticalFix UTF-8 slicing on environment variable names to prevent panics.
Line 428 uses byte-level slicing
k[1..]on a UTF-8&str. When an environment variable name starts with a multi-byte character (e.g.,Ä=value), Rust panics because the slice index 1 lands in the middle of the multi-byte sequence rather than at a valid UTF-8 boundary. This crashes the VM instead of raisingValueError.Suggested fix
- if k.is_empty() || k[1..].contains('=') { + if k.is_empty() || k.chars().skip(1).any(|ch| ch == '=') { return Err(vm.new_value_error("illegal environment variable name")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_winapi.rs` around lines 425 - 429, The code currently slices the UTF-8 string k with k[1..], which can panic for multi-byte characters; replace the byte-slice check with a char-aware check such as using k.chars().skip(1).any(|c| c == '=') so the condition becomes if k.is_empty() || k.chars().skip(1).any(|c| c == '=') { return Err(vm.new_value_error("illegal environment variable name")); } to avoid UTF-8 boundary panics while keeping the same semantics for k and v validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/errno.rs`:
- Around line 42-46: The errno module is missing the Windows constant
WSAHOST_NOT_FOUND in its exported WSA error list; update the export list in
crates/vm/src/stdlib/errno.rs by replacing the placeholder/TODO (WSAHOS) with
the full constant name WSAHOST_NOT_FOUND so the module exposes that constant
(used by crates/stdlib/src/socket.rs for EAI mapping).
---
Outside diff comments:
In `@crates/vm/src/stdlib/_winapi.rs`:
- Around line 425-429: The code currently slices the UTF-8 string k with k[1..],
which can panic for multi-byte characters; replace the byte-slice check with a
char-aware check such as using k.chars().skip(1).any(|c| c == '=') so the
condition becomes if k.is_empty() || k.chars().skip(1).any(|c| c == '=') {
return Err(vm.new_value_error("illegal environment variable name")); } to avoid
UTF-8 boundary panics while keeping the same semantics for k and v validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6d320c3e-6392-41ac-9255-594c41d0ab3e
⛔ Files ignored due to path filters (3)
Lib/test/test_mmap.pyis excluded by!Lib/**Lib/test/test_subprocess.pyis excluded by!Lib/**Lib/test/test_sys.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/stdlib/src/mmap.rscrates/vm/src/stdlib/_winapi.rscrates/vm/src/stdlib/errno.rscrates/vm/src/stdlib/sys.rs
Comment on lines
+42
to
+46
| WSAESTALE, WSAETIMEDOUT, WSAETOOMANYREFS, WSAEUSERS, WSAEWOULDBLOCK, WSAID_ACCEPTEX, | ||
| WSAID_CONNECTEX, WSAID_DISCONNECTEX, WSAID_GETACCEPTEXSOCKADDRS, WSAID_TRANSMITFILE, | ||
| WSAID_TRANSMITPACKETS, WSAID_WSAPOLL, WSAID_WSARECVMSG, WSANO_DATA, WSANO_RECOVERY, | ||
| WSANOTINITIALISED, WSAPROTOCOL_LEN, WSASERVICE_NOT_FOUND, WSASYS_STATUS_LEN, | ||
| WSASYSCALLFAILURE, WSASYSNOTREADY, WSATRY_AGAIN, WSATYPE_NOT_FOUND, WSAVERNOTSUPPORTED, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the errno.rs file at the specified lines to see the current state
wc -l crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 98
🏁 Script executed:
# Check what's at lines 42-46 in errno.rs
sed -n '40,50p' crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 785
🏁 Script executed:
# Check what's at line 566 in errno.rs
sed -n '560,575p' crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 665
🏁 Script executed:
# Search for WSAHOST_NOT_FOUND in the codebase
rg "WSAHOST_NOT_FOUND" --type rustRepository: RustPython/RustPython
Length of output: 170
🏁 Script executed:
# Check socket.rs around lines 85-95 for the claimed usage
sed -n '80,100p' crates/stdlib/src/socket.rsRepository: RustPython/RustPython
Length of output: 1371
🏁 Script executed:
# Check if WSAHOS is defined or referenced elsewhere
rg "WSAHOS" --type rust -B2 -A2Repository: RustPython/RustPython
Length of output: 888
🏁 Script executed:
# Check broader context around the TODO at line 566
sed -n '560,575p' crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 665
🏁 Script executed:
# Look at the import statements at the top of errno.rs to understand the Windows socket setup
sed -n '1,50p' crates/vm/src/stdlib/errno.rs | head -40Repository: RustPython/RustPython
Length of output: 1695
🏁 Script executed:
# Check if there are any comments explaining why WSAHOST_NOT_FOUND was removed
rg -B5 -A5 "WSAHOST_NOT_FOUND|WSAHOS" crates/vm/src/stdlib/errno.rsRepository: RustPython/RustPython
Length of output: 468
Add WSAHOST_NOT_FOUND to the errno module's Windows error exports.
errno no longer exposes WSAHOST_NOT_FOUND at the module level, even though it's a standard Winsock error code used in crates/stdlib/src/socket.rs for EAI mapping. While socket.rs imports this constant directly from windows_sys, the omission leaves the errno module incomplete on Windows. Replace the WSAHOS TODO comment with the correct full constant name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/errno.rs` around lines 42 - 46, The errno module is
missing the Windows constant WSAHOST_NOT_FOUND in its exported WSA error list;
update the export list in crates/vm/src/stdlib/errno.rs by replacing the
placeholder/TODO (WSAHOS) with the full constant name WSAHOST_NOT_FOUND so the
module exposes that constant (used by crates/stdlib/src/socket.rs for EAI
mapping).
Copilot AI
pushed a commit
that referenced
this pull request
Mar 25, 2026
* Disallow instantiation of sys.getwindowsversion type Add slot_new to PyWindowsVersion that raises TypeError, matching sys.flags behavior. * Remove incorrect WSAHOS errno constant WSAHOS was hardcoded as an alias for WSAHOST_NOT_FOUND, but CPython guards it with #ifdef WSAHOS which doesn't exist in modern Windows SDK headers. * Fix mmap resize to raise OSError instead of SystemError * Fix CreateProcess with empty environment on Windows Empty env dict produced a single null terminator, but CreateProcessW requires a double null for a valid empty environment block. * Revert mmap resize error to SystemError and fix errno.rs formatting mmap resize raises SystemError (not OSError) when mremap is unavailable, matching CPython behavior. test_mmap catches SystemError to skip unsupported resize operations. * Fix named mmap resize to raise OSError and unmark test_sleep expectedFailure Named mmap resize on Windows should raise OSError (not SystemError). Remove expectedFailure mark from TimeEINTRTest.test_sleep as it now passes. * Use expectedFailureIf for TimeEINTRTest.test_sleep on Linux test_sleep passes on macOS but fails on Linux due to timing. * Remove expectedFailure for TimeEINTRTest.test_sleep test_sleep now passes on all platforms.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary by CodeRabbit
Bug Fixes
Changes