crates/stdlib/src/ssl/compat.rs (1)

1196-1198: ⚠️ Potential issue | 🔴 Critical

Return empty bytes on EOF instead of WantRead at line 1196.

When sock_peek() returns empty bytes, it signals TCP EOF (peer closure). Currently line 1196–1198 converts this to SslError::WantRead, which bypasses the EOF detection logic in ssl_read_tls_records() (lines 1985–2012). The function explicitly handles empty socket-mode bytes by calling process_new_packets() to distinguish clean shutdown (ZeroReturn) from ragged EOF (Eof). Returning WantRead instead prevents that logic from executing and can cause retry loops or incorrect error reporting. Callers at lines 1719 and 2125 also expect to receive bytes and check bytes_read == 0 for EOF detection; the current path short-circuits this.

Return Ok(vm.ctx.new_bytes(vec![]).into()) to allow empty bytes to flow downstream for proper EOF classification.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/ssl/compat.rs` around lines 1196 - 1198, When sock_peek()
returns an empty Vec, don't convert that to SslError::WantRead; instead return
an empty-byte VM object so downstream EOF logic can run. Replace the branch in
compat::sock_peek handling that currently does "return Err(SslError::WantRead)"
with returning Ok(vm.ctx.new_bytes(vec![]).into()) (so callers like the code at
ssl_read_tls_records() and callers at the sites that check bytes_read == 0
receive empty bytes). This ensures ssl_read_tls_records() can call
process_new_packets() and correctly distinguish ZeroReturn vs Eof rather than
short-circuiting with WantRead.

crates/stdlib/src/ssl/compat.rs (2)

1184-1189: ⚠️ Potential issue | 🟠 Major

Keep EOF/reset normalization in the new helper.

recv_one_tls_record() now only special-cases BlockingIOError. On the non-BIO paths that switched to this helper, ConnectionResetError / ConnectionAbortedError from either the peek or the recv will bubble out as raw Python exceptions instead of SslError::Eof, which regresses the SSL EOF path and suppress_ragged_eofs.

Proposed fix

         Err(e) => {
             if is_blocking_io_error(&e, vm) {
                 return Err(SslError::WantRead);
             }
+            if is_connection_closed_error(&e, vm) {
+                return Err(SslError::Eof);
+            }
             return Err(SslError::Py(e));
         }
     };
@@
     socket.sock_recv(recv_size, vm).map_err(|e| {
         if is_blocking_io_error(&e, vm) {
             SslError::WantRead
+        } else if is_connection_closed_error(&e, vm) {
+            SslError::Eof
         } else {
             SslError::Py(e)
         }
     })

If the old conn.process_new_packets() check is still needed before classifying EOF, keep that logic at the non-BIO call sites. As per coding guidelines, "Follow Rust best practices for error handling and memory management".

Also applies to: 1234-1240

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/ssl/compat.rs` around lines 1184 - 1189, The new helper
recv_one_tls_record() currently only maps BlockingIOError to SslError::WantRead
and lets ConnectionResetError/ConnectionAbortedError escape as raw Python
exceptions, breaking EOF normalization and suppress_ragged_eofs; update
recv_one_tls_record() (and the duplicate logic around the other occurrence at
~1234-1240) to map connection-reset/aborted errors to SslError::Eof (as the
previous code did), and if the original conn.process_new_packets() check is
required for correct EOF classification on non-BIO paths, reintroduce that check
at the non-BIO call sites before calling recv_one_tls_record() so EOF/reset are
normalized to SslError::Eof instead of propagating raw Python errors.

---

1182-1228: ⚠️ Potential issue | 🟠 Major

Peek a full wire-format record; 16 KiB is insufficient for both TLS 1.2 and TLS 1.3.

SSL3_RT_MAX_PLAIN_LENGTH (16384) is a plaintext bound, not a wire-format record bound. TLS 1.3 records can reach 5 + 16640 = 16645 bytes (header + ciphertext expansion). TLS 1.2 records can reach 5 + 18432 = 18437 bytes. The current 16384-byte peek may capture only a prefix of a valid ciphertext record, causing the remaining bytes to be misinterpreted as a new record header on the next call.

The proposed fix of adding 256 bytes (16645 total) correctly handles TLS 1.3 but is insufficient for TLS 1.2 support, which requires approximately 18437 bytes. Increase the peek bound to cover the maximum record size for the configured TLS version(s), or read the 5-byte header first and then peek the remaining record_length bytes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/ssl/compat.rs` around lines 1182 - 1228, The current peek
uses SSL3_RT_MAX_PLAIN_LENGTH (16384) which is only plaintext size and can
truncate TLS wire-format records (TLS1.3 up to ~16645 bytes and TLS1.2 up to
~18437 bytes); update the logic in the function using socket.sock_peek and
SSL3_RT_MAX_PLAIN_LENGTH so we never mis-parse a partial record: first call
sock_peek for exactly TLS_RECORD_HEADER_SIZE (5) to obtain the header, parse
record_body_len from bytes [3..5] (as done with record_body_len =
u16::from_be_bytes(...)), then call sock_peek again for TLS_RECORD_HEADER_SIZE +
record_body_len (or ensure you peek that many bytes) before deciding recv_size;
keep the same WantRead/Eof/Py error handling around sock_peek and
socket.sock_recv.

crates/stdlib/src/ssl/compat.rs (1)

1167-1241: ⚠️ Potential issue | 🔴 Critical

The generalized single-record helper can still split a valid TLS record mid-stream.

recv_one_tls_record() still peeks only SSL3_RT_MAX_PLAIN_LENGTH, which is just the record body cap. A legal 16 KiB record is already 16,389 bytes on the wire before any ciphertext overhead, so this logic will treat it as “incomplete”, consume only the first 16,384 bytes, and leave the next call starting in the middle of that same record. Once this PR routes normal ssl_read() / ssl_ensure_data_available() traffic through the helper, the next header parse is reading payload bytes rather than a real TLS header and the stream state is corrupted. Please size the boundary detection for the maximum wire-record length rustls accepts before reusing this helper outside the handshake.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/stdlib/src/ssl/compat.rs` around lines 1167 - 1241,
recv_one_tls_record currently peeks only SSL3_RT_MAX_PLAIN_LENGTH which can
split a legal max-size TLS record; change the initial peek size passed to
socket.sock_peek in recv_one_tls_record to the maximum possible on‑the‑wire TLS
record length rustls accepts (i.e. TLS_RECORD_HEADER_SIZE +
SSL3_RT_MAX_PLAIN_LENGTH plus any max cipher/nonce/tag overhead your TLS
implementation permits) instead of SSL3_RT_MAX_PLAIN_LENGTH so the header/length
parsing will always see a full record boundary; update any associated constant
(introduce something like MAX_WIRE_RECORD_LEN or TLS_MAX_ON_WIRE_SIZE) and use
that symbol in the peek and related logic so the helper never consumes only the
first fragment of a legal record.