Skip to content

Fix SSL error handling #6351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Dec 8, 2025
Merged

Fix SSL error handling #6351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions crates/stdlib/src/ssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,18 +1254,29 @@ mod _ssl {
} else {
// [SSL] PEM lib
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",
)
}
Comment on lines 1256 to 1280
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})?;

Expand Down Expand Up @@ -1866,7 +1877,7 @@ mod _ssl {
// [PEM: NO_START_LINE] no start line
return Err(super::compat::SslError::create_ssl_error_with_reason(
vm,
"PEM",
Some("PEM"),
"NO_START_LINE",
"[PEM: NO_START_LINE] no start line",
));
Expand Down Expand Up @@ -2127,7 +2138,10 @@ mod _ssl {
}
// PEM parsing errors
_ => super::compat::SslError::create_ssl_error_with_reason(
vm, "X509", "", "PEM lib",
vm,
Some("X509"),
"",
"PEM lib",
),
Comment on lines +2141 to 2145
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
})
Expand Down
32 changes: 23 additions & 9 deletions crates/stdlib/src/ssl/compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,19 +473,25 @@ impl SslError {
/// If attribute setting fails (extremely rare), returns the exception without attributes
pub(super) fn create_ssl_error_with_reason(
vm: &VirtualMachine,
library: &str,
library: Option<&str>,
reason: &str,
message: impl Into<String>,
) -> PyBaseExceptionRef {
let exc = vm.new_exception_msg(PySSLError::class(&vm.ctx).to_owned(), message.into());
let msg = message.into();
// SSLError args should be (errno, message) format
// FIXME: Use 1 as generic SSL error code
let exc = vm.new_exception(
PySSLError::class(&vm.ctx).to_owned(),
vec![vm.new_pyobj(1i32), vm.new_pyobj(msg)],
);

// Set library and reason attributes
// Ignore errors as they're extremely rare (e.g., out of memory)
let _ = exc.as_object().set_attr(
"library",
vm.ctx.new_str(library).as_object().to_owned(),
vm,
);
let library_obj = match library {
Some(lib) => vm.ctx.new_str(lib).as_object().to_owned(),
None => vm.ctx.none(),
};
let _ = exc.as_object().set_attr("library", library_obj, vm);
let _ =
exc.as_object()
.set_attr("reason", vm.ctx.new_str(reason).as_object().to_owned(), vm);
Expand Down Expand Up @@ -524,7 +530,12 @@ impl SslError {
.unwrap_or("UNKNOWN");

// Delegate to create_ssl_error_with_reason for actual exception creation
Self::create_ssl_error_with_reason(vm, lib_str, reason_str, format!("[SSL] {reason_str}"))
Self::create_ssl_error_with_reason(
vm,
Some(lib_str),
reason_str,
format!("[SSL] {reason_str}"),
)
}

/// Convert to Python exception
Expand All @@ -533,7 +544,10 @@ impl SslError {
SslError::WantRead => create_ssl_want_read_error(vm),
SslError::WantWrite => create_ssl_want_write_error(vm),
SslError::Timeout(msg) => timeout_error_msg(vm, msg),
SslError::Syscall(msg) => vm.new_os_error(msg),
SslError::Syscall(msg) => {
// Create SSLError with library=None for syscall errors during SSL operations
Self::create_ssl_error_with_reason(vm, None, &msg, msg.clone())
}
SslError::Ssl(msg) => vm.new_exception_msg(
PySSLError::class(&vm.ctx).to_owned(),
format!("SSL error: {msg}"),
Expand Down
Loading