Skip to content

Fix SSL read over-consuming TCP data and ZeroReturn handling #7418

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
Mar 13, 2026
Merged

Fix SSL read over-consuming TCP data and ZeroReturn handling #7418

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
4 changes: 0 additions & 4 deletions Lib/test/test_ftplib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,6 @@ def retr():
retr()


@unittest.skip("TODO: RUSTPYTHON; SSL + asyncore has problem")
@skipUnless(ssl, "SSL not available")
@requires_subprocess()
class TestTLS_FTPClassMixin(TestFTPClass):
Expand All @@ -922,7 +921,6 @@ def setUp(self, encoding=DEFAULT_ENCODING):


@skipUnless(ssl, "SSL not available")
@unittest.skip("TODO: RUSTPYTHON; SSL + asyncore has problem")
@requires_subprocess()
class TestTLS_FTPClass(TestCase):
"""Specific TLS_FTP class tests."""
Expand Down Expand Up @@ -971,7 +969,6 @@ def test_data_connection(self):
LIST_DATA.encode(self.client.encoding))
self.assertEqual(self.client.voidresp(), "226 transfer complete")

@unittest.skip('TODO: RUSTPYTHON flaky TimeoutError')
def test_login(self):
# login() is supposed to implicitly secure the control connection
self.assertNotIsInstance(self.client.sock, ssl.SSLSocket)
Expand All @@ -984,7 +981,6 @@ def test_auth_issued_twice(self):
self.client.auth()
self.assertRaises(ValueError, self.client.auth)

@unittest.skip('TODO: RUSTPYTHON flaky TimeoutError')
def test_context(self):
self.client.quit()
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
Expand Down
11 changes: 5 additions & 6 deletions crates/stdlib/src/ssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3629,18 +3629,17 @@ mod _ssl {
return return_data(buf, &buffer, vm);
}
}
// Clean closure with close_notify
// CPython behavior depends on whether we've sent our close_notify:
// - If we've already sent close_notify (unwrap was called): raise SSLZeroReturnError
// - If we haven't sent close_notify yet: return empty bytes
// Clean closure via close_notify from peer.
// If we already sent close_notify (unwrap was called),
// raise SSLZeroReturnError (bidirectional shutdown).
// Otherwise return empty bytes, which callers (asyncore,
// asyncio sslproto) interpret as EOF.
let our_shutdown_state = *self.shutdown_state.lock();
if our_shutdown_state == ShutdownState::SentCloseNotify
|| our_shutdown_state == ShutdownState::Completed
{
// We already sent close_notify, now receiving peer's → SSLZeroReturnError
Err(create_ssl_zero_return_error(vm).upcast())
} else {
// We haven't sent close_notify yet → return empty bytes
return_data(vec![], &buffer, vm)
}
}
Expand Down
108 changes: 64 additions & 44 deletions crates/stdlib/src/ssl/compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,21 +1164,20 @@ fn handshake_write_loop(
/// TLS record header size (content_type + version + length).
const TLS_RECORD_HEADER_SIZE: usize = 5;

/// Determine how many bytes to read from the socket during a TLS handshake.
/// Read exactly one TLS record from the TCP socket.
///
/// OpenSSL reads one TLS record at a time (no read-ahead by default).
/// Rustls, however, consumes all available TCP data when fed via read_tls().
/// If application data arrives simultaneously with the final handshake record,
/// the eager read drains the TCP buffer, leaving the app data in rustls's
/// internal buffer where select() cannot see it. This causes asyncore-based
/// servers (which rely on select() for readability) to miss the data and the
/// peer times out.
/// If a close_notify or other control record arrives alongside application
/// data, the eager read drains the TCP buffer, leaving the control record in
/// rustls's internal buffer where select() cannot see it. This causes
/// asyncore-based servers (which rely on select() for readability) to miss
/// the data and the peer times out.
///
/// Fix: peek at the TCP buffer to find the first complete TLS record boundary
/// and recv() only that many bytes. Any remaining data (including application
/// data that piggybacked on the same TCP segment) stays in the kernel buffer
/// and remains visible to select().
fn handshake_recv_one_record(socket: &PySSLSocket, vm: &VirtualMachine) -> SslResult<PyObjectRef> {
/// and recv() only that many bytes. Any remaining data stays in the kernel
/// buffer and remains visible to select().
fn recv_one_tls_record(socket: &PySSLSocket, vm: &VirtualMachine) -> SslResult<PyObjectRef> {
// Peek at what is available without consuming it.
let peeked_obj = match socket.sock_peek(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
Ok(d) => d,
Expand All @@ -1195,7 +1194,10 @@ fn handshake_recv_one_record(socket: &PySSLSocket, vm: &VirtualMachine) -> SslRe
let peeked_bytes = peeked.borrow_buf();

if peeked_bytes.is_empty() {
return Err(SslError::WantRead);
// Empty peek means the peer has closed the TCP connection (FIN).
// Non-blocking sockets would have returned EAGAIN/EWOULDBLOCK
// (caught above as WantRead), so empty bytes here always means EOF.
return Err(SslError::Eof);
}

if peeked_bytes.len() < TLS_RECORD_HEADER_SIZE {
Expand Down Expand Up @@ -1238,6 +1240,34 @@ fn handshake_recv_one_record(socket: &PySSLSocket, vm: &VirtualMachine) -> SslRe
})
}

/// Read a single TLS record for post-handshake I/O while preserving the
/// SSL-vs-socket error precedence from the old sock_recv() path.
fn recv_one_tls_record_for_data(
conn: &mut TlsConnection,
socket: &PySSLSocket,
vm: &VirtualMachine,
) -> SslResult<PyObjectRef> {
match recv_one_tls_record(socket, vm) {
Ok(data) => Ok(data),
Err(SslError::Eof) => {
if let Err(rustls_err) = conn.process_new_packets() {
return Err(SslError::from_rustls(rustls_err));
}
Ok(vm.ctx.new_bytes(vec![]).into())
}
Err(SslError::Py(e)) => {
if let Err(rustls_err) = conn.process_new_packets() {
return Err(SslError::from_rustls(rustls_err));
}
if is_connection_closed_error(&e, vm) {
return Err(SslError::Eof);
}
Err(SslError::Py(e))
}
Err(e) => Err(e),
}
}

fn handshake_read_data(
conn: &mut TlsConnection,
socket: &PySSLSocket,
Expand Down Expand Up @@ -1272,7 +1302,7 @@ fn handshake_read_data(
// record. This matches OpenSSL's default (no read-ahead) behaviour
// and keeps remaining data in the kernel buffer where select() can
// detect it.
handshake_recv_one_record(socket, vm)?
recv_one_tls_record(socket, vm)?
} else {
match socket.sock_recv(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
Ok(d) => d,
Expand Down Expand Up @@ -1716,17 +1746,8 @@ pub(super) fn ssl_read(
}
// Blocking socket or socket with timeout: try to read more data from socket.
// Even though rustls says it doesn't want to read, more TLS records may arrive.
// This handles the case where rustls processed all buffered TLS records but
// more data is coming over the network.
let data = match socket.sock_recv(2048, vm) {
Ok(data) => data,
Err(e) => {
if is_connection_closed_error(&e, vm) {
return Err(SslError::Eof);
}
return Err(SslError::Py(e));
}
};
// Use single-record reading to avoid consuming close_notify alongside data.
let data = recv_one_tls_record_for_data(conn, socket, vm)?;

let bytes_read = data
.clone()
Expand Down Expand Up @@ -2128,28 +2149,27 @@ fn ssl_ensure_data_available(
// else: non-blocking socket (timeout=0) or blocking socket (timeout=None) - skip select
}

let data = match socket.sock_recv(2048, vm) {
Ok(data) => data,
Err(e) => {
if is_blocking_io_error(&e, vm) {
return Err(SslError::WantRead);
}
// Before returning socket error, check if rustls already has a queued TLS alert
// This mirrors CPython/OpenSSL behavior: SSL errors take precedence over socket errors
// On Windows, TCP RST may arrive before we read the alert, but rustls may have
// already received and buffered the alert from a previous read
if let Err(rustls_err) = conn.process_new_packets() {
return Err(SslError::from_rustls(rustls_err));
}
// In SSL context, connection closed errors (ECONNABORTED, ECONNRESET) indicate
// unexpected connection termination - the peer closed without proper TLS shutdown.
// This is semantically equivalent to "EOF occurred in violation of protocol"
// because no close_notify alert was received.
// On Windows, TCP RST can arrive before we read the TLS alert, causing these errors.
if is_connection_closed_error(&e, vm) {
return Err(SslError::Eof);
// Read one TLS record at a time for non-BIO sockets (matching
// OpenSSL's default no-read-ahead behaviour). This prevents
// consuming a close_notify that arrives alongside application data,
// keeping it in the kernel buffer where select() can detect it.
let data = if !is_bio {
recv_one_tls_record_for_data(conn, socket, vm)?
} else {
match socket.sock_recv(2048, vm) {
Ok(data) => data,
Err(e) => {
if is_blocking_io_error(&e, vm) {
return Err(SslError::WantRead);
}
if let Err(rustls_err) = conn.process_new_packets() {
return Err(SslError::from_rustls(rustls_err));
}
if is_connection_closed_error(&e, vm) {
return Err(SslError::Eof);
}
return Err(SslError::Py(e));
}
return Err(SslError::Py(e));
}
};

Expand Down
Loading