Fix SSL read over-consuming TCP data

youknowone · youknowone · commit 34cf3ddac722 · 2026-03-13T14:38:03.000+09:00
Use single-record reading (recv_one_tls_record) for all SSL
reads, not just handshake. This prevents rustls from eagerly
consuming close_notify alongside application data, which left
the TCP buffer empty and caused select()-based servers to miss
readability and time out.

Also fix recv_one_tls_record to return Eof (not WantRead) when
peek returns empty bytes, since empty peek means the peer has
closed the TCP connection.
diff --git a/Lib/test/test_ftplib.py b/Lib/test/test_ftplib.py
@@ -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):
@@ -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."""
@@ -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)
@@ -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)
diff --git a/crates/stdlib/src/ssl.rs b/crates/stdlib/src/ssl.rs
@@ -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)
                     }
                 }
diff --git a/crates/stdlib/src/ssl/compat.rs b/crates/stdlib/src/ssl/compat.rs
@@ -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,
@@ -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 {
@@ -1272,7 +1274,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,
@@ -1716,17 +1718,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(socket, vm)?;
 
                 let bytes_read = data
                     .clone()
@@ -2128,28 +2121,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(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));
             }
         };
 

Original file line number	Diff line number	Diff line change
`@@ -3629,18 +3629,17 @@ mod _ssl {`
`3629`	`3629`	`return return_data(buf, &buffer, vm);`
`3630`	`3630`	`}`
`3631`	`3631`	`}`
`3632`		`- // Clean closure with close_notify`
`3633`		`- // CPython behavior depends on whether we've sent our close_notify:`
`3634`		`- // - If we've already sent close_notify (unwrap was called): raise SSLZeroReturnError`
`3635`		`- // - If we haven't sent close_notify yet: return empty bytes`
	`3632`	`+ // Clean closure via close_notify from peer.`
	`3633`	`+ // If we already sent close_notify (unwrap was called),`
	`3634`	`+ // raise SSLZeroReturnError (bidirectional shutdown).`
	`3635`	`+ // Otherwise return empty bytes, which callers (asyncore,`
	`3636`	`+ // asyncio sslproto) interpret as EOF.`
`3636`	`3637`	`let our_shutdown_state = *self.shutdown_state.lock();`
`3637`	`3638`	`if our_shutdown_state == ShutdownState::SentCloseNotify`
`3638`	`3639`	`\|\| our_shutdown_state == ShutdownState::Completed`
`3639`	`3640`	`{`
`3640`		`- // We already sent close_notify, now receiving peer's → SSLZeroReturnError`
`3641`	`3641`	`Err(create_ssl_zero_return_error(vm).upcast())`
`3642`	`3642`	`} else {`
`3643`		`- // We haven't sent close_notify yet → return empty bytes`
`3644`	`3643`	`return_data(vec![], &buffer, vm)`
`3645`	`3644`	`}`
`3646`	`3645`	`}`