Replace windows to windows-sys #6356
Conversation
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)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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:OpenMutexWhas incorrect parameter type forname.The
nameparameter isu16, but it should be a wide string (null-terminated*const u16). A singleu16value is being cast toPCWSTR, 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 _) }
🧹 Nitpick comments (2)
crates/vm/src/stdlib/winapi.rs (2)
293-302: Inconsistent return type: OpenProcess returnsisizeinstead ofWinHandle.Other process handle functions like
GetCurrentProcess,CreateProcess, andTerminateProcessuseWinHandle, butOpenProcessreturns rawisize. 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 usessize_of::<isize>()buthandlelistisVec<usize>.On line 386,
handlelistis extracted asArgSequence<usize>, making it aVec<usize>. The size calculation should usesize_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 _,
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
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)
🧰 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/time.rscrates/vm/src/windows.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/winapi.rs
🔇 Additional comments (12)
crates/vm/Cargo.toml (1)
115-140: LGTM!The addition of
Win32_System_Timeto the windows-sys features is correctly aligned with the changes intime.rsthat now useGetTimeZoneInformationfromwindows_sys.crates/vm/src/stdlib/nt.rs (1)
559-579: LGTM! The PathCchSkipRoot migration is correctly implemented.The implementation properly:
- Uses an output pointer for the
endparameter- Checks
hr == 0forS_OK(correct HRESULT check)- Validates the pointer is non-null before use
- Uses safe pointer arithmetic with
offset_fromand validates the resultOne 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/windows.rs (2)
13-15: LGTM! TheWinHandlewrapper provides clean Python interop.The wrapper correctly encapsulates the raw
HANDLEtype for safer usage across the codebase.
64-77: TheHandleInttype discrepancy is acknowledged.The existing TODO on line 64 notes that
HandleIntshould change toisizewhen fully ported to windows-rs. The currentusizeusage 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 likeINVALID_HANDLE_VALUE(-1).crates/vm/src/stdlib/winapi.rs (8)
77-79: LGTM! CloseHandle correctly uses WinHandle.
82-96: LGTM! GetStdHandle properly handles the NULL and INVALID_HANDLE_VALUE cases.The function correctly returns
Nonefor null handles and errors for invalid handles.
98-118: LGTM! CreatePipe migration is correct.The function properly initializes the handles using
MaybeUninitand wraps them inWinHandle.
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.
| 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.
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 | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/vm/src/stdlib/winapi.rs(12 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/winapi.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/winapi.rs (1)
crates/vm/src/stdlib/os.rs (1)
errno_err(39-41)
⏰ 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: 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
🔇 Additional comments (15)
crates/vm/src/stdlib/winapi.rs (15)
15-21: WinHandle/WindowsSysResult and windows_sys imports look consistentImporting
WinHandle,WindowsSysResult,INVALID_HANDLE_VALUE, andMAX_PATHfrom 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/WindowsSysResultWrapping
CloseHandleasfn CloseHandle(handle: WinHandle) -> WindowsSysResult<i32>and passinghandle.0matches the BOOL-returning Win32 API and keeps error handling centralized inWindowsSysResult.
82-96: GetStdHandle semantics preserved with WinHandleThe new
GetStdHandlereturningPyResult<Option<WinHandle>>correctly:
- Treats
INVALID_HANDLE_VALUEas an error viaerrno_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
CreatePipenow:
- Uses
MaybeUninit<HANDLE>for read/write ends.- Wraps the resulting handles as
WinHandlein 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 soundThe updated
DuplicateHandle:
- Accepts
WinHandleforsrc_process,src, andtarget_process.- Uses a
MaybeUninit<HANDLE>out-parameter.- Wraps the duplicated handle in
WinHandleon success.The argument order and conversion of
inherit/optionslook correct for the underlying API.
154-156: GetCurrentProcess returning WinHandle is consistentWrapping
GetCurrentProcess()inWinHandlealigns it with other APIs that now traffic inWinHandle. This matches its intended use as a pseudo-handle passed into other FFI calls.
158-169: GetFileType correctly updated to accept WinHandleTaking
h: WinHandleand callingGetFileType(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 handlesThe updated
CreateProcess:
- Continues to set up
STARTUPINFOEXW, environment, and attribute list as before.- Returns
WinHandle(procinfo.hProcess)andWinHandle(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 WinHandleThe new
OpenProcesswrapper:
- Takes
inherit_handle: booland converts it viai32::from(inherit_handle)to the expected BOOL.- Checks
handle.is_null()and maps failure toerrno_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.OpenProcesscorrectly handle the newPyResult<WinHandle>return type and adjusted signature.
445-445: AttrList handle list size calculation matches usize-based handlesUsing
(handlelist.len() * size_of::<usize>()) as _aligns the size passed toUpdateProcThreadAttributewith the type ofhandlelist: 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 onWAIT_FAILEDmirrors the Win32 API behavior, now using the typedWinHandlewrapper. No issues with the conversion or error propagation viaerrno_err(vm).
467-477: GetExitCodeProcess wrapper correctly uses WinHandle and WindowsSysResultThe function:
- Accepts
h: WinHandle.- Uses
MaybeUninitfor 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 straightforwardPassing
h.0intoTerminateProcessand wrapping the BOOL result inWindowsSysResult<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
LoadLibrarynow:
- Converts the path via
to_wide_with_nul.- Calls
LoadLibraryWfrom windows_sys.- Checks
handle.is_null()and raises a runtime error on failure.- Returns
handle as isizeon 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
isizeand handle failures via exceptions rather than sentinel values.
499-515: GetModuleFileName correctly casts handle and bounds the UTF-16 sliceCasting
handletoHMODULEand passing it toGetModuleFileNameW, then slicingpath[..length]before UTF-16 decoding, is the standard pattern here. The MAX_PATH-based buffer and thelength == 0failure check are unchanged in behavior.
| 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.
🧩 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
OpenMutexWcall will returnNULL, 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)🤖 Prompt for AI Agents
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.
close #6189
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.