Replace windows to windows-sys - #6356
Conversation
Sorry, something went wrong.
Bumps [windows](https://github.com/microsoft/windows-rs) from 0.52.0 to 0.62.2. - [Release notes](https://github.com/microsoft/windows-rs/releases) - [Commits](https://github.com/microsoft/windows-rs/commits) --- updated-dependencies: - dependency-name: windows dependency-version: 0.62.2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
WalkthroughMigrates Windows API usage from the windows crate to windows-sys, introduces a new public WinHandle wrapper, and updates Windows-specific modules (timezone, path, process/file/handle wrappers) and Cargo features to use windows-sys and the new handle type. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
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.
|
✅ Actions performed
Review triggered.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/stdlib/winapi.rs (1)509-521: OpenMutexW has incorrect parameter type for name.
The name parameter is u16, but it should be a wide string (null-terminated *const u16). A single u16 value is being cast to PCWSTR, which will cause undefined behavior when the function tries to read a string from that address.
The signature should likely be:
#[pyfunction] -fn OpenMutexW(desired_access: u32, inherit_handle: bool, name: u16) -> PyResult<isize> { +fn OpenMutexW(desired_access: u32, inherit_handle: bool, name: PyStrRef, vm: &VirtualMachine) -> PyResult<isize> { + let name = name.as_str().to_wide_with_nul(); let handle = unsafe { windows_sys::Win32::System::Threading::OpenMutexW( desired_access, i32::from(inherit_handle), - windows_sys::core::PCWSTR::from(name as _), + name.as_ptr(), ) }; - // if handle.is_invalid() { - // return Err(errno_err(vm)); - // } + if handle.is_null() { + return Err(errno_err(vm)); + } Ok(handle as _) }
crates/vm/src/stdlib/winapi.rs (2)📜 Review details293-302: Inconsistent return type: OpenProcess returns isize instead of WinHandle.
Other process handle functions like GetCurrentProcess, CreateProcess, and TerminateProcess use WinHandle, but OpenProcess returns raw isize. Consider updating for consistency.
#[pyfunction] -fn OpenProcess(desired_access: u32, inherit_handle: bool, process_id: u32) -> isize { - unsafe { +fn OpenProcess(desired_access: u32, inherit_handle: bool, process_id: u32, vm: &VirtualMachine) -> PyResult<WinHandle> { + let handle = unsafe { windows_sys::Win32::System::Threading::OpenProcess( desired_access, i32::from(inherit_handle), process_id, - ) as _ + ) + }; + if handle.is_null() { + return Err(errno_err(vm)); } + Ok(WinHandle(handle)) }
432-441: Size calculation uses size_of::<isize>() but handlelist is Vec<usize>.
On line 386, handlelist is extracted as ArgSequence<usize>, making it a Vec<usize>. The size calculation should use size_of::<usize>() for consistency. While both are the same size on all supported platforms (32-bit and 64-bit), using the matching type is clearer.
- (handlelist.len() * std::mem::size_of::<isize>()) as _, + (handlelist.len() * std::mem::size_of::<usize>()) as _,
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between a3d638a and 338170a.
⛔ Files ignored due to path filters (1)- Cargo.lock is excluded by !**/*.lock
- crates/vm/Cargo.toml (1 hunks)
- crates/vm/src/stdlib/nt.rs (1 hunks)
- crates/vm/src/stdlib/time.rs (2 hunks)
- crates/vm/src/stdlib/winapi.rs (11 hunks)
- crates/vm/src/windows.rs (2 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/vm/src/stdlib/time.rs
- crates/vm/src/windows.rs
- crates/vm/src/stdlib/nt.rs
- crates/vm/src/stdlib/winapi.rs
crates/vm/Cargo.toml (1)crates/vm/src/stdlib/nt.rs (1)115-140: LGTM!
The addition of Win32_System_Time to the windows-sys features is correctly aligned with the changes in time.rs that now use GetTimeZoneInformation from windows_sys.
crates/vm/src/windows.rs (2)559-579: LGTM! The PathCchSkipRoot migration is correctly implemented.
The implementation properly:
- Uses an output pointer for the end parameter
- Checks hr == 0 for S_OK (correct HRESULT check)
- Validates the pointer is non-null before use
- Uses safe pointer arithmetic with offset_from and validates the result
One minor observation: the debug assert message on lines 571-574 prints backslashed (which has / replaced with \), not the original path. This is fine for debugging but could be slightly confusing.
crates/vm/src/stdlib/winapi.rs (8)13-15: LGTM! The WinHandle wrapper provides clean Python interop.
The wrapper correctly encapsulates the raw HANDLE type for safer usage across the codebase.
64-77: The HandleInt type discrepancy is acknowledged.
The existing TODO on line 64 notes that HandleInt should change to isize when fully ported to windows-rs. The current usize usage works in practice since Windows handles typically fit in the positive range, but this should be addressed in a follow-up to avoid potential issues with special handle values like INVALID_HANDLE_VALUE (-1).
77-79: LGTM! CloseHandle correctly uses WinHandle.
82-96: LGTM! GetStdHandle properly handles the NULL and INVALID_HANDLE_VALUE cases.
The function correctly returns None for null handles and errors for invalid handles.
98-118: LGTM! CreatePipe migration is correct.
The function properly initializes the handles using MaybeUninit and wraps them in WinHandle.
120-146: LGTM! DuplicateHandle correctly uses WinHandle for all handle parameters.
154-156: LGTM! GetCurrentProcess returns WinHandle.
158-169: LGTM! GetFileType accepts WinHandle.
203-291: LGTM! CreateProcess correctly returns WinHandle for process and thread handles.
447-475: LGTM! WaitForSingleObject, GetExitCodeProcess, and TerminateProcess correctly use WinHandle.
Sorry, something went wrong.
| fn get_tz_info() -> TIME_ZONE_INFORMATION { | ||
| let mut info: TIME_ZONE_INFORMATION = unsafe { std::mem::zeroed() }; | ||
| unsafe { GetTimeZoneInformation(&mut info) }; | ||
| info | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Missing error check for GetTimeZoneInformation.
The return value of GetTimeZoneInformation is discarded. This function returns TIME_ZONE_ID_INVALID (0xFFFFFFFF) on failure, which should be checked to avoid returning zeroed/invalid timezone data.
Consider adding error handling:
fn get_tz_info() -> TIME_ZONE_INFORMATION {
let mut info: TIME_ZONE_INFORMATION = unsafe { std::mem::zeroed() };
- unsafe { GetTimeZoneInformation(&mut info) };
+ let result = unsafe { GetTimeZoneInformation(&mut info) };
+ debug_assert!(result != 0xFFFFFFFF, "GetTimeZoneInformation failed");
info
}Alternatively, if the callers can handle errors, consider returning a Result type.
📝 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.
| fn get_tz_info() -> TIME_ZONE_INFORMATION { | |
| let mut info: TIME_ZONE_INFORMATION = unsafe { std::mem::zeroed() }; | |
| unsafe { GetTimeZoneInformation(&mut info) }; | |
| info | |
| } | |
| fn get_tz_info() -> TIME_ZONE_INFORMATION { | |
| let mut info: TIME_ZONE_INFORMATION = unsafe { std::mem::zeroed() }; | |
| let result = unsafe { GetTimeZoneInformation(&mut info) }; | |
| debug_assert!(result != 0xFFFFFFFF, "GetTimeZoneInformation failed"); | |
| info | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 338170a and 52aa978.
📒 Files selected for processing (1)- crates/vm/src/stdlib/winapi.rs (12 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/vm/src/stdlib/winapi.rs
crates/vm/src/stdlib/os.rs (1)⏰ 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)
- errno_err (39-41)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
crates/vm/src/stdlib/winapi.rs (15)15-21: WinHandle/WindowsSysResult and windows_sys imports look consistent
Importing WinHandle, WindowsSysResult, INVALID_HANDLE_VALUE, and MAX_PATH from the new windows_sys-based stack is appropriate and matches how the rest of the file uses these types/constants. No issues here.
76-79: CloseHandle wrapper correctly migrated to WinHandle/WindowsSysResult
Wrapping CloseHandle as fn CloseHandle(handle: WinHandle) -> WindowsSysResult<i32> and passing handle.0 matches the BOOL-returning Win32 API and keeps error handling centralized in WindowsSysResult.
82-96: GetStdHandle semantics preserved with WinHandle
The new GetStdHandle returning PyResult<Option<WinHandle>> correctly:
- Treats INVALID_HANDLE_VALUE as an error via errno_err(vm).
- Treats a NULL handle as None.
- Wraps valid handles in WinHandle.
This preserves the old sentinel behavior while aligning with the new handle abstraction.
98-118: CreatePipe wrapper correctly handles HANDLE out-params
CreatePipe now:
- Uses MaybeUninit<HANDLE> for read/write ends.
- Wraps the resulting handles as WinHandle in the returned tuple.
- Relies on WindowsSysResult(..).to_pyresult(vm)? for error propagation.
This is idiomatic and matches the windows_sys FFI conventions.
120-146: DuplicateHandle migration to WinHandle is sound
The updated DuplicateHandle:
- Accepts WinHandle for src_process, src, and target_process.
- Uses a MaybeUninit<HANDLE> out-parameter.
- Wraps the duplicated handle in WinHandle on success.
The argument order and conversion of inherit/options look correct for the underlying API.
154-156: GetCurrentProcess returning WinHandle is consistent
Wrapping GetCurrentProcess() in WinHandle aligns it with other APIs that now traffic in WinHandle. This matches its intended use as a pseudo-handle passed into other FFI calls.
158-169: GetFileType correctly updated to accept WinHandle
Taking h: WinHandle and calling GetFileType(h.0) preserves behavior while tightening the handle type. The existing error check (file_type == 0 && GetLastError() != 0) is preserved and remains appropriate.
203-291: CreateProcess now returning WinHandle for process/thread handles
The updated CreateProcess:
- Continues to set up STARTUPINFOEXW, environment, and attribute list as before.
- Returns WinHandle(procinfo.hProcess) and WinHandle(procinfo.hThread) instead of raw handles.
This keeps FFI boundary concerns localized and is consistent with the new handle abstraction. No functional regressions are apparent in the startup-info or env/attribute handling in this diff.
294-311: OpenProcess error handling improved and aligned with WinHandle
The new OpenProcess wrapper:
- Takes inherit_handle: bool and converts it via i32::from(inherit_handle) to the expected BOOL.
- Checks handle.is_null() and maps failure to errno_err(vm).
- Returns a PyResult<WinHandle> on success.
This is a clear improvement over returning a raw integer and manually checking for sentinel values at call sites.
You may want to run cargo test -p vm -- --nocapture (or your existing Windows test suite) on a Windows host to confirm that all Python-level callers of _winapi.OpenProcess correctly handle the new PyResult<WinHandle> return type and adjusted signature.
445-445: AttrList handle list size calculation matches usize-based handles
Using (handlelist.len() * size_of::<usize>()) as _ aligns the size passed to UpdateProcThreadAttribute with the type of handlelist: Vec<usize>. This keeps the allocation and size math consistent with how handles are represented here.
457-464: WaitForSingleObject correctly updated to take WinHandle
WaitForSingleObject(h.0, ms) with an error check on WAIT_FAILED mirrors the Win32 API behavior, now using the typed WinHandle wrapper. No issues with the conversion or error propagation via errno_err(vm).
467-477: GetExitCodeProcess wrapper correctly uses WinHandle and WindowsSysResult
The function:
- Accepts h: WinHandle.
- Uses MaybeUninit for the exit code out-parameter.
- Wraps the call in WindowsSysResult(..).to_pyresult(vm)?.
This matches the underlying API’s contract and is consistent with the project’s WindowsSysResult error-handling pattern.
480-484: TerminateProcess wrapper migration is straightforward
Passing h.0 into TerminateProcess and wrapping the BOOL result in WindowsSysResult<i32> is correct and consistent with how other BOOL-based APIs are handled.
488-496: LoadLibrary uses windows_sys correctly and checks for null handle
LoadLibrary now:
- Converts the path via to_wide_with_nul.
- Calls LoadLibraryW from windows_sys.
- Checks handle.is_null() and raises a runtime error on failure.
- Returns handle as isize on success.
This matches the documented contract (NULL on failure) and keeps FFI details localized.
If this function is used by higher-level ctypes-like loaders, please ensure those callers still expect an isize and handle failures via exceptions rather than sentinel values.
499-515: GetModuleFileName correctly casts handle and bounds the UTF-16 slice
Casting handle to HMODULE and passing it to GetModuleFileNameW, then slicing path[..length] before UTF-16 decoding, is the standard pattern here. The MAX_PATH-based buffer and the length == 0 failure check are unchanged in behavior.
Sorry, something went wrong.
| fn OpenMutexW( | ||
| desired_access: u32, | ||
| inherit_handle: bool, | ||
| name: PyStrRef, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<isize> { | ||
| let name_wide = name.as_str().to_wide_with_nul(); | ||
| let handle = unsafe { | ||
| windows_sys::Win32::System::Threading::OpenMutexW( | ||
| desired_access, | ||
| i32::from(inherit_handle), | ||
| windows_sys::core::PCWSTR::from(name as _), | ||
| name_wide.as_ptr(), | ||
| ) | ||
| }; | ||
| // if handle.is_invalid() { | ||
| // return Err(errno_err(vm)); | ||
| // } | ||
| if handle == INVALID_HANDLE_VALUE { | ||
| return Err(errno_err(vm)); | ||
| } | ||
| Ok(handle as _) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain🌐 Web query:
Win32 OpenMutexW API return value failure NULL INVALID_HANDLE_VALUE
💡 Result:
Short answer: OpenMutexW returns NULL on failure — it does NOT return INVALID_HANDLE_VALUE. On success it returns a valid HANDLE; on failure check GetLastError() for the reason (e.g., ERROR_FILE_NOT_FOUND if the named mutex doesn't exist, access denied if security blocks the requested access). [1]
Common causes when OpenMutexW returns NULL
- The named mutex doesn't exist. GetLastError() == ERROR_FILE_NOT_FOUND (2). [1]
- You asked for access that the mutex's security descriptor disallows. [1]
- Name mismatch (case sensitive) or wrong namespace (use "Global\name" vs "Local\name" appropriately across sessions). [1]
- Another object with the same name but different type exists (CreateMutex/OpenMutex can fail with ERROR_INVALID_HANDLE when a non-mutex object already uses that name). [3]
- Unicode/ANSI mismatch or incorrect lpName string (OpenMutexW expects LPCWSTR). [3]
- Race conditions between create/open across processes. [1][3]
What to do
- Check the return against NULL, not INVALID_HANDLE_VALUE. Then call GetLastError() to diagnose. [1][2]
- Ensure you pass a wide string to OpenMutexW (L"Name" or use the generic OpenMutex with proper TCHAR usage).
- Verify namespace and spelling; try tools like WinObj to inspect kernel object names if needed.
- If you need create-or-open semantics, use CreateMutex (it opens if exists, creates if not) and check GetLastError() for ERROR_ALREADY_EXISTS. [1][3]
References
- Microsoft Docs: OpenMutexW (return value and errors). [1]
- Discussion: checking handles vs INVALID_HANDLE_VALUE (use 0/NULL for many objects). [2]
- Community notes on CreateMutex/OpenMutex failures (name collisions, Unicode issues). [3]
OpenMutexW failure check uses the wrong sentinel, causing silent errors
The current implementation checks:
if handle == INVALID_HANDLE_VALUE {
return Err(errno_err(vm));
}However, OpenMutexW returns NULL on failure, not INVALID_HANDLE_VALUE. With this code:
- A failing OpenMutexW call will return NULL, which passes the check
- Such failures will slip through as Ok(0isize), and the error will only surface later when using the bogus handle
This is a correctness bug and should be fixed to treat NULL as failure, similar to OpenProcess:
let handle = unsafe {
windows_sys::Win32::System::Threading::OpenMutexW(
desired_access,
i32::from(inherit_handle),
name_wide.as_ptr(),
)
};
- if handle == INVALID_HANDLE_VALUE {
- return Err(errno_err(vm));
- }
- Ok(handle as _)
+ if handle.is_null() {
+ return Err(errno_err(vm));
+ }
+ Ok(handle as isize)In crates/vm/src/stdlib/winapi.rs around lines 518 to 536, the OpenMutexW failure check is using INVALID_HANDLE_VALUE but OpenMutexW returns NULL (0) on failure; change the check to test for a zero handle (if handle == 0) and return Err(errno_err(vm)) in that case, keeping the existing cast/Ok(handle as _) for the success path.
Sorry, something went wrong.
close #6189
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.