Fix SSL read over-consuming TCP data and ZeroReturn handling

youknowone · youknowone · commit 1d749eb149df · 2026-03-13T11:47:00.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 ZeroReturn to raise SSLEOFError when shutdown has not
started, so suppress_ragged_eofs=True can suppress it properly.
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,19 +3629,22 @@ 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.
+                    // The handling depends on our shutdown state:
+                    // - If we already sent close_notify (unwrap was called):
+                    //   Raise SSLZeroReturnError (bidirectional shutdown).
+                    // - If we haven't sent close_notify yet:
+                    //   Raise SSLEOFError so SSLSocket.read() can suppress it
+                    //   via suppress_ragged_eofs=True (returning b'').
+                    //   This also lets asyncore's SSLConnection.recv() catch
+                    //   SSL_ERROR_EOF and call handle_close().
                     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)
+                        Err(create_ssl_eof_error(vm).upcast())
                     }
                 }
                 Err(crate::ssl::compat::SslError::WantRead) => {
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,
@@ -1272,7 +1271,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 +1715,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 +2118,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));
             }
         };
 
