Update urllib and windows libraries from v3.14.2 #6858
Conversation
📝 WalkthroughWalkthroughThese changes extend the Windows API bindings for RustPython by adding msvcrt functions (ungetch, ungetwch, kbhit, locking, heapmin), exposing FileSystem constants, and implementing a CreateFile wrapper. Update scripts are enhanced with explicit UTF-8 encoding and improved extension module test resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin win-lib |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/msvcrt.rs`:
- Line 36: The FFI declaration fn _locking and the public wrapper locking() use
i64 for the nbytes parameter which mismatches the Windows CRT ABI (should be
32-bit long); change the third parameter type from i64 to i32 in the extern "C"
fn _locking declaration and in the pub fn locking(...) wrapper signature and any
internal uses/casts in that wrapper to pass an i32. Ensure you do not try to
reference a non-existent _locking64 export; if larger ranges are needed, use
Win32 LockFile/LockFileEx instead.
🧹 Nitpick comments (1)
crates/vm/src/stdlib/msvcrt.rs (1)
122-125: Consider returningboolforkbhit()to match CPython.CPython's
msvcrt.kbhit()returns a boolean (True/False), while this implementation returns the rawi32. The current implementation works but doesn't strictly match Python semantics.♻️ Optional: Return bool for Python compatibility
#[pyfunction] -fn kbhit() -> i32 { - unsafe { _kbhit() } +fn kbhit() -> bool { + unsafe { _kbhit() != 0 } }
| fn _putwch(c: u16) -> u32; | ||
| fn _ungetch(c: i32) -> i32; | ||
| fn _ungetwch(c: u32) -> u32; | ||
| fn _locking(fd: i32, mode: i32, nbytes: i64) -> i32; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd crates/vm/src/stdlib && wc -l msvcrt.rsRepository: RustPython/RustPython
Length of output: 78
🏁 Script executed:
cd crates/vm/src/stdlib && sed -n '30,45p' msvcrt.rsRepository: RustPython/RustPython
Length of output: 535
🏁 Script executed:
cd crates/vm/src/stdlib && sed -n '120,140p' msvcrt.rsRepository: RustPython/RustPython
Length of output: 595
🌐 Web query:
Windows CRT _locking function signature nbytes parameter type int long
💡 Result:
In the Microsoft C Runtime (CRT), _locking has this signature:
int _locking(int fd, int mode, long nbytes);So the nbytes parameter type is long (signed long), not int. [1]
Source: Microsoft Learn _locking documentation. [1]
[1] (learn.microsoft.com)
Citations:
🌐 Web query:
_locking vs _locking64 Windows C runtime function difference
💡 Result:
-
_lockingis the (documented) MSVC C runtime function that locks/unlocks a byte range starting at the current file pointer fornbytesbytes, wherenbytesis a 32-bitlong:
int _locking(int fd, int mode, long nbytes);This means the length you can request in a single call is limited by 32-bitlongrange (andlongstays 32-bit even on 64-bit Windows). [1] -
_locking64is not a standard/documented MSVC CRT API (at least not in the classicmsvcrt.dllexport surface):msvcrt.dllexports_lockingbut does not list an_locking64export. [2] A separate MSVCRT export-definition listing likewise shows_lockingbut not_locking64. [3]
What to use instead (for “64-bit”/large-range locks): use the Win32 file-locking APIs (LockFile / LockFileEx), which support 64-bit ranges via low/high parts. [4]
Sources:
[1] Microsoft Learn: _locking signature/behavior https://learn.microsoft.com/en-us/previous-versions/8054ew2f(v=vs.140)
[2] msvcrt.dll exported functions list (shows _locking, no _locking64) https://strontic.github.io/xcyclopedia/library/msvcrt.dll-6297CA08C873060692FF97074895FA95.html
[3] MSVCRT export .def listing (shows _locking, no _locking64) https://www-user.tu-chemnitz.de/~heha/viewzip.php/hs/msvcrt-light.zip/src/msvcrt.winxp-x64.def?as=DEF
[4] Microsoft Learn: byte-range locking overview (LockFile / LockFileEx) https://learn.microsoft.com/en-us/windows/win32/fileio/locking-and-unlocking-byte-ranges-in-files
Change nbytes parameter from i64 to i32 to match Windows CRT ABI.
The Windows CRT _locking function signature specifies long (32-bit on Windows, even on 64-bit systems) for the nbytes parameter:
int _locking(int fd, int mode, long nbytes);Using i64 at line 36 and in the wrapper function (lines 127-135) causes an ABI mismatch that can lead to undefined behavior. Change to i32 for both the FFI declaration and the public locking() function signature. Note: _locking64 is not a standard export in msvcrt.dll; if 64-bit file range locking is needed, use Win32 APIs like LockFile or LockFileEx.
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/msvcrt.rs` at line 36, The FFI declaration fn _locking
and the public wrapper locking() use i64 for the nbytes parameter which
mismatches the Windows CRT ABI (should be 32-bit long); change the third
parameter type from i64 to i32 in the extern "C" fn _locking declaration and in
the pub fn locking(...) wrapper signature and any internal uses/casts in that
wrapper to pass an i32. Ensure you do not try to reference a non-existent
_locking64 export; if larger ranges are needed, use Win32 LockFile/LockFileEx
instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/stdlib/winapi.rs`:
- Around line 119-153: The CreateFile wrapper currently ignores the passed-in
_security_attributes and _template_file but still accepts non-None values;
update CreateFile to validate these parameters (the _security_attributes and
_template_file PyObjectRef args) and reject any non-None input by returning a
Python error via the VirtualMachine (e.g., Err(vm.new_type_error(...)) or
vm.new_value_error(...)) with a clear message like "security_attributes must be
None" / "template_file must be None"; perform these checks before calling
CreateFileW so callers are explicitly informed the function does not support
non-None security/template arguments.
♻️ Duplicate comments (1)
crates/vm/src/stdlib/msvcrt.rs (1)
34-38: ABI mismatch:_lockingexpects 32‑bitlong, noti64.Using
i64risks UB in FFI calls. This matches the earlier review finding; please adjust both the extern signature and wrapper parameter toi32.🛠️ Proposed fix
- fn _locking(fd: i32, mode: i32, nbytes: i64) -> i32; + fn _locking(fd: i32, mode: i32, nbytes: i32) -> i32; @@ - fn locking(fd: i32, mode: i32, nbytes: i64, vm: &VirtualMachine) -> PyResult<()> { + fn locking(fd: i32, mode: i32, nbytes: i32, vm: &VirtualMachine) -> PyResult<()> {MSVC _locking function signature and nbytes parameter type (long vs long long)Also applies to: 127-135
| /// CreateFile - Create or open a file or I/O device. | ||
| #[pyfunction] | ||
| #[allow(clippy::too_many_arguments)] | ||
| fn CreateFile( | ||
| file_name: PyStrRef, | ||
| desired_access: u32, | ||
| share_mode: u32, | ||
| _security_attributes: PyObjectRef, // Always NULL (0) | ||
| creation_disposition: u32, | ||
| flags_and_attributes: u32, | ||
| _template_file: PyObjectRef, // Always NULL (0) | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<WinHandle> { | ||
| use windows_sys::Win32::Storage::FileSystem::CreateFileW; | ||
|
|
||
| let file_name_wide = file_name.as_wtf8().to_wide_with_nul(); | ||
|
|
||
| let handle = unsafe { | ||
| CreateFileW( | ||
| file_name_wide.as_ptr(), | ||
| desired_access, | ||
| share_mode, | ||
| null(), | ||
| creation_disposition, | ||
| flags_and_attributes, | ||
| null_mut(), | ||
| ) | ||
| }; | ||
|
|
||
| if handle == INVALID_HANDLE_VALUE { | ||
| return Err(vm.new_last_os_error()); | ||
| } | ||
|
|
||
| Ok(WinHandle(handle)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CPython _winapi CreateFile security_attributes template_file implementation
💡 Result:
In CPython, _winapi.CreateFile is a very thin wrapper over the Win32 CreateFile API:
-
Signature in
Modules/_winapi.c(shown here from CPython v3.9.7) is:_winapi.CreateFile(file_name, desired_access, share_mode, security_attributes, creation_disposition, flags_and_attributes, template_file) -> HANDLEand the implementation just calls
CreateFile(file_name, ..., security_attributes, ..., template_file)(wrapped inPy_BEGIN_ALLOW_THREADS) and raisesOSErroronINVALID_HANDLE_VALUE. (chromium.googlesource.com) -
security_attributesis declared asLPSECURITY_ATTRIBUTES, but Argument Clinic definesLPSECURITY_ATTRIBUTESas a raw pointer-sized integer (“k” on 32-bit, “K” on 64-bit) (i.e., an address), not a Python object representing aSECURITY_ATTRIBUTESstruct. (chromium.googlesource.com)
Practically: pass0to meanNULL(no security attrs / non-inheritable handle by default), or pass an integer address to a validSECURITY_ATTRIBUTESstruct you created elsewhere in native code. -
template_fileis aHANDLE(also passed straight through). Pass0/NULLfor “no template file”. (chromium.googlesource.com)
For the Win32 meaning of these two parameters:
lpSecurityAttributescan beNULL; it controls the optional security descriptor and whether the returned handle is inheritable. (learn.microsoft.com)hTemplateFilecan beNULL; it supplies attributes/extended attributes for a newly created file and is ignored when opening an existing file. (learn.microsoft.com)
Citations:
- 1: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/tags/v3.9.7/Modules/_winapi.c
- 2: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/tags/v3.9.7/Modules/_winapi.c
- 3: https://chromium.googlesource.com/external/github.com/python/cpython/%2B/refs/tags/v3.9.7/Modules/_winapi.c
- 4: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea?utm_source=openai
- 5: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea?utm_source=openai
🌐 Web query:
Windows API CreateFileW security_attributes template_file parameter significance
💡 Result:
lpSecurityAttributes (4th parameter)
Controls (a) the new object’s security and (b) whether the returned handle is inheritable:
- Points to a
SECURITY_ATTRIBUTEScontaining:lpSecurityDescriptor: security descriptor used only when a new file/object is created; ignored when opening an existing file, but…bInheritHandle: still controls whether the returned handle can be inherited by child processes. (learn.microsoft.com)
- If
lpSecurityAttributes == NULL:- the handle is not inheritable
- a newly created file gets a default security descriptor (ACLs typically inherited from the parent directory). (learn.microsoft.com)
hTemplateFile (7th parameter)
Provides attributes/extended attributes to copy when creating a new file:
- A handle to an existing template file (must be open with
GENERIC_READ) whose file attributes and extended attributes are applied to the new file. (learn.microsoft.com) - Ignored when opening an existing file. (learn.microsoft.com)
- When creating, if a template is supplied, its extended attributes are copied and can override
FILE_ATTRIBUTE_*choices you passed indwFlagsAndAttributes. (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew?utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew?utm_source=openai
- 3: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew?utm_source=openai
- 4: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew?utm_source=openai
- 5: https://learn.microsoft.com/en-us/windows/win32/fileio/creating-and-opening-files?utm_source=openai
🏁 Script executed:
# Check if there are any tests or other code in the repository that might use CreateFile with these parameters
fd -t f "\.rs$" | xargs rg -l "CreateFile" | head -20Repository: RustPython/RustPython
Length of output: 147
🏁 Script executed:
# Check if there are any test files or usage examples for CreateFile
rg -t rs "CreateFile" crates/vm/src --context 5
# Also check if there are tests specifically for this function
fd -t f "test.*\.rs|.*_test\.rs" | xargs rg -l "CreateFile" 2>/dev/null || echo "No test files found with CreateFile"Repository: RustPython/RustPython
Length of output: 144
🏁 Script executed:
# Check the implementation context to understand how other similar parameters are handled
cat -n crates/vm/src/stdlib/winapi.rs | head -200Repository: RustPython/RustPython
Length of output: 8794
Add guard to reject non-None security_attributes and template_file parameters.
The function signature accepts these as PyObjectRef, but silently ignores non-None values and always passes NULL to the Win32 API. This can mislead callers who pass non-None values expecting ACL, handle inheritance, or template attribute behavior. If support isn't ready, explicitly reject non-None values to make the limitation clear.
Suggested implementation
fn CreateFile(
file_name: PyStrRef,
desired_access: u32,
share_mode: u32,
_security_attributes: PyObjectRef,
creation_disposition: u32,
flags_and_attributes: u32,
_template_file: PyObjectRef,
vm: &VirtualMachine,
) -> PyResult<WinHandle> {
use windows_sys::Win32::Storage::FileSystem::CreateFileW;
+ if !_security_attributes.is(&vm.ctx.none()) || !_template_file.is(&vm.ctx.none()) {
+ return Err(vm.new_not_implemented_error(
+ "security_attributes and template_file are not supported yet".to_owned(),
+ ));
+ }
+
let file_name_wide = file_name.as_wtf8().to_wide_with_nul();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// CreateFile - Create or open a file or I/O device. | |
| #[pyfunction] | |
| #[allow(clippy::too_many_arguments)] | |
| fn CreateFile( | |
| file_name: PyStrRef, | |
| desired_access: u32, | |
| share_mode: u32, | |
| _security_attributes: PyObjectRef, // Always NULL (0) | |
| creation_disposition: u32, | |
| flags_and_attributes: u32, | |
| _template_file: PyObjectRef, // Always NULL (0) | |
| vm: &VirtualMachine, | |
| ) -> PyResult<WinHandle> { | |
| use windows_sys::Win32::Storage::FileSystem::CreateFileW; | |
| let file_name_wide = file_name.as_wtf8().to_wide_with_nul(); | |
| let handle = unsafe { | |
| CreateFileW( | |
| file_name_wide.as_ptr(), | |
| desired_access, | |
| share_mode, | |
| null(), | |
| creation_disposition, | |
| flags_and_attributes, | |
| null_mut(), | |
| ) | |
| }; | |
| if handle == INVALID_HANDLE_VALUE { | |
| return Err(vm.new_last_os_error()); | |
| } | |
| Ok(WinHandle(handle)) | |
| } | |
| /// CreateFile - Create or open a file or I/O device. | |
| #[pyfunction] | |
| #[allow(clippy::too_many_arguments)] | |
| fn CreateFile( | |
| file_name: PyStrRef, | |
| desired_access: u32, | |
| share_mode: u32, | |
| _security_attributes: PyObjectRef, // Always NULL (0) | |
| creation_disposition: u32, | |
| flags_and_attributes: u32, | |
| _template_file: PyObjectRef, // Always NULL (0) | |
| vm: &VirtualMachine, | |
| ) -> PyResult<WinHandle> { | |
| use windows_sys::Win32::Storage::FileSystem::CreateFileW; | |
| if !_security_attributes.is(&vm.ctx.none()) || !_template_file.is(&vm.ctx.none()) { | |
| return Err(vm.new_not_implemented_error( | |
| "security_attributes and template_file are not supported yet".to_owned(), | |
| )); | |
| } | |
| let file_name_wide = file_name.as_wtf8().to_wide_with_nul(); | |
| let handle = unsafe { | |
| CreateFileW( | |
| file_name_wide.as_ptr(), | |
| desired_access, | |
| share_mode, | |
| null(), | |
| creation_disposition, | |
| flags_and_attributes, | |
| null_mut(), | |
| ) | |
| }; | |
| if handle == INVALID_HANDLE_VALUE { | |
| return Err(vm.new_last_os_error()); | |
| } | |
| Ok(WinHandle(handle)) | |
| } |
🤖 Prompt for AI Agents
In `@crates/vm/src/stdlib/winapi.rs` around lines 119 - 153, The CreateFile
wrapper currently ignores the passed-in _security_attributes and _template_file
but still accepts non-None values; update CreateFile to validate these
parameters (the _security_attributes and _template_file PyObjectRef args) and
reject any non-None input by returning a Python error via the VirtualMachine
(e.g., Err(vm.new_type_error(...)) or vm.new_value_error(...)) with a clear
message like "security_attributes must be None" / "template_file must be None";
perform these checks before calling CreateFileW so callers are explicitly
informed the function does not support non-None security/template arguments.
youknowone
changed the title
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [+] lib: cpython/Lib/ntpath.py dependencies:
dependent tests: (3 tests)
[+] lib: cpython/Lib/nturl2path.py dependencies:
dependent tests: (no tests depend on nturl2path) [+] lib: cpython/Lib/genericpath.py dependencies:
dependent tests: (16 tests)
[+] test: cpython/Lib/test/test_msvcrt.py dependencies: dependent tests: (1 tests)
[+] test: cpython/Lib/test/test_robotparser.py dependencies: dependent tests: (no tests depend on robotparser) [+] lib: cpython/Lib/urllib dependencies:
dependent tests: (26 tests)
[+] test: cpython/Lib/test/test_urllib2.py dependencies: dependent tests: (no tests depend on urllib2) [+] test: cpython/Lib/test/test_urllib2_localnet.py dependencies: dependent tests: (no tests depend on urllib2_localnet) [+] test: cpython/Lib/test/test_urllib2net.py dependencies: dependent tests: (no tests depend on urllib2net) [+] test: cpython/Lib/test/test_urllibnet.py dependencies: dependent tests: (no tests depend on urllibnet) [+] test: cpython/Lib/test/test_urlparse.py dependencies: dependent tests: (no tests depend on urlparse) [+] test: cpython/Lib/test/test_winapi.py dependencies: dependent tests: (no tests depend on winapi) Legend:
|
1 participant
#6839
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.