Skip to content

Fix ssl-vendor (OpenSSL) #7985

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

Open
joshuamegnauth54 wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −27
Open

Fix ssl-vendor (OpenSSL) #7985

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
10 changes: 2 additions & 8 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ exitcode = "1.1.2"
flame = "0.2.2"
flamer = "0.5"
flate2 = { version = "1.1.9", default-features = false }
foreign-types-shared = "0.3.1"
# Bump only when the openssl crate bumps it
foreign-types-shared = "0.1"
gethostname = "1.0.2"
getrandom = { version = "0.3", features = ["std"] }
glob = "0.3"
Expand Down
38 changes: 20 additions & 18 deletions crates/stdlib/src/openssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ mod _ssl {
LazyLock, PyMappedRwLockReadGuard, PyMutex, PyRwLock, PyRwLockReadGuard,
PyRwLockWriteGuard,
},
socket::{self, PySocket},
socket::{self, PySocket, SockWaitKind, sock_wait},
vm::{
AsObject, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
builtins::{
Expand Down Expand Up @@ -2292,7 +2292,12 @@ mod _ssl {
self.0.get_timeout().map(|d| Instant::now() + d)
}

fn select(&self, needs: SslNeeds, deadline: &SocketDeadline) -> SelectRet {
fn select(
&self,
needs: SslNeeds,
deadline: &SocketDeadline,
vm: &VirtualMachine,
) -> SelectRet {
let sock = match self.0.sock_opt() {
Some(s) => s,
None => return SelectRet::Closed,
Expand All @@ -2307,15 +2312,11 @@ mod _ssl {
Err(true) => None, // Blocking: no timeout, wait indefinitely
Err(false) => return SelectRet::Nonblocking,
};
let res = socket::sock_select(
&sock,
match needs {
SslNeeds::Read => socket::SelectKind::Read,
SslNeeds::Write => socket::SelectKind::Write,
},
timeout,
);
match res {
let wait_kind = match needs {
SslNeeds::Read => SockWaitKind::Read,
SslNeeds::Write => SockWaitKind::Write,
};
match sock_wait(&*sock, wait_kind, timeout, vm) {
Ok(true) => SelectRet::TimedOut,
_ => SelectRet::Ok,
}
Comment on lines +2319 to 2322
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Errors from sock_wait are silently discarded.

sock_wait returns PyResult<bool> and can error with EBADF, signal interrupts, or I/O failures. The wildcard _ catches both Ok(false) (correct) and Err(...) (incorrect), converting errors to SelectRet::Ok. This causes SSL operations to proceed on error and breaks signal responsiveness (e.g., Ctrl+C during handshake).

Consider propagating errors properly:

Proposed fix 
-            match sock_wait(&*sock, wait_kind, timeout, vm) {
-                Ok(true) => SelectRet::TimedOut,
-                _ => SelectRet::Ok,
-            }
+            match sock_wait(&*sock, wait_kind, timeout, vm) {
+                Ok(true) => SelectRet::TimedOut,
+                Ok(false) => SelectRet::Ok,
+                Err(_) => SelectRet::Closed, // Treat errors as socket unavailable
+            }

Note: This is a minimal fix. A more thorough solution would change select to return Result<SelectRet, PyBaseExceptionRef> and propagate the actual exception to callers.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
match sock_wait(&*sock, wait_kind, timeout, vm) {
Ok(true) => SelectRet::TimedOut,
_ => SelectRet::Ok,
}
match sock_wait(&*sock, wait_kind, timeout, vm) {
Ok(true) => SelectRet::TimedOut,
Ok(false) => SelectRet::Ok,
Err(_) => SelectRet::Closed,
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/openssl.rs` around lines 2319 - 2322, The match currently
swallows errors from sock_wait; change the code to propagate sock_wait errors
instead of treating them as SelectRet::Ok—call sock_wait(&*sock, wait_kind,
timeout, vm)? (or otherwise map Err to return the PyErr) and then match on the
resulting bool to return SelectRet::TimedOut or SelectRet::Ok so that errors
(signals, EBADF, I/O) are not discarded; update the block around sock_wait to
use the ? operator or explicit Err->return Err(...) propagation so exceptions
bubble up instead of being converted to SelectRet::Ok.

Expand All @@ -2325,13 +2326,14 @@ mod _ssl {
&self,
err: &ssl::Error,
deadline: &SocketDeadline,
vm: &VirtualMachine,
) -> (Option<SslNeeds>, SelectRet) {
let needs = match err.code() {
ssl::ErrorCode::WANT_READ => Some(SslNeeds::Read),
ssl::ErrorCode::WANT_WRITE => Some(SslNeeds::Write),
_ => None,
};
let state = needs.map_or(SelectRet::Ok, |needs| self.select(needs, deadline));
let state = needs.map_or(SelectRet::Ok, |needs| self.select(needs, deadline, vm));
(needs, state)
}
}
Expand Down Expand Up @@ -2850,7 +2852,7 @@ mod _ssl {
break;
}
// Wait briefly for peer's close_notify before retrying
match socket_stream.select(SslNeeds::Read, &deadline) {
match socket_stream.select(SslNeeds::Read, &deadline, vm) {
SelectRet::TimedOut => {
return Err(socket::timeout_error_msg(
vm,
Expand Down Expand Up @@ -2888,7 +2890,7 @@ mod _ssl {
};

// Wait on the socket
match socket_stream.select(needs, &deadline) {
match socket_stream.select(needs, &deadline, vm) {
SelectRet::TimedOut => {
let msg = if err == sys::SSL_ERROR_WANT_READ {
"The read operation timed out"
Expand Down Expand Up @@ -2984,7 +2986,7 @@ mod _ssl {
let (needs, state) = stream
.get_ref()
.expect("handshake called in bio mode; should only be called in socket mode")
.socket_needs(&err, &timeout);
.socket_needs(&err, &timeout, vm);
match state {
SelectRet::TimedOut => {
// Clean up SNI ex_data before returning error
Expand Down Expand Up @@ -3038,7 +3040,7 @@ mod _ssl {
.get_ref()
.expect("write called in bio mode; should only be called in socket mode");
let timeout = socket_ref.timeout_deadline();
let state = socket_ref.select(SslNeeds::Write, &timeout);
let state = socket_ref.select(SslNeeds::Write, &timeout, vm);
match state {
SelectRet::TimedOut => {
return Err(socket::timeout_error_msg(
Expand All @@ -3058,7 +3060,7 @@ mod _ssl {
let (needs, state) = stream
.get_ref()
.expect("write called in bio mode; should only be called in socket mode")
.socket_needs(&err, &timeout);
.socket_needs(&err, &timeout, vm);
match state {
SelectRet::TimedOut => {
return Err(socket::timeout_error_msg(
Expand Down Expand Up @@ -3229,7 +3231,7 @@ mod _ssl {
let (needs, state) = stream
.get_ref()
.expect("read called in bio mode; should only be called in socket mode")
.socket_needs(&err, &timeout);
.socket_needs(&err, &timeout, vm);
match state {
SelectRet::TimedOut => {
return Err(socket::timeout_error_msg(
Expand Down
Loading