Fix SSL error handling #6351
Fix SSL error handling #6351
Conversation
WalkthroughThe changes refactor SSL error handling by converting the 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)
✨ 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/stdlib/src/ssl.rs(3 hunks)crates/stdlib/src/ssl/compat.rs(3 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.rscrates/stdlib/src/ssl/compat.rs
🧠 Learnings (1)
📚 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 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
🧬 Code graph analysis (2)
crates/stdlib/src/ssl.rs (1)
crates/stdlib/src/ssl/compat.rs (1)
create_ssl_error_with_reason(474-500)
crates/stdlib/src/ssl/compat.rs (1)
crates/vm/src/vm/vm_new.rs (1)
new_pyobj(37-39)
🔇 Additional comments (4)
crates/stdlib/src/ssl/compat.rs (3)
475-499: SSLError construction with optionallibrarylooks correct
create_ssl_error_with_reasonnow cleanly mapslibrary: Option<&str>to a Pythonlibraryattribute (string orNone) and setsreasonfrom the provided string while building(errno, message)args once. No logic or lifetime issues spotted here.
533-538: Updated call site correctly wrapslib_strinSome
create_ssl_error_from_codesnow passesSome(lib_str)intocreate_ssl_error_with_reason, preserving the previous behavior while accommodating the newOption<&str>parameter.
547-550: Behavior change: syscall errors now surface asSSLErrorinstead ofOSError
SslError::Syscall(msg)is now converted viacreate_ssl_error_with_reason(vm, None, &msg, msg.clone()), producing anSSLError(subclass ofOSError) withlibrary=Noneandreason == message, where previouslyvm.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.
crates/stdlib/src/ssl.rs (1)
1879-1883:load_dh_paramsNO_START_LINE error wiring is consistentThe 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.
| 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.
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 Agents
In 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.
| vm, | ||
| Some("X509"), | ||
| "", | ||
| "PEM lib", | ||
| ), |
There was a problem hiding this comment.
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 Agents
In 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.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.