impl more nt - #6984
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughWindows-specific codecs and extensive Windows stdlib changes were added (code-page encode/decode, codec delegation), large rewrites to nt.rs (symlink, path normalization, reparse-point and attribute APIs), a tiny exceptions newline-trim tweak, and tighter env-var validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python caller
participant VM as VirtualMachine/_codecs
participant Sub as _codecs_windows (submodule)
participant Win as Windows API
Py->>VM: call codec function (e.g., code_page_encode)
alt on non-Windows
VM->>VM: delegate_pycodecs! -> route to registered PyCodecs
VM-->>Py: return result
else on Windows
VM->>Sub: call Windows-specific wrapper
Sub->>Win: WideCharToMultiByte / MultiByteToWideChar
Win-->>Sub: bytes/string + status
Sub-->>VM: result
VM-->>Py: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 inconclusive)
✏️ 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. ❤️ 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: 3
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/nt.rs (1)2262-2294: ⚠️ Potential issue | 🟠 Major
Change ValueError to OSError for non-symlink reparse tags.
readlink should raise OSError (with EINVAL errno), not ValueError, to match POSIX semantics when the reparse point is not a symlink or junction.
The proposed fix references ERROR_NOT_A_REPARSE_POINT, which does not exist in windows_sys. Instead, use libc::EINVAL:
🛠️ Corrected error mapping- } else { - return Err(vm.new_value_error("not a symbolic link".to_owned())); - }; + } else { + let err = io::Error::from_raw_os_error(libc::EINVAL); + return Err(OSErrorBuilder::with_filename(&err, path, vm)); + };
In `@crates/vm/src/stdlib/codecs.rs`: - Around line 770-865: The decode currently falls back to non-strict decoding whenever the first MultiByteToWideChar call returns 0; change code in the code_page_decode path to check args.errors (the _errors variable) when strict_flags uses MB_ERR_INVALID_CHARS: if size == 0 and _errors == "strict" then return vm.new_unicode_decode_error(...) (use the same error formatting as other branches) instead of falling back to call MultiByteToWideChar with flags 0; otherwise (when _errors != "strict") keep the existing fallback logic. Reference symbols: args.errors / _errors, strict_flags, MB_ERR_INVALID_CHARS, MultiByteToWideChar, vm.new_unicode_decode_error, and the code_page_decode return paths. In `@crates/vm/src/stdlib/nt.rs`: - Around line 573-666: The disk_device check in _test_file_type_by_name incorrectly omits network filesystems so regular files on UNC/share paths aren't treated as disk devices; update the matches!(...) in _test_file_type_by_name to include FILE_DEVICE_NETWORK_FILE_SYSTEM (import it from windows_sys::Win32::System::Ioctl) alongside FILE_DEVICE_DISK, FILE_DEVICE_VIRTUAL_DISK, and FILE_DEVICE_CD_ROM so _test_info(...) correctly treats network-mounted regular files as disk devices and allows the rest of the logic (including the handle-based fallback) to run. - Around line 1287-1333: In _getfinalpathname, the CreateFileW call opens the file with access 0 and share mode 0 which causes sharing violations; change the access parameter to FILE_READ_ATTRIBUTES and the sharemode parameter to FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE (matching other stdlib patterns) when calling CreateFileW so the handle is opened permissively for metadata reads; update the CreateFileW invocation in the _getfinalpathname function accordingly and keep the rest of the logic (GetFinalPathNameByHandleW/CloseHandle) unchanged.
crates/vm/src/stdlib/nt.rs (1)1585-1750: Add tests for the new Windows normpath implementation.
This is a sizeable port; consider tests for UNC/device paths, drive-relative forms, dot/dotdot handling, trailing separators, and bytes input to guard parity.
Sorry, something went wrong.
| if args.code_page < 0 { | ||
| return Err(vm.new_value_error("invalid code page number".to_owned())); | ||
| } | ||
| let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); | ||
| let code_page = args.code_page as u32; | ||
| let data = args.data.borrow_buf(); | ||
| let len = data.len(); | ||
|
|
||
| if data.is_empty() { | ||
| return Ok((String::new(), 0)); | ||
| } | ||
|
|
||
| // Some code pages don't support MB_ERR_INVALID_CHARS | ||
| let strict_flags = if code_page == 65000 | ||
| || code_page == 42 | ||
| || (50220..=50222).contains(&code_page) | ||
| || code_page == 50225 | ||
| || code_page == 50227 | ||
| || code_page == 50229 | ||
| || (57002..=57011).contains(&code_page) | ||
| { | ||
| 0 | ||
| } else { | ||
| MB_ERR_INVALID_CHARS | ||
| }; | ||
|
|
||
| let size = unsafe { | ||
| MultiByteToWideChar( | ||
| code_page, | ||
| strict_flags, | ||
| data.as_ptr().cast(), | ||
| len as i32, | ||
| std::ptr::null_mut(), | ||
| 0, | ||
| ) | ||
| }; | ||
|
|
||
| if size == 0 { | ||
| let size = unsafe { | ||
| MultiByteToWideChar( | ||
| code_page, | ||
| 0, | ||
| data.as_ptr().cast(), | ||
| len as i32, | ||
| std::ptr::null_mut(), | ||
| 0, | ||
| ) | ||
| }; | ||
| if size == 0 { | ||
| let err = std::io::Error::last_os_error(); | ||
| return Err(vm.new_os_error(format!("code_page_decode failed: {err}"))); | ||
| } | ||
|
|
||
| let mut buffer = vec![0u16; size as usize]; | ||
| let result = unsafe { | ||
| MultiByteToWideChar( | ||
| code_page, | ||
| 0, | ||
| data.as_ptr().cast(), | ||
| len as i32, | ||
| buffer.as_mut_ptr(), | ||
| size, | ||
| ) | ||
| }; | ||
| if result == 0 { | ||
| let err = std::io::Error::last_os_error(); | ||
| return Err(vm.new_os_error(format!("code_page_decode failed: {err}"))); | ||
| } | ||
| buffer.truncate(result as usize); | ||
| let s = String::from_utf16(&buffer).map_err(|e| { | ||
| vm.new_unicode_decode_error(format!("code_page_decode failed: {e}")) | ||
| })?; | ||
| return Ok((s, len)); | ||
| } | ||
|
|
||
| let mut buffer = vec![0u16; size as usize]; | ||
| let result = unsafe { | ||
| MultiByteToWideChar( | ||
| code_page, | ||
| strict_flags, | ||
| data.as_ptr().cast(), | ||
| len as i32, | ||
| buffer.as_mut_ptr(), | ||
| size, | ||
| ) | ||
| }; | ||
| if result == 0 { | ||
| let err = std::io::Error::last_os_error(); | ||
| return Err(vm.new_os_error(format!("code_page_decode failed: {err}"))); | ||
| } | ||
| buffer.truncate(result as usize); | ||
| let s = String::from_utf16(&buffer) | ||
| .map_err(|e| vm.new_unicode_decode_error(format!("code_page_decode failed: {e}")))?; | ||
|
|
||
| Ok((s, len)) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
errors="strict" isn’t enforced on decode failures.
When the strict MultiByteToWideChar pass returns 0, the code always falls back to replacement decoding, so invalid bytes won’t raise a UnicodeDecodeError. Gate the fallback on errors != "strict" (or raise a decode error) when MB_ERR_INVALID_CHARS is supported.
- let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict");
+ let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict");
let code_page = args.code_page as u32;
let data = args.data.borrow_buf();
let len = data.len();
@@
let size = unsafe {
MultiByteToWideChar(
code_page,
strict_flags,
@@
)
};
if size == 0 {
+ if errors == "strict" && strict_flags != 0 {
+ return Err(vm.new_unicode_decode_error(format!(
+ "'cp{code_page}' codec can't decode bytes: invalid character"
+ )));
+ }
let size = unsafe {
MultiByteToWideChar(
code_page,
0,CPython code_page_decode strict behavior MultiByteToWideChar MB_ERR_INVALID_CHARS UnicodeDecodeError
In `@crates/vm/src/stdlib/codecs.rs` around lines 770 - 865, The decode currently falls back to non-strict decoding whenever the first MultiByteToWideChar call returns 0; change code in the code_page_decode path to check args.errors (the _errors variable) when strict_flags uses MB_ERR_INVALID_CHARS: if size == 0 and _errors == "strict" then return vm.new_unicode_decode_error(...) (use the same error formatting as other branches) instead of falling back to call MultiByteToWideChar with flags 0; otherwise (when _errors != "strict") keep the existing fallback logic. Reference symbols: args.errors / _errors, strict_flags, MB_ERR_INVALID_CHARS, MultiByteToWideChar, vm.new_unicode_decode_error, and the code_page_decode return paths.
Sorry, something went wrong.
| /// _testFileTypeByName - test file type by path name | ||
| fn _test_file_type_by_name(path: &std::path::Path, tested_type: u32) -> bool { | ||
| use crate::common::fileutils::windows::{ | ||
| FILE_INFO_BY_NAME_CLASS, get_file_information_by_name, | ||
| }; | ||
| use crate::common::windows::ToWideString; | ||
| use windows_sys::Win32::Foundation::{CloseHandle, INVALID_HANDLE_VALUE}; | ||
| use windows_sys::Win32::Storage::FileSystem::{ | ||
| CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, | ||
| FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, | ||
| OPEN_EXISTING, | ||
| CreateFileW, FILE_ATTRIBUTE_REPARSE_POINT, FILE_FLAG_BACKUP_SEMANTICS, | ||
| FILE_FLAG_OPEN_REPARSE_POINT, FILE_READ_ATTRIBUTES, OPEN_EXISTING, | ||
| }; | ||
|
|
||
| // For islink/isjunction, use symlink_metadata to check reparse points | ||
| if (tested_type == PY_IFLNK || tested_type == PY_IFMNT) | ||
| && let Ok(meta) = path.symlink_metadata() | ||
| { | ||
| use std::os::windows::fs::MetadataExt; | ||
| let attrs = meta.file_attributes(); | ||
| use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; | ||
| if (attrs & FILE_ATTRIBUTE_REPARSE_POINT) == 0 { | ||
| return false; | ||
| use windows_sys::Win32::Storage::FileSystem::{FILE_DEVICE_CD_ROM, FILE_DEVICE_DISK}; | ||
| use windows_sys::Win32::System::Ioctl::FILE_DEVICE_VIRTUAL_DISK; | ||
|
|
||
| match get_file_information_by_name( | ||
| path.as_os_str(), | ||
| FILE_INFO_BY_NAME_CLASS::FileStatBasicByNameInfo, | ||
| ) { | ||
| Ok(info) => { | ||
| let disk_device = matches!( | ||
| info.DeviceType, | ||
| FILE_DEVICE_DISK | FILE_DEVICE_VIRTUAL_DISK | FILE_DEVICE_CD_ROM | ||
| ); | ||
| let result = _test_info( | ||
| info.FileAttributes, | ||
| info.ReparseTag, | ||
| disk_device, | ||
| tested_type, | ||
| ); | ||
| if !result | ||
| || (tested_type != PY_IFREG && tested_type != PY_IFDIR) | ||
| || (info.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0 | ||
| { | ||
| return result; | ||
| } | ||
| } | ||
| Err(err) => { | ||
| if let Some(code) = err.raw_os_error() | ||
| && file_info_error_is_trustworthy(code as u32) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| // Need to check reparse tag, fall through to CreateFileW | ||
| } | ||
|
|
||
| let wide_path = path.to_wide_with_nul(); | ||
|
|
||
| // For symlinks/junctions, add FILE_FLAG_OPEN_REPARSE_POINT to not follow | ||
| let mut flags = FILE_FLAG_BACKUP_SEMANTICS; | ||
| if tested_type != PY_IFREG && tested_type != PY_IFDIR { | ||
| flags |= FILE_FLAG_OPEN_REPARSE_POINT; | ||
| } | ||
|
|
||
| // Use sharing flags to avoid access denied errors | ||
| let handle = unsafe { | ||
| CreateFileW( | ||
| wide_path.as_ptr(), | ||
| FILE_READ_ATTRIBUTES, | ||
| FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, | ||
| 0, | ||
| core::ptr::null(), | ||
| OPEN_EXISTING, | ||
| flags, | ||
| std::ptr::null_mut(), | ||
| ) | ||
| }; | ||
|
|
||
| if handle == INVALID_HANDLE_VALUE { | ||
| // Fallback: try using Rust's metadata for isdir/isfile | ||
| if tested_type == PY_IFDIR { | ||
| return path.metadata().is_ok_and(|m| m.is_dir()); | ||
| } else if tested_type == PY_IFREG { | ||
| return path.metadata().is_ok_and(|m| m.is_file()); | ||
| } | ||
| // For symlinks/junctions, try without FILE_FLAG_BACKUP_SEMANTICS | ||
| let handle = unsafe { | ||
| CreateFileW( | ||
| wide_path.as_ptr(), | ||
| FILE_READ_ATTRIBUTES, | ||
| FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, | ||
| core::ptr::null(), | ||
| OPEN_EXISTING, | ||
| FILE_FLAG_OPEN_REPARSE_POINT, | ||
| std::ptr::null_mut(), | ||
| ) | ||
| }; | ||
| if handle == INVALID_HANDLE_VALUE { | ||
| return false; | ||
| } | ||
| let result = _test_file_type_by_handle(handle, tested_type, true); | ||
| if handle != INVALID_HANDLE_VALUE { | ||
| let result = _test_file_type_by_handle(handle, tested_type, false); | ||
| unsafe { CloseHandle(handle) }; | ||
| return result; | ||
| } | ||
|
|
||
| let result = _test_file_type_by_handle(handle, tested_type, true); | ||
| unsafe { CloseHandle(handle) }; | ||
| result | ||
| match unsafe { windows_sys::Win32::Foundation::GetLastError() } { | ||
| windows_sys::Win32::Foundation::ERROR_ACCESS_DENIED | ||
| | windows_sys::Win32::Foundation::ERROR_SHARING_VIOLATION | ||
| | windows_sys::Win32::Foundation::ERROR_CANT_ACCESS_FILE | ||
| | windows_sys::Win32::Foundation::ERROR_INVALID_PARAMETER => { | ||
| let stat = if tested_type == PY_IFREG || tested_type == PY_IFDIR { | ||
| crate::windows::win32_xstat(path.as_os_str(), true) | ||
| } else { | ||
| crate::windows::win32_xstat(path.as_os_str(), false) | ||
| }; | ||
| if let Ok(st) = stat { | ||
| let disk_device = (st.st_mode & libc::S_IFREG as u16) != 0; | ||
| return _test_info( | ||
| st.st_file_attributes, | ||
| st.st_reparse_tag, | ||
| disk_device, | ||
| tested_type, | ||
| ); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
||
| false | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
# First, let's examine the file structure and locate the relevant code
git ls-files "crates/vm/src/stdlib/nt.rs"Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Check if FILE_DEVICE_NETWORK_FILE_SYSTEM constant exists in the codebase
rg "FILE_DEVICE_NETWORK_FILE_SYSTEM" --type rustRepository: RustPython/RustPython
Length of output: 153
🏁 Script executed:
# Find the _test_info function to understand how disk_device is used
rg "_test_info" --type rust -A 10 -B 2Repository: RustPython/RustPython
Length of output: 4357
🏁 Script executed:
# Check the Cargo.toml to see which version of windows_sys is used
rg "windows_sys" Cargo.tomlRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Search for windows device type constants used in the codebase
rg "FILE_DEVICE_" --type rust | head -30Repository: RustPython/RustPython
Length of output: 1896
🏁 Script executed:
# Get the full _test_info function to understand how disk_device affects PY_IFREG
sed -n '520,565p' crates/vm/src/stdlib/nt.rsRepository: RustPython/RustPython
Length of output: 1844
🏁 Script executed:
# Get more context around _test_info to see the complete function
sed -n '490,595p' crates/vm/src/stdlib/nt.rsRepository: RustPython/RustPython
Length of output: 4160
🏁 Script executed:
# Check how crates/common/src/fileutils.rs handles device types
rg "FILE_DEVICE_NETWORK_FILE_SYSTEM" -B 5 -A 5 crates/common/src/fileutils.rsRepository: RustPython/RustPython
Length of output: 560
🏁 Script executed:
# Get the complete _test_info function to see PY_IFREG logic
sed -n '450,490p' crates/vm/src/stdlib/nt.rsRepository: RustPython/RustPython
Length of output: 1943
🏁 Script executed:
# Check windows_sys for the FILE_DEVICE_NETWORK_FILE_SYSTEM constant availability
rg "FILE_DEVICE_NETWORK_FILE_SYSTEM" -t rustRepository: RustPython/RustPython
Length of output: 153
🏁 Script executed:
# Verify the constant path in windows_sys
python3 << 'EOF'
# Let's check if the constant is available and in what module
# From the fileutils.rs output, it's: Ioctl::FILE_DEVICE_NETWORK_FILE_SYSTEM
# We need to verify if it's in Win32::System::Ioctl
import subprocess
result = subprocess.run(['rg', 'Ioctl::FILE_DEVICE_NETWORK_FILE_SYSTEM', '-A', '2', '-B', '2', 'crates/common/src/fileutils.rs'],
capture_output=True, text=True)
print("Context around FILE_DEVICE_NETWORK_FILE_SYSTEM in fileutils.rs:")
print(result.stdout)
EOFRepository: RustPython/RustPython
Length of output: 369
🏁 Script executed:
# Verify the exact module path for FILE_DEVICE_NETWORK_FILE_SYSTEM in windows_sys
rg "use.*Ioctl" crates/common/src/fileutils.rs -B 5 | head -20Repository: RustPython/RustPython
Length of output: 268
🏁 Script executed:
# Verify the pattern used in fileutils.rs matches what was proposed in the diff
sed -n '1,50p' crates/common/src/fileutils.rs | grep -A 20 "use.*Ioctl"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Check the imports in fileutils.rs to confirm the module path
sed -n '1,100p' crates/common/src/fileutils.rs | grep -E "use windows_sys"Repository: RustPython/RustPython
Length of output: 351
🏁 Script executed:
# Double-check the exact location where FILE_DEVICE_NETWORK_FILE_SYSTEM is used
rg "FILE_DEVICE_NETWORK_FILE_SYSTEM" -B 10 -A 2 crates/common/src/fileutils.rs | head -30Repository: RustPython/RustPython
Length of output: 651
Include FILE_DEVICE_NETWORK_FILE_SYSTEM in disk_device type checks for network file support.
Regular files on network shares (UNC paths, etc.) report DeviceType as FILE_DEVICE_NETWORK_FILE_SYSTEM, but the current disk_device check only includes FILE_DEVICE_DISK, FILE_DEVICE_VIRTUAL_DISK, and FILE_DEVICE_CD_ROM. This causes _test_info() to return false for PY_IFREG on network paths, triggering an early return without reaching the handle-based fallback. Add FILE_DEVICE_NETWORK_FILE_SYSTEM to the matches pattern (import from windows_sys::Win32::System::Ioctl), or consider relaxing the device type gating to ensure regular files on all mountable filesystem types are handled correctly.
🤖 Prompt for AI AgentsIn `@crates/vm/src/stdlib/nt.rs` around lines 573 - 666, The disk_device check in _test_file_type_by_name incorrectly omits network filesystems so regular files on UNC/share paths aren't treated as disk devices; update the matches!(...) in _test_file_type_by_name to include FILE_DEVICE_NETWORK_FILE_SYSTEM (import it from windows_sys::Win32::System::Ioctl) alongside FILE_DEVICE_DISK, FILE_DEVICE_VIRTUAL_DISK, and FILE_DEVICE_CD_ROM so _test_info(...) correctly treats network-mounted regular files as disk devices and allows the rest of the logic (including the handle-based fallback) to run.
Sorry, something went wrong.
| fn _getfinalpathname(path: OsPath, vm: &VirtualMachine) -> PyResult { | ||
| let real = path | ||
| .as_ref() | ||
| .canonicalize() | ||
| .map_err(|e| e.to_pyexception(vm))?; | ||
| Ok(path.mode().process_path(real, vm)) | ||
| use windows_sys::Win32::Storage::FileSystem::{ | ||
| CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, GetFinalPathNameByHandleW, OPEN_EXISTING, | ||
| VOLUME_NAME_DOS, | ||
| }; | ||
|
|
||
| let wide = path.to_wide_cstring(vm)?; | ||
| let handle = unsafe { | ||
| CreateFileW( | ||
| wide.as_ptr(), | ||
| 0, | ||
| 0, | ||
| core::ptr::null(), | ||
| OPEN_EXISTING, | ||
| FILE_FLAG_BACKUP_SEMANTICS, | ||
| std::ptr::null_mut(), | ||
| ) | ||
| }; | ||
| if handle == INVALID_HANDLE_VALUE { | ||
| let err = io::Error::last_os_error(); | ||
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | ||
| } | ||
|
|
||
| let mut buffer: Vec<u16> = vec![0; Foundation::MAX_PATH as usize]; | ||
| let result = loop { | ||
| let ret = unsafe { | ||
| GetFinalPathNameByHandleW( | ||
| handle, | ||
| buffer.as_mut_ptr(), | ||
| buffer.len() as u32, | ||
| VOLUME_NAME_DOS, | ||
| ) | ||
| }; | ||
| if ret == 0 { | ||
| let err = io::Error::last_os_error(); | ||
| let _ = unsafe { Foundation::CloseHandle(handle) }; | ||
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | ||
| } | ||
| if (ret as usize) < buffer.len() { | ||
| let final_path = std::ffi::OsString::from_wide(&buffer[..ret as usize]); | ||
| break Ok(path.mode().process_path(final_path, vm)); | ||
| } | ||
| buffer.resize(ret as usize, 0); | ||
| }; | ||
|
|
||
| unsafe { Foundation::CloseHandle(handle) }; | ||
| result |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
# First, locate and examine the file
fd nt.rs crates/vm/src/stdlib/Repository: RustPython/RustPython
Length of output: 203
🏁 Script executed:
# Read the specific lines in question
sed -n '1280,1340p' crates/vm/src/stdlib/nt.rsRepository: RustPython/RustPython
Length of output: 2137
🏁 Script executed:
# Check the imports at the top of the file to understand the current setup
head -100 crates/vm/src/stdlib/nt.rs | grep -A 20 "use windows_sys"Repository: RustPython/RustPython
Length of output: 2278
🏁 Script executed:
# Check if the proposed constants exist in windows_sys by searching Rust files
rg "FILE_READ_ATTRIBUTES|FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE" --type rs | head -20Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
# Check the windows_sys crate version and available exports
fd "Cargo.toml" | xargs grep -l "windows_sys" | head -5Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look at how other functions in nt.rs handle file access
rg "CreateFileW" crates/vm/src/stdlib/nt.rs -A 10 -B 2Repository: RustPython/RustPython
Length of output: 6696
🌐 Web query:
GetFinalPathNameByHandleW access rights requirements FILE_READ_ATTRIBUTES Windows API best practices
💡 Result:
Minimal access rights (practical + “least privilege”)
For GetFinalPathNameByHandleW, the safest minimal access to request when opening the handle is:
- dwDesiredAccess = FILE_READ_ATTRIBUTES (rather than GENERIC_READ) so you can read name/metadata without requesting read-data access. FILE_READ_ATTRIBUTES is the specific right used to read file metadata. [3][4]
Many callers also succeed with
- dwDesiredAccess = 0 (query-only handle) when they only need metadata and want to avoid requesting GENERIC_READ; Microsoft documents that a zero access mask can still allow querying certain metadata/attributes. [2]
However, this is not guaranteed to work for every file system / redirector / scenario, so FILE_READ_ATTRIBUTES is the more reliable “minimal” choice.
Share mode / flags (best practices)
When opening the handle you’ll pass to GetFinalPathNameByHandleW:
- Prefer permissive sharing to avoid collisions with other opens:
dwShareMode = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE - If opening a directory, include:
FILE_FLAG_BACKUP_SEMANTICS (required to open directory handles via CreateFile). - If you only need the path and not I/O, avoid unnecessary rights (don’t use GENERIC_READ unless you actually need to read file data).
Avoiding ERROR_ACCESS_DENIED surprises (notably SMB)
If you request a normalized name (FILE_NAME_NORMALIZED, the default), Microsoft notes that on SMB shares the system may resolve and query each path component; if the caller lacks permission on any component, the call can fail with ERROR_ACCESS_DENIED. [1]
Mitigations:
- Use FILE_NAME_OPENED when you don’t strictly need normalization. [1]
- Or ensure the security context has traverse/query permission for the full component chain on the share.
Recommended pattern (typical)
Open a handle “as lightly as possible” for path resolution:
- CreateFileW(path, FILE_READ_ATTRIBUTES, FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, …, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS /*if dir*/, NULL)
- Then call GetFinalPathNameByHandleW(handle, …, dwFlags) (choose FILE_NAME_OPENED vs normalized per your needs). [1]
Sources: Microsoft GetFinalPathNameByHandleW remarks/flags and SMB behavior [1]; CreateFile note about dwDesiredAccess = 0 for querying metadata/attributes [2]; definition of FILE_READ_ATTRIBUTES right [3][4].
Use permissive share mode to avoid sharing violations when opening handle for final path.
The handle is opened with share mode 0, which prevents other processes from opening the file. This causes ERROR_SHARING_VIOLATION failures even though only metadata is needed. Use FILE_READ_ATTRIBUTES for access rights and FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE for share mode, matching Windows best practices and existing patterns in the codebase.
🛠️ Suggested change to reduce sharing violations use windows_sys::Win32::Storage::FileSystem::{
- CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, GetFinalPathNameByHandleW, OPEN_EXISTING,
- VOLUME_NAME_DOS,
+ CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE,
+ FILE_SHARE_READ, FILE_SHARE_WRITE, GetFinalPathNameByHandleW, OPEN_EXISTING,
+ VOLUME_NAME_DOS,
};
let wide = path.to_wide_cstring(vm)?;
let handle = unsafe {
CreateFileW(
wide.as_ptr(),
- 0,
- 0,
+ FILE_READ_ATTRIBUTES,
+ FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
core::ptr::null(),
OPEN_EXISTING,
FILE_FLAG_BACKUP_SEMANTICS,
std::ptr::null_mut(),
)
};‼️ 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.
| fn _getfinalpathname(path: OsPath, vm: &VirtualMachine) -> PyResult { | |
| let real = path | |
| .as_ref() | |
| .canonicalize() | |
| .map_err(|e| e.to_pyexception(vm))?; | |
| Ok(path.mode().process_path(real, vm)) | |
| use windows_sys::Win32::Storage::FileSystem::{ | |
| CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, GetFinalPathNameByHandleW, OPEN_EXISTING, | |
| VOLUME_NAME_DOS, | |
| }; | |
| let wide = path.to_wide_cstring(vm)?; | |
| let handle = unsafe { | |
| CreateFileW( | |
| wide.as_ptr(), | |
| 0, | |
| 0, | |
| core::ptr::null(), | |
| OPEN_EXISTING, | |
| FILE_FLAG_BACKUP_SEMANTICS, | |
| std::ptr::null_mut(), | |
| ) | |
| }; | |
| if handle == INVALID_HANDLE_VALUE { | |
| let err = io::Error::last_os_error(); | |
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | |
| } | |
| let mut buffer: Vec<u16> = vec![0; Foundation::MAX_PATH as usize]; | |
| let result = loop { | |
| let ret = unsafe { | |
| GetFinalPathNameByHandleW( | |
| handle, | |
| buffer.as_mut_ptr(), | |
| buffer.len() as u32, | |
| VOLUME_NAME_DOS, | |
| ) | |
| }; | |
| if ret == 0 { | |
| let err = io::Error::last_os_error(); | |
| let _ = unsafe { Foundation::CloseHandle(handle) }; | |
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | |
| } | |
| if (ret as usize) < buffer.len() { | |
| let final_path = std::ffi::OsString::from_wide(&buffer[..ret as usize]); | |
| break Ok(path.mode().process_path(final_path, vm)); | |
| } | |
| buffer.resize(ret as usize, 0); | |
| }; | |
| unsafe { Foundation::CloseHandle(handle) }; | |
| result | |
| fn _getfinalpathname(path: OsPath, vm: &VirtualMachine) -> PyResult { | |
| use windows_sys::Win32::Storage::FileSystem::{ | |
| CreateFileW, FILE_FLAG_BACKUP_SEMANTICS, FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, | |
| FILE_SHARE_READ, FILE_SHARE_WRITE, GetFinalPathNameByHandleW, OPEN_EXISTING, | |
| VOLUME_NAME_DOS, | |
| }; | |
| let wide = path.to_wide_cstring(vm)?; | |
| let handle = unsafe { | |
| CreateFileW( | |
| wide.as_ptr(), | |
| FILE_READ_ATTRIBUTES, | |
| FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, | |
| core::ptr::null(), | |
| OPEN_EXISTING, | |
| FILE_FLAG_BACKUP_SEMANTICS, | |
| std::ptr::null_mut(), | |
| ) | |
| }; | |
| if handle == INVALID_HANDLE_VALUE { | |
| let err = io::Error::last_os_error(); | |
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | |
| } | |
| let mut buffer: Vec<u16> = vec![0; Foundation::MAX_PATH as usize]; | |
| let result = loop { | |
| let ret = unsafe { | |
| GetFinalPathNameByHandleW( | |
| handle, | |
| buffer.as_mut_ptr(), | |
| buffer.len() as u32, | |
| VOLUME_NAME_DOS, | |
| ) | |
| }; | |
| if ret == 0 { | |
| let err = io::Error::last_os_error(); | |
| let _ = unsafe { Foundation::CloseHandle(handle) }; | |
| return Err(OSErrorBuilder::with_filename(&err, path, vm)); | |
| } | |
| if (ret as usize) < buffer.len() { | |
| let final_path = std::ffi::OsString::from_wide(&buffer[..ret as usize]); | |
| break Ok(path.mode().process_path(final_path, vm)); | |
| } | |
| buffer.resize(ret as usize, 0); | |
| }; | |
| unsafe { Foundation::CloseHandle(handle) }; | |
| result |
In `@crates/vm/src/stdlib/nt.rs` around lines 1287 - 1333, In _getfinalpathname, the CreateFileW call opens the file with access 0 and share mode 0 which causes sharing violations; change the access parameter to FILE_READ_ATTRIBUTES and the sharemode parameter to FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE (matching other stdlib patterns) when calling CreateFileW so the handle is opened permissively for metadata reads; update the CreateFileW invocation in the _getfinalpathname function accordingly and keep the rest of the logic (GetFinalPathNameByHandleW/CloseHandle) unchanged.
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin nt |
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 (2)crates/vm/src/stdlib/codecs.rs (2)465-521: ⚠️ Potential issue | 🟠 Major
Honor errors="strict" in mbcs_decode before falling back.
The _errors variable is parsed but never used. Currently, when the strict MultiByteToWideChar pass with MB_ERR_INVALID_CHARS fails, the code unconditionally retries without the flag, allowing invalid bytes to pass through even in strict mode. In strict mode, this should raise an error instead.
🛠️ Proposed fix- let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); + let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); let data = args.data.borrow_buf(); let len = data.len(); @@ -488,6 +488,11 @@ mod windows_codecs { if size == 0 { + if errors == "strict" { + return Err(vm.new_unicode_decode_error( + "'mbcs' codec can't decode bytes: invalid character".to_owned(), + )); + } // Try without MB_ERR_INVALID_CHARS for non-strict mode (replacement behavior)
667-723: ⚠️ Potential issue | 🟠 Major
Honor errors="strict" in oem_decode and other decode functions.
At line 667, _errors is parsed but never used. When the strict MultiByteToWideChar pass fails (size == 0), the code unconditionally retries without MB_ERR_INVALID_CHARS, bypassing strict error checking. This violates codec semantics: errors="strict" should raise an error on invalid bytes, not silently fall back to lenient decoding.
The same issue exists in mbcs_decode (line 465) and code_page_decode (line 895). By contrast, the corresponding encode functions (oem_encode, code_page_encode) correctly use the errors parameter to control behavior in strict mode.
🛠️ Proposed fix for oem_decode- let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); + let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); let data = args.data.borrow_buf(); let len = data.len(); if data.is_empty() { return Ok((String::new(), 0)); } // Get the required buffer size for UTF-16 let size = unsafe { MultiByteToWideChar( CP_OEMCP, MB_ERR_INVALID_CHARS, data.as_ptr().cast(), len as i32, std::ptr::null_mut(), 0, ) }; if size == 0 { + if errors == "strict" { + return Err(vm.new_unicode_decode_error( + "'oem' codec can't decode bytes: invalid character".to_owned(), + )); + } // Try without MB_ERR_INVALID_CHARS for non-strict mode (replacement behavior)Apply the same logic to mbcs_decode and code_page_decode.
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/io.py dependencies:
dependent tests: (87 tests)
[ ] test: cpython/Lib/test/test_cmd_line_script.py (TODO: 16) dependencies: dependent tests: (no tests depend on cmd_line_script) [x] lib: cpython/Lib/codecs.py dependencies:
dependent tests: (105 tests)
[ ] lib: cpython/Lib/compileall.py dependencies:
dependent tests: (1 tests)
[ ] lib: cpython/Lib/concurrent dependencies:
dependent tests: (11 tests)
[x] test: cpython/Lib/test/test_exceptions.py (TODO: 27) dependencies: dependent tests: (no tests depend on exception) [x] lib: cpython/Lib/os.py dependencies:
dependent tests: (160 tests)
[x] test: cpython/Lib/test/test_posix.py (TODO: 4) dependencies: dependent tests: (no tests depend on posix) [x] lib: cpython/Lib/runpy.py dependencies:
dependent tests: (3 tests)
[ ] lib: cpython/Lib/test/support dependencies:
dependent tests: (no tests depend on support) [ ] lib: cpython/Lib/subprocess.py dependencies:
dependent tests: (52 tests)
[x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (148 tests)
[ ] lib: cpython/Lib/zipfile dependencies:
dependent tests: (8 tests)
Legend:
|
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 (2)crates/vm/src/stdlib/codecs.rs (2)447-502: ⚠️ Potential issue | 🟠 Major
errors="strict" isn't enforced on decode failures in mbcs_decode.
The _errors variable (line 447) is extracted but never used. When MultiByteToWideChar with MB_ERR_INVALID_CHARS returns 0, the code unconditionally falls back to replacement decoding (lines 468-502). This means invalid bytes won't raise a UnicodeDecodeError even when errors="strict".
🛠️ Proposed fix- let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); + let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); let data = args.data.borrow_buf(); let len = data.len(); @@ if size == 0 { + if errors == "strict" { + return Err(vm.new_unicode_decode_error( + "'mbcs' codec can't decode bytes: invalid character".to_string(), + )); + } // Try without MB_ERR_INVALID_CHARS for non-strict mode (replacement behavior) let size = unsafe {
637-692: ⚠️ Potential issue | 🟠 Major
errors="strict" isn't enforced on decode failures in oem_decode.
Same issue as mbcs_decode: the _errors variable (line 637) is unused, and the code unconditionally falls back to replacement decoding when the strict check fails. This violates Python's codec error handling semantics.
🛠️ Proposed fix- let _errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); + let errors = args.errors.as_ref().map(|s| s.as_str()).unwrap_or("strict"); let data = args.data.borrow_buf(); let len = data.len(); @@ if size == 0 { + if errors == "strict" { + return Err(vm.new_unicode_decode_error( + "'oem' codec can't decode bytes: invalid character".to_string(), + )); + } // Try without MB_ERR_INVALID_CHARS for non-strict mode (replacement behavior) let size = unsafe {
crates/vm/src/stdlib/codecs.rs (1)338-348: Redundant #[cfg(windows)] attributes inside already-gated module.
The structs and functions inside _codecs_windows (lines 338, 347, 428, 440, 528, 537, 618, 630, 718, 729, 828, 842) have #[cfg(windows)] attributes, but the enclosing module at line 332 is already gated by #[cfg(windows)]. These inner attributes are redundant and can be removed for cleaner code.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes