Fix SSL error handling#6351

youknowone

Summary by CodeRabbit

Chores
- Refactored SSL/PEM error handling to improve consistency and maintainability in error categorization and reporting

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai

Walkthrough

The 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

Cohort / File(s)	Summary
SSL error function signature `crates/stdlib/src/ssl/compat.rs`	Changed `create_ssl_error_with_reason` to accept `library: Option<&str>` instead of `library: &str`; updated SSLError construction to always pass errno (1) and message; adjusted library attribute setting to handle `Some(library)` (creates Python string) or `None` (sets attribute to None).
Call site updates `crates/stdlib/src/ssl.rs`	Updated all `create_ssl_error_with_reason` invocations to wrap library arguments with `Some(...)` when providing a library name (e.g., `Some("SSL")`, `Some("X509")`, `Some("PEM")`).
SYS_CALL error path `crates/stdlib/src/ssl/compat.rs`	Changed `SslError::Syscall` handling from calling `vm.new_os_error()` to using `create_ssl_error_with_reason(vm, None, &msg, msg.clone())` for consistent error creation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Clear, cohesive refactoring pattern applied across all call sites
One straightforward function signature change with homogeneous call site updates
Potential review focus areas:
- Verify all call sites in ssl.rs have been updated with Some(...) wrapper
- Confirm the behavioral change for None library (setting attribute to None vs. string) is intentional
- Validate that the SYS_CALL case change from vm.new_os_error() to custom error creation maintains expected behavior

Possibly related PRs

RustPython/RustPython#6210: Also modifies SSL error/exception construction with explicit library/reason attribute handling
RustPython/RustPython#6228: Overlapping SSL error handling changes in ssl.rs/compat.rs with expanded error conversion and error-data usage

Suggested reviewers

coolreader18
ShaharNaveh

Poem

🐰 With library now optional and kind,
Our SSL errors leave None behind,
No more strings when context is bare,
The exception grows lighter with care!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title 'Fix SSL error handling' directly relates to the main change: updating SSL error handling signatures and call sites to pass optional library categories.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ 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

@coderabbitai review

coderabbitai

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing 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)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 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

🧠 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 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.
crates/stdlib/src/ssl.rs (1)
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.

youknowone force-pushed the ssl branch from 37d1872 to 4947e64 Compare December 8, 2025 15:58

SSLError

e26e751

youknowone force-pushed the ssl branch from 4947e64 to e26e751 Compare December 8, 2025 16:46

youknowone marked this pull request as ready for review December 8, 2025 17:46

coderabbitai Bot reviewed Dec 8, 2025

View reviewed changes

youknowone deleted the ssl branch December 8, 2025 17:55

coderabbitai Bot mentioned this pull request Dec 25, 2025

Fix SSL test_preauth_data_to_tls_server #6508

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSL error handling#6351