Fix SSL error handling - #6351
Conversation
Sorry, something went wrong.
WalkthroughThe changes refactor SSL error handling by converting the create_ssl_error_with_reason function's library parameter from a required string to an optional Option<&str>. All call sites are updated to pass Some(library) when available or None otherwise, and the resulting exception now correctly handles cases where no library context applies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
✅ Actions performed
Review triggered.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between a3d638a and e26e751.
📒 Files selected for processing (2)- crates/stdlib/src/ssl.rs (3 hunks)
- crates/stdlib/src/ssl/compat.rs (3 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.rs
- crates/stdlib/src/ssl/compat.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 Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
- crates/stdlib/src/ssl/compat.rs
crates/stdlib/src/ssl/compat.rs (1)crates/stdlib/src/ssl/compat.rs (1)
- create_ssl_error_with_reason (474-500)
crates/vm/src/vm/vm_new.rs (1)🔇 Additional comments (4)
- new_pyobj (37-39)
crates/stdlib/src/ssl/compat.rs (3)crates/stdlib/src/ssl.rs (1)475-499: SSLError construction with optional library looks correct
create_ssl_error_with_reason now cleanly maps library: Option<&str> to a Python library attribute (string or None) and sets reason from the provided string while building (errno, message) args once. No logic or lifetime issues spotted here.
533-538: Updated call site correctly wraps lib_str in Some
create_ssl_error_from_codes now passes Some(lib_str) into create_ssl_error_with_reason, preserving the previous behavior while accommodating the new Option<&str> parameter.
547-550: Behavior change: syscall errors now surface as SSLError instead of OSError
SslError::Syscall(msg) is now converted via create_ssl_error_with_reason(vm, None, &msg, msg.clone()), producing an SSLError (subclass of OSError) with library=None and reason == message, where previously vm.new_os_error(msg) was used. This is likely intentional for consistency with other SSL paths, but it does change the concrete exception type and args layout seen by Python code.
Please re-run the ssl-related test suite (especially any tests asserting syscall error types/attributes) to confirm this behavioral change matches the intended CPython compatibility.
1879-1883: load_dh_params NO_START_LINE error wiring is consistent
The NO_START_LINE case now calls:
super::compat::SslError::create_ssl_error_with_reason( vm, Some("PEM"), "NO_START_LINE", "[PEM: NO_START_LINE] no start line", )which matches the helper’s expected (library, reason, message) usage and is consistent with CPython-style error strings for this condition.
Sorry, something went wrong.
| super::compat::SslError::create_ssl_error_with_reason( | ||
| vm, "SSL", "", "PEM lib", | ||
| vm, | ||
| Some("SSL"), | ||
| "", | ||
| "PEM lib", | ||
| ) | ||
| } | ||
| } | ||
| // PEM parsing errors - [SSL] PEM lib | ||
| _ => super::compat::SslError::create_ssl_error_with_reason( | ||
| vm, "SSL", "", "PEM lib", | ||
| vm, | ||
| Some("SSL"), | ||
| "", | ||
| "PEM lib", | ||
| ), | ||
| } | ||
| } else { | ||
| // Unknown error type - [SSL] PEM lib | ||
| super::compat::SslError::create_ssl_error_with_reason(vm, "SSL", "", "PEM lib") | ||
| super::compat::SslError::create_ssl_error_with_reason( | ||
| vm, | ||
| Some("SSL"), | ||
| "", | ||
| "PEM lib", | ||
| ) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
PEM error calls pass empty reason string; swap reason/message usage
In load_cert_chain, the non-password PEM error paths build the SSL error as:
super::compat::SslError::create_ssl_error_with_reason(
vm,
Some("SSL"),
"",
"PEM lib",
)(and similarly in the _ and non-io::Error branches).
Per create_ssl_error_with_reason’s contract and other call sites, reason should hold a short reason like "PEM lib", while message is the main display string. Using an empty reason here means exc.reason is not informative and is inconsistent with comments like // [SSL] PEM lib and with usages such as the NO_START_LINE path.
Recommend at least giving reason a non-empty value and, ideally, including the [SSL] … prefix in the message, e.g.:
- 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",
+ "[SSL] PEM lib",
+ )Apply the same pattern to the _ arm and the else branch so that all PEM-lib errors expose a consistent and non-empty reason attribute.
🤖 Prompt for AI AgentsIn crates/stdlib/src/ssl.rs around lines 1256 to 1280, the PEM error branches
call create_ssl_error_with_reason(vm, Some("SSL"), "", "PEM lib") leaving the
reason empty; change these calls so reason receives a short non-empty identifier
(e.g. "PEM lib") and message receives the full display string (e.g. "[SSL] PEM
lib" or include context), i.e. swap the empty-string reason and the current
message value so exc.reason is informative; apply the same change to the `_` arm
and the final `else` branch so all PEM parsing error paths consistently pass a
non-empty reason and an appropriate message.
Sorry, something went wrong.
| vm, | ||
| Some("X509"), | ||
| "", | ||
| "PEM lib", | ||
| ), |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
X509 PEM parsing errors also use empty reason; align with helper semantics
In load_certs_from_file_helper, PEM parsing errors are converted via:
_ => super::compat::SslError::create_ssl_error_with_reason(
vm,
Some("X509"),
"",
"PEM lib",
),This shares the same problem as the PEM-lib errors above: reason is "" while message is "PEM lib", contrary to the helper’s documentation and other usages. This makes exc.reason effectively unusable and diverges from the intended CPython-style (library, reason) pairing.
Consider updating to something like:
- _ => 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",
+ "[X509] PEM lib",
+ ),so that callers relying on exc.reason and exc.library see consistent values.
🤖 Prompt for AI AgentsIn crates/stdlib/src/ssl.rs around lines 2141-2145, PEM parsing errors currently
call create_ssl_error_with_reason(vm, Some("X509"), "", "PEM lib") which leaves
exc.reason empty; change the call to provide the library as Some("X509"), set
reason to "PEM lib" and message to "" (i.e., create_ssl_error_with_reason(vm,
Some("X509"), "PEM lib", "")) so exc.reason and exc.library follow the helper’s
intended (library, reason) semantics.
Sorry, something went wrong.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.