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

206-223: Verify handle lifetime and error semantics for standard vs non-standard fds.

Two observations:

1. For non-standard fds (lines 214-218), the BorrowedHandle from crt_fd::as_handle goes out of scope after extracting the raw handle value. This is safe because the raw value remains valid as long as the underlying fd is open, consistent with similar patterns elsewhere in this file.

2. For the null/invalid handle check (lines 221-223): when GetStdHandle returns NULL (no associated handle), calling errno_err may report a stale or misleading error since no Windows error was set. Consider setting a specific error or differentiating between the two failure modes for clearer diagnostics.

         if h.is_null() || h == INVALID_HANDLE_VALUE {
+            // Note: NULL means no console attached (not an error condition per se),
+            // while INVALID_HANDLE_VALUE indicates a true error
             return Err(errno_err(vm));
         }

---

257-261: Consider guarding against invalid window rectangles.

The srWindow coordinates are i16 (SHORT). If Right < Left or Bottom < Top (malformed rect), the subtraction yields a negative value that wraps to a large usize. While Windows should never return such a rect, a defensive check avoids surprising behavior in edge cases.

         let csbi = unsafe { csbi.assume_init() };
         let w = csbi.srWindow;
-        let columns = (w.Right - w.Left + 1) as usize;
-        let lines = (w.Bottom - w.Top + 1) as usize;
+        let columns = (w.Right.saturating_sub(w.Left).saturating_add(1)) as usize;
+        let lines = (w.Bottom.saturating_sub(w.Top).saturating_add(1)) as usize;
         Ok(_os::TerminalSizeData { columns, lines })

Alternatively, if matching CPython's exact behavior is preferred, the current implementation is acceptable.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

206-215: Consider removing the redundant INVALID_HANDLE_VALUE check.

The crt_fd::as_handle() function returns io::Result<BorrowedHandle<'_>>, which means it already validates the handle internally. The explicit check for INVALID_HANDLE_VALUE on lines 213-215 is redundant because as_handle() would have returned an error if the handle were invalid.

Apply this diff to remove the redundant check:

 let fd = fd.unwrap_or(1); // default to stdout

 // Use _get_osfhandle for all fds
 let borrowed = unsafe { crt_fd::Borrowed::borrow_raw(fd) };
 let handle = crt_fd::as_handle(borrowed).map_err(|e| e.to_pyexception(vm))?;
 let h = handle.as_raw_handle() as Foundation::HANDLE;

-if h == INVALID_HANDLE_VALUE {
-    return Err(errno_err(vm));
-}
-
 let mut csbi = MaybeUninit::uninit();

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

238-243: Consider saving the error before CloseHandle to preserve the correct error code.

If GetConsoleScreenBufferInfo fails and then CloseHandle also fails (rare but possible), the error code from GetLastError() captured by errno_err would be from CloseHandle rather than from the actual console operation.

         let ret =
             unsafe { Console::GetConsoleScreenBufferInfo(console_handle, csbi.as_mut_ptr()) };
+        let err = if ret == 0 {
+            Some(errno_err(vm))
+        } else {
+            None
+        };
         unsafe { Foundation::CloseHandle(console_handle) };
-        if ret == 0 {
-            return Err(errno_err(vm));
+        if let Some(e) = err {
+            return Err(e);
         }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

223-240: Consider using RAII for console handle resource management.

The console_handle is manually opened and closed, which could leak the handle if a panic occurs between lines 234 and 240. While panics are unlikely in this code path, following Rust best practices would involve wrapping the handle in an RAII type with a Drop implementation.

Additionally, GetConsoleScreenBufferInfo only requires GENERIC_READ access, so GENERIC_WRITE on line 227 may be unnecessary.

Consider wrapping the handle:

struct ConsoleHandle(Foundation::HANDLE);

impl Drop for ConsoleHandle {
    fn drop(&mut self) {
        unsafe { Foundation::CloseHandle(self.0) };
    }
}

Then use:

-            let console_handle = unsafe {
+            let console_handle = ConsoleHandle(unsafe {
                 FileSystem::CreateFileW(
                     conout.as_ptr(),
-                    Foundation::GENERIC_READ | Foundation::GENERIC_WRITE,
+                    Foundation::GENERIC_READ,
                     FileSystem::FILE_SHARE_READ | FileSystem::FILE_SHARE_WRITE,
                     std::ptr::null(),
                     FileSystem::OPEN_EXISTING,
                     0,
                     std::ptr::null_mut(),
                 )
-            };
+            });
             if console_handle == INVALID_HANDLE_VALUE {
                 return Err(errno_err(vm));
             }
             let ret =
-                unsafe { Console::GetConsoleScreenBufferInfo(console_handle, csbi.as_mut_ptr()) };
-            unsafe { Foundation::CloseHandle(console_handle) };
+                unsafe { Console::GetConsoleScreenBufferInfo(console_handle.0, csbi.as_mut_ptr()) };

Based on coding guidelines, Rust best practices for resource management should be followed.

---

245-249: Optional: Add defensive bounds checking for window coordinates.

The calculation assumes w.Right >= w.Left and w.Bottom >= w.Top. While the Windows API should always return valid bounds, adding defensive checks would make the code more robust against invalid data.

         let csbi = unsafe { csbi.assume_init() };
         let w = csbi.srWindow;
+        if w.Right < w.Left || w.Bottom < w.Top {
+            return Err(vm.new_os_error("Invalid console screen buffer dimensions".to_string()));
+        }
         let columns = (w.Right - w.Left + 1) as usize;
         let lines = (w.Bottom - w.Top + 1) as usize;

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro