Fix openssl build and shared ssl/error.rs #6456
Conversation
WalkthroughThis PR centralizes SSL error handling by introducing a shared Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/ssl.rs (1)
1129-1175:create_ssl_error_with_reasoncalls leave.reasonempty for PEM errorsIn the
load_cert_chainerror mapping, both the generic PEM error branch and the fallback branch use:super::compat::SslError::create_ssl_error_with_reason( vm, Some("SSL"), "", "PEM lib", )
create_ssl_error_with_reasonis documented to take a human‑readable reason string and a separate message, and it stores the reason into thereasonattribute on theSSLErrorinstance. Passing""as the reason meansSSLError.reasonwill be an empty string, which contradicts the helper’s intent and comments like// [SSL] PEM lib. The same pattern appears inload_certs_from_file_helperwithSome("X509"), "", "PEM lib".This will make library/reason metadata inconsistent and can break tests or user code that inspects
exc.reasonfor PEM/X509 failures.Consider instead populating the
reasonfield with"PEM lib"while keeping the existing message text:- super::compat::SslError::create_ssl_error_with_reason( - vm, - Some("SSL"), - "", - "PEM lib", - ) + super::compat::SslError::create_ssl_error_with_reason( + vm, + Some("SSL"), + "PEM lib", + "PEM lib", + )and similarly in
load_certs_from_file_helper:- _ => super::compat::SslError::create_ssl_error_with_reason( - vm, - Some("X509"), - "", - "PEM lib", - ), + _ => super::compat::SslError::create_ssl_error_with_reason( + vm, + Some("X509"), + "PEM lib", + "PEM lib", + ),This keeps the user‑visible message unchanged while ensuring
SSLError.reasoncarries a meaningful value instead of the empty string.crates/stdlib/src/openssl.rs (1)
3404-3408: Remove debug print statement.This
eprintln!appears to be leftover debug output that should be removed before release. Debug prints to stderr can pollute application output and confuse users.// Successfully loaded at least one certificate from DER data. // Clear any trailing errors from EOF. // CPython clears errors when: // - DER: was_bio_eof is set (EOF reached) // - PEM: PEM_R_NO_START_LINE error (normal EOF) // Both cases mean successful completion with loaded certs. - eprintln!( - "[x509_stack_from_der] SUCCESS: Clearing errors and returning {} certs", - certs.len() - ); sys::ERR_clear_error(); Ok(certs)
🧹 Nitpick comments (2)
crates/stdlib/src/openssl/cert.rs (1)
161-178: IPv6 slice-to-array conversion is correct; can be slightly simplified
ip.len() == 16guard makes theunwrap()ontry_from(&ip[0..16])safe, so behavior is correct. If you want to tighten it up, you can lean onTryIntofor 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_ptrfield fromHandshakeVmGuard.The
_ssl_ptrfield is stored during construction but never accessed afterward. TheDropimplementation 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 {} } }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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)
🧰 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/stdlib/src/ssl/compat.rscrates/stdlib/src/openssl/cert.rscrates/stdlib/src/ssl/error.rscrates/stdlib/src/openssl.rscrates/stdlib/src/ssl.rs
🧠 Learnings (2)
📚 Learning: 2025-12-09T08:46:58.660Z
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
📚 Learning: 2025-11-29T12:17:28.606Z
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
🧬 Code graph analysis (4)
crates/stdlib/src/ssl/compat.rs (1)
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/stdlib/src/ssl/error.rs (1)
crates/vm/src/stdlib/os.rs (1)
strerror(1544-1548)
crates/stdlib/src/openssl.rs (1)
crates/stdlib/src/ssl.rs (2)
_msg_callback(1691-1693)e(1131-1131)
crates/stdlib/src/ssl.rs (2)
crates/stdlib/src/csv.rs (1)
error(38-44)crates/stdlib/src/ssl/error.rs (1)
create_ssl_want_read_error(86-92)
🔇 Additional comments (10)
crates/stdlib/src/ssl/compat.rs (1)
33-40: Shared SSL error imports are wired correctlyImporting
PySSLSocketfromsuper::_ssland the various SSL error types/helpers fromsuper::errormatches the new centralized error module and keepscompatbackend-agnostic. Looks good.crates/stdlib/src/ssl/error.rs (1)
3-117: Centralized SSL error hierarchy and helpers look consistentThe new
ssl_errormodule cleanly centralizes SSL exception types and the WANT/EOF/zero-return constructors. The customPySSLError.__str__behavior (preferringstrerror, otherwise falling back toargs) aligns with OSError semantics, and the helpers correctly subtypePyOSErrorwith 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 rustlsReader
PySSLSocket::pendinglocks the connection, creates a rustlsReader, and usesBufRead::fill_buf()to determine how many decrypted bytes are currently buffered without consuming them. Since rustlsReader<'_>implementsBufRead, this is a non‑destructive way to emulate OpenSSL’sSSL_pending(). Returning0on errors (WouldBlockor others) also matches OpenSSL’s behavior of reporting “no pending data” instead of surfacing I/O errors viapending(). This is a good fit for asyncore‑stylereadable()checks.crates/stdlib/src/openssl.rs (7)
593-606: LGTM - Manual ex_data cleanup is correctly implemented.The
cleanup_sni_ex_datafunction properly handles the lifecycle ofSniCallbackData:
- 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 correctpending()implementation.The method correctly exposes OpenSSL's
SSL_pendingfunction, 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_errorhelper 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_handshakeimplementation correctly callscleanup_sni_ex_dataon all exit paths (success, timeout, closed, error). TheHandshakeVmGuardRAII 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
PyWeakforssl_socket_weakcorrectly breaks the reference cycle that would otherwise occur:PySslSocket→SslStream→SSL→ ex_data →SniCallbackData→ back toPySslSocket. The implementation properly minimizes lock contention by checkinghas_sni_callbackbefore doing the more expensive weak reference creation.
554-562: LGTM - Correctly avoids deadlock during handshake callbacks.The function properly handles the scenario where
set_contextmight be called from within an SNI callback during handshake (when the connection lock is already held). By checking the thread-localHANDSHAKE_SSL_PTRfirst, it avoids attempting to re-acquire the lock.
1 participant
Summary by CodeRabbit
New Features
pending()method to SSL sockets to check available encrypted data.Improvements
✏️ Tip: You can customize this high-level summary in your review settings.