Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Feb 5, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Windows socket error reporting with improved error code preservation and consistency.
  • Improvements

    • Strengthened stability through added buffer validation for async I/O operations.
    • Improved garbage collection support for overlapped socket operations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 5, 2026

📝 Walkthrough

Walkthrough

This change refactors the Windows overlapped I/O module by introducing module initialization with Winsock setup, adding garbage collection traversal support through Traverse implementations, updating function signatures for stricter type safety, centralizing error handling via a new helper, and caching operation results in data structures.

Changes

Cohort / File(s) Summary
Module Initialization & GC Support
crates/stdlib/src/overlapped.rs
Added module_exec function for Winsock initialization and updated #[pyclass] with traverse support; extended imports for PyModule, PyStrRef, PyTupleRef, and Traverse trait implementations for OverlappedInner, OverlappedReadFrom, and OverlappedReadFromInto.
API Signature Changes
crates/stdlib/src/overlapped.rs
Updated WSAConnect, parse_address to accept PyTupleRef instead of PyObjectRef; changed unparse_address return type from bare PyObjectRef to PyResult for explicit error handling; replaced from_windows_err helper with set_from_windows_err for uniform Windows error translation.
Data Structure Enhancements
crates/stdlib/src/overlapped.rs
Added result: Option<PyObjectRef> field to OverlappedReadFrom and OverlappedReadFromInto to cache operation results; updated Debug formatting to display result field; modified initialization code to set result: None.
Address Parsing & Error Handling
crates/stdlib/src/overlapped.rs
Enhanced IPv4/IPv6 address parsing to handle port restoration and return address bytes with updated length; refactored address unparsing to use std::net types (Ipv4Addr, Ipv6Addr); added buffer size checks and contiguity validation; consolidated error paths through set_from_windows_err.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 In Windows lands where sockets dance,
We hop through overlapping chance,
With traversal spring and errors caught,
Fresh signatures and results sought!
Winsock hops with module grace—
GC cleanup finds its place!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.19% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'align overlapped to CPython 3.14.2' is clear and specific, directly describing the PR's main objective of aligning the _overlapped module implementation with CPython 3.14.2 compatibility standards.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@youknowone youknowone marked this pull request as ready for review February 5, 2026 13:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/stdlib/src/overlapped.rs (2)

1826-1862: ⚠️ Potential issue | 🟡 Minor

Fix incorrect error message for unsupported address family.
BindLocal takes an address family, not a tuple length.

Proposed fix
-            return Err(vm.new_value_error("expected tuple of length 2 or 4".to_owned()));
+            return Err(vm.new_value_error("unsupported address family".to_owned()));

1395-1482: ⚠️ Potential issue | 🔴 Critical

Validate size against buffer length to prevent out-of-bounds writes.

WSARecvFrom writes WSABUF.len bytes into the buffer. If size exceeds buf.desc.len, Windows writes past the buffer boundary. Compare this with WSARecvInto, which correctly uses the buffer length directly.

Proposed fix
             let buf_len = buf.desc.len;
             if buf_len > u32::MAX as usize {
                 return Err(vm.new_value_error("buffer too large".to_owned()));
             }
+            if size as usize > buf_len {
+                return Err(vm.new_value_error("size exceeds buffer length".to_owned()));
+            }

@youknowone youknowone merged commit cdb7b0d into RustPython:main Feb 5, 2026
12 of 13 checks passed
@youknowone youknowone deleted the overlapped branch February 5, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant