crates/vm/src/stdlib/winreg.rs (2)

175-184: __exit__ duplicates Close logic—consider reusing.

This has the same two-lock issue as Close and duplicates the error message. Consider calling the existing Close method.

     #[pymethod(magic)]
     fn exit(zelf: PyRef<Self>, _args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> {
-        let res = unsafe { Registry::RegCloseKey(*zelf.hkey.write()) };
-        *zelf.hkey.write() = std::ptr::null_mut();
-        if res == 0 {
-            Ok(())
-        } else {
-            Err(vm.new_os_error("msg TODO".to_string()))
-        }
+        zelf.Close(vm)
     }

---

160-168: Remove or implement commented-out code.

Several blocks of commented-out code exist (AsHKEY, EnumKey). Consider either implementing these functions or removing the dead code to improve maintainability. Note that the commented EnumKey has a syntax error (std::mem::sizeof should be std::mem::size_of::<T>()).

Also applies to: 396-418

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/vm/src/stdlib/winreg.rs (10)

141-150: Duplicate: Double write lock acquisition and placeholder error message.

This issue was already flagged in a previous review. The two separate hkey.write() calls create a race window, and the error message is still a placeholder.

---

152-158: Duplicate: Detach does not prevent Drop from closing the handle.

This issue was already flagged in a previous review.

---

378-394: Duplicate: Incorrect parameter order in RegDeleteKeyExW call.

This issue was already flagged in a previous review. The reserved and access parameters are swapped.

---

460-505: Duplicate: Registry type is discarded, causing redundant API call.

This issue was already flagged in a previous review.

---

572-591: Duplicate: Null pointer passed as output parameter causes undefined behavior.

This issue was already flagged in a previous review.

---

629-649: Duplicate: QueryValue queries size but never retrieves or returns the actual value.

This issue was already flagged in a previous review.

---

673-689: Duplicate: Buffer length not set, causing empty slice.

This issue was already flagged in a previous review. The retBuf variable name also violates Rust naming conventions (ret_buf).

---

729-745: Duplicate: Encoding mismatch - UTF-8 string passed to UTF-16 API.

This issue was already flagged in a previous review.

---

899-899: Duplicate: Original error from py2reg is discarded.

This issue was already flagged in a previous review.

---

914-931: Duplicate: Non-null-terminated string passed to ANSI API, should use Wide variant.

This issue was already flagged in a previous review.

crates/vm/src/stdlib/winreg.rs (4)

115-117: Document the safety invariants for Send and Sync implementations.

The unsafe impl Send and unsafe impl Sync require documenting why they are safe. While Windows registry handles can technically be used across threads, the current implementation has race conditions (e.g., Close acquires the write lock twice). Consider adding a safety comment explaining the invariants:

-    // TODO: Fix
-    unsafe impl Send for PyHKEYObject {}
-    unsafe impl Sync for PyHKEYObject {}
+    // SAFETY: Registry handles (HKEY) can be used from any thread.
+    // Thread-safety is ensured by the PyRwLock wrapper which synchronizes access.
+    unsafe impl Send for PyHKEYObject {}
+    unsafe impl Sync for PyHKEYObject {}

---

175-184: __exit__ duplicates Close logic with the same issues.

The __exit__ method has the same double write lock acquisition problem and placeholder error message as Close. Consider reusing Close:

     #[pymethod]
     fn __exit__(zelf: PyRef<Self>, _args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> {
-        let res = unsafe { Registry::RegCloseKey(*zelf.hkey.write()) };
-        *zelf.hkey.write() = std::ptr::null_mut();
-        if res == 0 {
-            Ok(())
-        } else {
-            Err(vm.new_os_error("msg TODO".to_string()))
-        }
+        zelf.Close(vm)
     }

---

262-295: Consider reducing code duplication in ConnectRegistry.

The two branches differ only in the first parameter. This can be simplified:

 fn ConnectRegistry(
     computer_name: Option<String>,
     key: PyRef<PyHKEYObject>,
     vm: &VirtualMachine,
 ) -> PyResult<PyHKEYObject> {
+    let wide_computer_name = computer_name.map(|n| to_utf16(n));
     let mut ret_key = std::ptr::null_mut();
-    if let Some(computer_name) = computer_name {
-        let wide_computer_name = to_utf16(computer_name);
-        let res = unsafe {
-            Registry::RegConnectRegistryW(
-                wide_computer_name.as_ptr(),
-                *key.hkey.read(),
-                &mut ret_key,
-            )
-        };
-        // ...
-    } else {
-        let res = unsafe {
-            Registry::RegConnectRegistryW(std::ptr::null_mut(), *key.hkey.read(), &mut ret_key)
-        };
-        // ...
-    }
+    let res = unsafe {
+        Registry::RegConnectRegistryW(
+            wide_computer_name.as_ref().map_or(std::ptr::null(), |v| v.as_ptr()),
+            *key.hkey.read(),
+            &mut ret_key,
+        )
+    };
+    if res == 0 {
+        Ok(PyHKEYObject::new(ret_key))
+    } else {
+        Err(vm.new_os_error(format!("error code: {res}")))
+    }
 }

---

396-418: Consider removing or completing the commented-out EnumKey function.

The commented-out EnumKey implementation has a syntax error (std::mem::sizeof should be std::mem::size_of) and is incomplete. Either complete it or remove the commented code to reduce noise:

-    // #[pyfunction]
-    // fn EnumKey(key: PyRef<PyHKEYObject>, index: i32, vm: &VirtualMachine) -> PyResult<String> {
-    //     let mut tmpbuf = [0u16; 257];
-    //     let mut len = std::mem::sizeof(tmpbuf.len())/std::mem::sizeof(tmpbuf[0]);
     ...
-    // }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

crates/vm/src/stdlib/winreg.rs (3)

113-146: Avoid closing predefined HKEY handles and make Close a no‑op for them

PyHKEYObject always calls RegCloseKey in Drop for any non‑null handle, and the predefined keys (HKEY_CLASSES_ROOT, HKEY_CURRENT_USER, etc.) are created via PyHKEYObject::new(...) with those special pseudo‑handles. On Windows, calling RegCloseKey on predefined keys is documented as unsupported/undefined, and CPython explicitly treats them as non‑owned handles and makes CloseKey a no‑op for them. This can lead to undefined behavior or odd failures if the VM ever drops these objects.

Suggest tracking ownership on the wrapper and skipping close for predefined keys:

-    #[pyattr]
-    #[pyclass(name)]
-    #[derive(Debug, PyPayload)]
-    pub struct PyHKEYObject {
-        hkey: AtomicHKEY,
-    }
+    #[pyattr]
+    #[pyclass(name)]
+    #[derive(Debug, PyPayload)]
+    pub struct PyHKEYObject {
+        hkey: AtomicHKEY,
+        // true if this object owns the underlying handle and must close it
+        owns_handle: bool,
+    }

@@
-    impl PyHKEYObject {
-        pub fn new(hkey: *mut std::ffi::c_void) -> Self {
-            Self {
-                hkey: AtomicHKEY::new(hkey),
-            }
-        }
+    impl PyHKEYObject {
+        pub fn new(hkey: *mut std::ffi::c_void) -> Self {
+            Self {
+                hkey: AtomicHKEY::new(hkey),
+                owns_handle: true,
+            }
+        }
+
+        pub fn new_predefined(hkey: *mut std::ffi::c_void) -> Self {
+            Self {
+                hkey: AtomicHKEY::new(hkey),
+                owns_handle: false,
+            }
+        }
@@
-    #[pyattr(once)]
-    fn HKEY_CLASSES_ROOT(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_CLASSES_ROOT).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_CLASSES_ROOT(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_CLASSES_ROOT).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_CURRENT_USER(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_CURRENT_USER).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_CURRENT_USER(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_CURRENT_USER).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_LOCAL_MACHINE(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_LOCAL_MACHINE).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_LOCAL_MACHINE(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_LOCAL_MACHINE).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_USERS(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_USERS).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_USERS(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_USERS).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_PERFORMANCE_DATA(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_PERFORMANCE_DATA).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_PERFORMANCE_DATA(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_PERFORMANCE_DATA).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_CURRENT_CONFIG(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_CURRENT_CONFIG).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_CURRENT_CONFIG(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_CURRENT_CONFIG).into_ref(&vm.ctx)
+    }
@@
-    #[pyattr(once)]
-    fn HKEY_DYN_DATA(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
-        PyHKEYObject::new(Registry::HKEY_DYN_DATA).into_ref(&vm.ctx)
-    }
+    #[pyattr(once)]
+    fn HKEY_DYN_DATA(vm: &VirtualMachine) -> PyRef<PyHKEYObject> {
+        PyHKEYObject::new_predefined(Registry::HKEY_DYN_DATA).into_ref(&vm.ctx)
+    }
@@
     #[pymethod]
     fn Close(&self, vm: &VirtualMachine) -> PyResult<()> {
-        // Atomically swap the handle with null and get the old value
-        let old_hkey = self.hkey.swap(std::ptr::null_mut());
+        // Predefined HKEYs are not owned and must not be closed – make Close a no‑op.
+        if !self.owns_handle {
+            return Ok(());
+        }
+        // Atomically swap the handle with null and get the old value
+        let old_hkey = self.hkey.swap(std::ptr::null_mut());
@@
     impl Drop for PyHKEYObject {
         fn drop(&mut self) {
             let hkey = self.hkey.swap(std::ptr::null_mut());
-            if !hkey.is_null() {
+            if !hkey.is_null() && self.owns_handle {
                 unsafe { Registry::RegCloseKey(hkey) };
             }
         }
     }

This keeps ownership semantics correct for opened/created keys, avoids closing pseudo‑handles, and keeps Detach behavior unchanged (it already nulls the handle).

Also applies to: 148-223

---

671-760: Fix unsafe &[u8] → &[u16] casts for UTF‑16 strings (potential UB)

Both QueryValue and reg_to_py reinterpret a Vec<u8> buffer as &[u16] via from_raw_parts(buffer.as_ptr() as *const u16, ...). The original allocation is for u8, whose alignment is 1; creating a &[u16] from that pointer violates Rust’s alignment requirements and is undefined behavior even if it “works” on x86.

You can safely reconstruct UTF‑16 code units with chunks_exact(2) and u16::from_le_bytes (Win32 registry data is little‑endian):

@@ fn QueryValue(key: HKEYArg, sub_key: Option<String>, vm: &VirtualMachine) -> PyResult<String> {
-            // Convert UTF-16 to String
-            let u16_slice = unsafe {
-                std::slice::from_raw_parts(buffer.as_ptr() as *const u16, size as usize / 2)
-            };
-            let len = u16_slice
-                .iter()
-                .position(|&c| c == 0)
-                .unwrap_or(u16_slice.len());
-            break String::from_utf16(&u16_slice[..len])
+            // Convert UTF‑16 (little‑endian) bytes to u16 safely
+            let u16_vec: Vec<u16> = buffer[..size as usize]
+                .chunks_exact(2)
+                .map(|chunk| u16::from_le_bytes([chunk[0], chunk[1]]))
+                .collect();
+            let len = u16_vec
+                .iter()
+                .position(|&c| c == 0)
+                .unwrap_or(u16_vec.len());
+            break String::from_utf16(&u16_vec[..len])
                 .map_err(|e| vm.new_value_error(format!("UTF16 error: {e}")));
@@ fn reg_to_py(vm: &VirtualMachine, ret_data: &[u8], typ: u32) -> PyResult {
-            REG_SZ | REG_EXPAND_SZ => {
-                // Treat the data as a UTF-16 string.
-                let u16_count = ret_data.len() / 2;
-                let u16_slice = unsafe {
-                    std::slice::from_raw_parts(ret_data.as_ptr() as *const u16, u16_count)
-                };
-                // Only use characters up to the first NUL.
-                let len = u16_slice
-                    .iter()
-                    .position(|&c| c == 0)
-                    .unwrap_or(u16_slice.len());
-                let s = String::from_utf16(&u16_slice[..len])
+            REG_SZ | REG_EXPAND_SZ => {
+                // Treat the data as a UTF-16 (little-endian) string.
+                let u16_vec: Vec<u16> = ret_data
+                    .chunks_exact(2)
+                    .map(|chunk| u16::from_le_bytes([chunk[0], chunk[1]]))
+                    .collect();
+                // Only use characters up to the first NUL.
+                let len = u16_vec
+                    .iter()
+                    .position(|&c| c == 0)
+                    .unwrap_or(u16_vec.len());
+                let s = String::from_utf16(&u16_vec[..len])
@@
-            REG_MULTI_SZ => {
-                if ret_data.is_empty() {
-                    Ok(vm.ctx.new_list(vec![]).into())
-                } else {
-                    let u16_count = ret_data.len() / 2;
-                    let u16_slice = unsafe {
-                        std::slice::from_raw_parts(ret_data.as_ptr() as *const u16, u16_count)
-                    };
-
-                    // Remove trailing null if present (like CPython's countStrings)
-                    let len = if u16_count > 0 && u16_slice[u16_count - 1] == 0 {
-                        u16_count - 1
-                    } else {
-                        u16_count
-                    };
-
-                    let mut strings: Vec<PyObjectRef> = Vec::new();
-                    let mut start = 0;
-                    for i in 0..len {
-                        if u16_slice[i] == 0 {
-                            let s = String::from_utf16(&u16_slice[start..i])
+            REG_MULTI_SZ => {
+                if ret_data.is_empty() {
+                    Ok(vm.ctx.new_list(vec![]).into())
+                } else {
+                    let u16_vec: Vec<u16> = ret_data
+                        .chunks_exact(2)
+                        .map(|chunk| u16::from_le_bytes([chunk[0], chunk[1]]))
+                        .collect();
+
+                    // Remove trailing null if present (like CPython's countStrings)
+                    let len = if !u16_vec.is_empty() && u16_vec[u16_vec.len() - 1] == 0 {
+                        u16_vec.len() - 1
+                    } else {
+                        u16_vec.len()
+                    };
+
+                    let mut strings: Vec<PyObjectRef> = Vec::new();
+                    let mut start = 0;
+                    for i in 0..len {
+                        if u16_vec[i] == 0 {
+                            let s = String::from_utf16(&u16_vec[start..i])
                                 .map_err(|e| vm.new_value_error(format!("UTF16 error: {e}")))?;
                             strings.push(vm.ctx.new_str(s).into());
                             start = i + 1;
                         }
                     }
                     // Handle last string if not null-terminated
                     if start < len {
-                        let s = String::from_utf16(&u16_slice[start..len])
+                        let s = String::from_utf16(&u16_vec[start..len])
                             .map_err(|e| vm.new_value_error(format!("UTF16 error: {e}")))?;
                         strings.push(vm.ctx.new_str(s).into());
                     }

This removes UB while preserving behavior for all supported string types.

Also applies to: 919-987

---

770-832: Honor the actual size returned by RegQueryValueExW before passing data to reg_to_py

QueryValueEx resizes the buffer on ERROR_MORE_DATA, but after the final successful call it ignores ret_size and passes the entire ret_buf slice to reg_to_py. For REG_BINARY or other non‑string types this can expose extra trailing zero bytes that are not part of the value, diverging from CPython’s behavior.

You already track ret_size inside the loop; make it persist and truncate the buffer before conversion:

-        let mut ret_buf = vec![0u8; buf_size as usize];
-        let mut typ = 0;
-
-        // Loop to handle ERROR_MORE_DATA
-        loop {
-            let mut ret_size = buf_size;
+        let mut ret_buf = vec![0u8; buf_size as usize];
+        let mut typ = 0;
+        let mut ret_size: u32 = buf_size;
+
+        // Loop to handle ERROR_MORE_DATA
+        loop {
+            ret_size = buf_size;
             let res = unsafe {
                 Registry::RegQueryValueExW(
                     hkey,
                     wide_name.as_ptr(),
                     std::ptr::null_mut(),
                     &mut typ,
                     ret_buf.as_mut_ptr(),
                     &mut ret_size,
                 )
             };
@@
-        }
-
-        let obj = reg_to_py(vm, ret_buf.as_slice(), typ)?;
+        }
+
+        // Only pass the bytes actually returned by the API.
+        ret_buf.truncate(ret_size as usize);
+        let obj = reg_to_py(vm, ret_buf.as_slice(), typ)?;

This keeps integer and string cases working as today, while making binary/unknown types match the real registry value length.

crates/vm/src/stdlib/winreg.rs (2)

392-401: DeleteKey behavior (ERROR 5) and more precise error mapping

The RegDeleteKeyW call itself looks correct; ERROR_ACCESS_DENIED (5) is what Windows returns when the parent key lacks delete rights (e.g. opened with KEY_READ instead of KEY_WRITE/KEY_ALL_ACCESS). That likely explains the “always returns 5” behavior you mentioned: the tests probably open the parent with read‑only access.

Two improvements would help here:

1. Make the error more informative (and consistent with other helpers) by using os_error_from_windows_code:

-        if res == 0 {
-            Ok(())
-        } else {
-            Err(vm.new_os_error(format!("error code: {res}")))
-        }
+        if res == 0 {
+            Ok(())
+        } else {
+            Err(os_error_from_windows_code(vm, res as i32, "RegDeleteKey"))
+        }

2. Consider documenting (or asserting in tests) that the key argument must be opened with appropriate delete/write access (mirroring CPython’s DeleteKey docs), rather than treating ERROR 5 here as an implementation bug.

---

1170-1186: Handle long expansions and returned size in ExpandEnvironmentStrings

ExpandEnvironmentStrings currently uses a fixed 1024‑wide buffer and then scans for the first NUL, ignoring the value returned in r. If the expansion is longer than 1023 characters, the API will indicate the required size in r, but you never reallocate and retry, so large expansions will be truncated or fail unexpectedly.

You can honor the returned size and re‑call once if needed:

     fn ExpandEnvironmentStrings(i: String, vm: &VirtualMachine) -> PyResult<String> {
         let wide_input = i.to_wide_with_nul();
-        let mut out = vec![0u16; 1024];
-        let r = unsafe {
+        let mut out: Vec<u16> = vec![0u16; 1024];
+        let mut r = unsafe {
             windows_sys::Win32::System::Environment::ExpandEnvironmentStringsW(
                 wide_input.as_ptr(),
                 out.as_mut_ptr(),
                 out.len() as u32,
             )
         };
         if r == 0 {
             return Err(vm.new_os_error("ExpandEnvironmentStringsW failed".to_string()));
         }
-        let len = out.iter().position(|&c| c == 0).unwrap_or(out.len());
-        String::from_utf16(&out[..len]).map_err(|e| vm.new_value_error(format!("UTF16 error: {e}")))
+        // If the buffer was too small, r is the required size including the null terminator.
+        if (r as usize) > out.len() {
+            out.resize(r as usize, 0);
+            r = unsafe {
+                windows_sys::Win32::System::Environment::ExpandEnvironmentStringsW(
+                    wide_input.as_ptr(),
+                    out.as_mut_ptr(),
+                    out.len() as u32,
+                )
+            };
+            if r == 0 {
+                return Err(vm.new_os_error("ExpandEnvironmentStringsW failed".to_string()));
+            }
+        }
+        // r includes the terminating NUL.
+        let len = (r as usize).saturating_sub(1);
+        String::from_utf16(&out[..len])
+            .map_err(|e| vm.new_value_error(format!("UTF16 error: {e}")))
     }

This matches the documented contract of ExpandEnvironmentStringsW and avoids silent truncation for long environment expansions.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

crates/vm/src/stdlib/winreg.rs (3)

31-43: Consider making UTF‑16 conversion resilient to misalignment

bytes_as_wide_slice uses align_to::<u16>() and then debug‑asserts that the prefix/suffix are empty. This is usually fine for registry buffers, but it relies on the backing allocation being 2‑byte aligned in practice rather than by Rust’s type system. If that assumption ever fails, you’ll silently drop leading/trailing bytes in release builds.

To make this more robust while keeping zero‑copy in the common case, you could:

• Check if !prefix.is_empty() || !suffix.is_empty() and, in that case, fall back to allocating a Vec<u16> by chunking the bytes manually; keep the current path when both are empty.
• Alternatively, skip align_to entirely in reg_to_py and always construct Vec<u16> via chunks_exact(2); it’s slightly more allocation but clearly correct and avoids relying on allocator alignment.

The current implementation is safe (no UB) but a bit brittle around that assumption.

Also applies to: 933-996

---

1097-1145: SetValueEx correctly wires py2reg output into RegSetValueExW

• Distinguishes between Some(v) and None from py2reg to either:
  • Set the given data blob, or
  • Write a zero‑length value (for types where that is meaningful).
• Uses value_name.to_wide_with_nul() for the value name and passes the exact len from py2reg.
• Propagates any error from py2reg directly.

One small stylistic improvement: you could avoid duplicating the RegSetValueExW call by factoring the common parts and just varying (ptr, len), but that’s purely optional.

---

1148-1177: Reflection key helpers are thin and correct

• DisableReflectionKey, EnableReflectionKey, and QueryReflectionKey wrap the respective APIs and map non‑zero results to OSError.
• QueryReflectionKey interprets a non‑zero BOOL as true.

Behaviorally this matches the Win32 and CPython semantics for key reflection. (learn.microsoft.com)

On very old or non‑server Windows versions, these APIs may return ERROR_CALL_NOT_IMPLEMENTED; if you want perfect CPython parity you could consider turning that into NotImplementedError, but that’s an optional polish.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

crates/vm/src/stdlib/winreg.rs (1)

120-153: Critical: Predefined HKEY constants must not be closed.

The predefined registry keys (HKEY_CLASSES_ROOT, HKEY_CURRENT_USER, etc.) are pseudo-handles that should never be passed to RegCloseKey. According to Windows documentation, calling RegCloseKey on these handles can cause undefined behavior or corruption on certain Windows versions.

The current implementation wraps these predefined keys in PyHkey instances, which means:

1. If a user calls .Close() on them, they will attempt to close the predefined handle
2. When the PyHkey is dropped, the Drop impl will also call RegCloseKey

You need to distinguish predefined keys from opened keys. Options:

Option 1: Add an is_predefined flag

 #[pyclass(name = "HKEYType")]
 #[derive(Debug, PyPayload)]
 struct PyHkey {
     hkey: AtomicHKEY,
+    is_predefined: bool,
 }

 impl PyHkey {
-    fn new(hkey: Registry::HKEY) -> Self {
+    fn new(hkey: Registry::HKEY) -> Self {
+        Self::new_with_flag(hkey, false)
+    }
+
+    fn new_predefined(hkey: Registry::HKEY) -> Self {
+        Self::new_with_flag(hkey, true)
+    }
+
+    fn new_with_flag(hkey: Registry::HKEY, is_predefined: bool) -> Self {
         Self {
             hkey: AtomicHKEY::new(hkey),
+            is_predefined,
         }
     }
 }

 impl Drop for PyHkey {
     fn drop(&mut self) {
+        if self.is_predefined {
+            return;
+        }
         let hkey = self.hkey.swap(std::ptr::null_mut());
         if !hkey.is_null() {
             unsafe { Registry::RegCloseKey(hkey) };
         }
     }
 }

 #[pyclass(with(AsNumber, Hashable))]
 impl PyHkey {
     #[pymethod]
     fn Close(&self, vm: &VirtualMachine) -> PyResult<()> {
+        if self.is_predefined {
+            return Ok(()); // Silently succeed for predefined keys
+        }
         // ... existing close logic
     }

     #[pymethod]
     fn Detach(&self) -> PyResult<usize> {
+        if self.is_predefined {
+            return Err(vm.new_value_error("Cannot detach predefined registry keys".to_string()));
+        }
         // ... existing detach logic
     }
 }

Then update the constant definitions:

 #[pyattr(once)]
 fn HKEY_CLASSES_ROOT(vm: &VirtualMachine) -> PyRef<PyHkey> {
-    PyHkey::new(Registry::HKEY_CLASSES_ROOT).into_ref(&vm.ctx)
+    PyHkey::new_predefined(Registry::HKEY_CLASSES_ROOT).into_ref(&vm.ctx)
 }

Based on past review comments, this issue was supposedly addressed in commits c2ca9a7 to 7c8c043, but the current code still shows the same vulnerability.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro