Fix SSL error handling #6351

youknowone · 2025-12-08T15:18:12Z

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 · 2025-12-08T15:18:29Z

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 · 2025-12-08T17:47:50Z

@coderabbitai review

coderabbitai · 2025-12-08T17:48:02Z

✅ 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.

coderabbitai · 2025-12-08T17:52:55Z

crates/stdlib/src/ssl.rs

                                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",
+                    )
                }


⚠️ 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 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.

coderabbitai · 2025-12-08T17:52:56Z

crates/stdlib/src/ssl.rs

+                        vm,
+                        Some("X509"),
+                        "",
+                        "PEM lib",
                    ),


⚠️ 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 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.

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 merged commit abfd148 into RustPython:main Dec 8, 2025
13 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix SSL error handling #6351

Fix SSL error handling #6351

Uh oh!

youknowone commented Dec 8, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 8, 2025 •

edited

Loading

Uh oh!

youknowone commented Dec 8, 2025

Uh oh!

coderabbitai bot commented Dec 8, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Dec 8, 2025

Uh oh!

coderabbitai bot Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Fix SSL error handling #6351

Fix SSL error handling #6351

Uh oh!

Conversation

youknowone commented Dec 8, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

youknowone commented Dec 8, 2025

Uh oh!

coderabbitai bot commented Dec 8, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Dec 8, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 8, 2025 •

edited

Loading