Actionable comments posted: 0

crates/stdlib/src/openssl/cert.rs (1)

161-178: IPv6 slice-to-array conversion is correct; can be slightly simplified

ip.len() == 16 guard makes the unwrap() on try_from(&ip[0..16]) safe, so behavior is correct. If you want to tighten it up, you can lean on TryInto for slightly clearer code:

-                        } else if ip.len() == 16 {
-                            // IPv6 - format with all zeros visible (not compressed)
-                            let ip_addr =
-                                std::net::Ipv6Addr::from(<[u8; 16]>::try_from(&ip[0..16]).unwrap());
+                        } else if ip.len() == 16 {
+                            // IPv6 - format with all zeros visible (not compressed)
+                            let bytes: [u8; 16] = ip.as_ref().try_into().unwrap();
+                            let ip_addr = std::net::Ipv6Addr::from(bytes);

Purely a readability/idiomatic tweak; no functional issue.

crates/stdlib/src/openssl.rs (1)

534-552: Consider removing unused _ssl_ptr field from HandshakeVmGuard.

The _ssl_ptr field is stored during construction but never accessed afterward. The Drop implementation only clears the thread-local storage without referencing this field. This appears to be dead code, possibly left over from an earlier design.

     // RAII guard to set/clear thread-local handshake context
     struct HandshakeVmGuard {
-        _ssl_ptr: *mut sys::SSL,
     }
 
     impl HandshakeVmGuard {
         fn new(vm: &VirtualMachine, ssl_ptr: *mut sys::SSL) -> Self {
             HANDSHAKE_VM.with(|cell| cell.set(Some(vm as *const _)));
             HANDSHAKE_SSL_PTR.with(|cell| cell.set(Some(ssl_ptr)));
-            HandshakeVmGuard { _ssl_ptr: ssl_ptr }
+            HandshakeVmGuard {}
         }
     }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between a4c93df and 96f3a2d.

• crates/stdlib/src/openssl.rs (38 hunks)
• crates/stdlib/src/openssl/cert.rs (1 hunks)
• crates/stdlib/src/ssl.rs (2 hunks)
• crates/stdlib/src/ssl/compat.rs (1 hunks)
• crates/stdlib/src/ssl/error.rs (1 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/stdlib/src/ssl/compat.rs
• crates/stdlib/src/openssl/cert.rs
• crates/stdlib/src/ssl/error.rs
• crates/stdlib/src/openssl.rs
• crates/stdlib/src/ssl.rs

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.

Applied to files:

• crates/stdlib/src/openssl.rs

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

• crates/stdlib/src/ssl.rs

crates/stdlib/src/ssl/error.rs (4)

• create_ssl_eof_error (102-108)
• create_ssl_want_read_error (86-92)
• create_ssl_want_write_error (94-100)
• create_ssl_zero_return_error (110-116)

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

• strerror (1544-1548)

crates/stdlib/src/ssl.rs (2)

• _msg_callback (1691-1693)
• e (1131-1131)

crates/stdlib/src/csv.rs (1)

• error (38-44)

crates/stdlib/src/ssl/error.rs (1)

• create_ssl_want_read_error (86-92)

crates/stdlib/src/ssl/compat.rs (1)

33-40: Shared SSL error imports are wired correctly

Importing PySSLSocket from super::_ssl and the various SSL error types/helpers from super::error matches the new centralized error module and keeps compat backend-agnostic. Looks good.

crates/stdlib/src/ssl/error.rs (1)

3-117: Centralized SSL error hierarchy and helpers look consistent

The new ssl_error module cleanly centralizes SSL exception types and the WANT/EOF/zero-return constructors. The custom PySSLError.__str__ behavior (preferring strerror, otherwise falling back to args) aligns with OSError semantics, and the helpers correctly subtype PyOSError with appropriate errno values. This should make error handling consistent across rustls and OpenSSL backends.

crates/stdlib/src/ssl.rs (1)

3340-3363: pending() correctly reports buffered plaintext using rustls Reader

PySSLSocket::pending locks the connection, creates a rustls Reader, and uses BufRead::fill_buf() to determine how many decrypted bytes are currently buffered without consuming them. Since rustls Reader<'_> implements BufRead, this is a non‑destructive way to emulate OpenSSL’s SSL_pending(). Returning 0 on errors (WouldBlock or others) also matches OpenSSL’s behavior of reporting “no pending data” instead of surfacing I/O errors via pending(). This is a good fit for asyncore‑style readable() checks.

crates/stdlib/src/openssl.rs (7)

593-606: LGTM - Manual ex_data cleanup is correctly implemented.

The cleanup_sni_ex_data function properly handles the lifecycle of SniCallbackData:

• Checks for null before freeing (safe for multiple calls)
• Clears the ex_data pointer to prevent double-free
• Called explicitly after handshake to release references promptly

---

712-727: LGTM - Intentional no-op callback is well-documented.

The comment clearly explains the deadlock concerns (referencing CPython bpo-43577) and why this callback is intentionally empty. This is a reasonable design choice that prioritizes stability over the feature.

---

2188-2192: LGTM - Simple and correct pending() implementation.

The method correctly exposes OpenSSL's SSL_pending function, which returns the number of bytes available for immediate read. Implementation is straightforward and matches CPython's behavior.

---

3176-3180: LGTM - Unified error creation helper.

The new_ssl_error helper provides a consistent way to create SSL errors as proper OSError subtypes, which aligns with CPython's behavior. This centralizes the error creation pattern used throughout the module.

---

2406-2483: LGTM - Comprehensive handshake cleanup across all exit paths.

The do_handshake implementation correctly calls cleanup_sni_ex_data on all exit paths (success, timeout, closed, error). The HandshakeVmGuard RAII pattern ensures the thread-local state is properly managed. The safety comments appropriately document the lifetime requirements.

---

1704-1720: LGTM - Weak reference prevents reference cycle.

Using PyWeak for ssl_socket_weak correctly breaks the reference cycle that would otherwise occur: PySslSocket → SslStream → SSL → ex_data → SniCallbackData → back to PySslSocket. The implementation properly minimizes lock contention by checking has_sni_callback before doing the more expensive weak reference creation.

---

554-562: LGTM - Correctly avoids deadlock during handshake callbacks.

The function properly handles the scenario where set_context might be called from within an SNI callback during handshake (when the connection lock is already held). By checking the thread-local HANDSHAKE_SSL_PTR first, it avoids attempting to re-acquire the lock.