Fix SSL handshake over-reading in STARTTLS by youknowone · Pull Request #7417 · RustPython/RustPython

youknowone · 2026-03-13T01:15:31Z

During STARTTLS handshake, sock_recv(16KB) could consume application data that arrived alongside handshake records. The consumed data ended up in rustls's internal buffer where select() could not detect it, causing asyncore-based servers to miss readable events and the peer to time out.

Use MSG_PEEK to find the TLS record boundary, then recv() only one complete record. Remaining data stays in the kernel TCP buffer, visible to select(). This matches OpenSSL's default no-read-ahead behaviour.

Fixes flaky test_poplib (TestPOP3_TLSClass) failures.

Summary by CodeRabbit

Bug Fixes
- Enhanced TLS handshake record processing to correctly handle TLS record boundaries, preventing premature buffer draining and potential loss of application data in socket-based connections.
- Improved timeout error reporting consistency across SSL, socket, and system API operations.

coderabbitai · 2026-03-13T01:15:41Z

📝 Walkthrough

Walkthrough

This change centralizes timeout error construction across SSL and OpenSSL modules using a shared helper function, and introduces TLS record boundary-aware reading in the SSL handshake path to prevent premature buffer consumption and potential application data loss.

Changes

Cohort / File(s)	Summary
SSL/OpenSSL timeout error centralization `crates/stdlib/src/openssl.rs`, `crates/stdlib/src/ssl.rs`	Replaced direct exception construction via `vm.new_exception_msg()` with calls to `socket::timeout_error_msg(vm, "...").upcast()` at multiple timeout fallback points in shutdown, read, and write paths.
TLS record boundary-aware handshake `crates/stdlib/src/ssl/compat.rs`	Introduced `TLS_RECORD_HEADER_SIZE` constant and new `handshake_recv_one_record()` helper to peek at TCP buffer, parse TLS record boundaries, and read only complete records in non-BIO socket mode, preventing kernel buffer overruns during handshake.
Windows API and VM error handling `crates/vm/src/stdlib/_winapi.rs`, `crates/vm/src/vm/vm_new.rs`	Updated `WAIT_TIMEOUT` error handling to emit OS subtype timeout errors instead of generic timeout exceptions; updated debug assertion message in `VirtualMachine::new_exception()` to guide toward `vm.new_os_subtype_error()` for OSError subtypes.

Sequence Diagram(s)

sequenceDiagram
    participant Socket as TCP Socket
    participant Peek as Peek Buffer
    participant Parser as TLS Parser
    participant Reader as Record Reader
    participant PyVM as Python VM

    Socket->>Peek: peek(header_size=5)
    Peek-->>Parser: raw_bytes[0:5]
    Parser->>Parser: parse version + length
    Parser-->>Reader: total_record_size
    Reader->>Socket: recv(total_record_size)
    Socket-->>Reader: complete_record_bytes
    Reader->>PyVM: return exact bytes read

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

fix ssl MSG_PEEK #6892: Modifies SSL socket read/handshake paths with TLS record-boundary aware peeking, directly related to the ssl/compat.rs record-aware reading mechanism.
Fix openssl build and shared ssl/error.rs #6456: Centralizes error construction paths in SSL/OpenSSL modules with helper functions, matching the timeout error refactoring approach.
Share more ssl consts and fix openssl #6462: Reworks SSL handshake, read, and shutdown paths including timeout and error handling centralization in the same modules.

Poem

🐰 Hops through buffers with careful grace,
TLS records each in their place,
Timeouts now speak with one true voice,
No data lost—a safer choice! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix SSL handshake over-reading in STARTTLS' accurately summarizes the main change: addressing a bug where SSL handshakes over-read data in STARTTLS scenarios. It is concise, clear, and directly relates to the primary technical issue being fixed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📝 Coding Plan

Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

During STARTTLS handshake, sock_recv(16KB) could consume application data that arrived alongside handshake records. The consumed data ended up in rustls's internal buffer where select() could not detect it, causing asyncore-based servers to miss readable events and the peer to time out. Use MSG_PEEK to find the TLS record boundary, then recv() only one complete record. Remaining data stays in the kernel TCP buffer, visible to select(). This matches OpenSSL's default no-read-ahead behaviour. Fixes flaky test_poplib (TestPOP3_TLSClass) failures.

coderabbitai

🧹 Nitpick comments (1)

crates/stdlib/src/openssl.rs (1)

2862-2866: Minor style inconsistency: prefer .to_owned() for consistency.

The rest of the file uses .to_owned() for string literal conversions (e.g., lines 3000, 3051, 3242), while this change uses .to_string(). Both are functionally equivalent, but using .to_owned() would maintain consistency.

🔧 Suggested fix for consistency

-                            return Err(socket::timeout_error_msg(
-                                vm,
-                                "The read operation timed out".to_string(),
-                            )
-                            .upcast());
+                            return Err(socket::timeout_error_msg(
+                                vm,
+                                "The read operation timed out".to_owned(),
+                            )
+                            .upcast());

🤖 Prompt for AI Agents

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

In `@crates/stdlib/src/openssl.rs` around lines 2862 - 2866, Replace the
.to_string() call on the literal inside the timeout error return with
.to_owned() for consistency with the rest of openssl.rs; specifically update the
call in the expression that constructs the socket::timeout_error_msg (the return
Err(... upcast()) statement that currently uses "The read operation timed
out".to_string()) to use "The read operation timed out".to_owned().

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@crates/stdlib/src/openssl.rs`:
- Around line 2862-2866: Replace the .to_string() call on the literal inside the
timeout error return with .to_owned() for consistency with the rest of
openssl.rs; specifically update the call in the expression that constructs the
socket::timeout_error_msg (the return Err(... upcast()) statement that currently
uses "The read operation timed out".to_string()) to use "The read operation
timed out".to_owned().

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 37b93a80-4151-4eba-8f46-f3bf90dbb5b8

📥 Commits

Reviewing files that changed from the base of the PR and between 83e3785 and 2602df0.

📒 Files selected for processing (5)

crates/stdlib/src/openssl.rs
crates/stdlib/src/ssl.rs
crates/stdlib/src/ssl/compat.rs
crates/vm/src/stdlib/_winapi.rs
crates/vm/src/vm/vm_new.rs

youknowone force-pushed the ssl-handshake branch from 083f2bf to 1fe003d Compare March 13, 2026 01:16

youknowone force-pushed the ssl-handshake branch from 1fe003d to 2602df0 Compare March 13, 2026 01:24

youknowone marked this pull request as ready for review March 13, 2026 02:34

coderabbitai bot reviewed Mar 13, 2026

View reviewed changes

youknowone merged commit 7c0981b into RustPython:main Mar 13, 2026
15 checks passed

youknowone deleted the ssl-handshake branch March 13, 2026 02:46

coderabbitai bot mentioned this pull request Mar 13, 2026

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SSL handshake over-reading in STARTTLS#7417

Fix SSL handshake over-reading in STARTTLS#7417
youknowone merged 1 commit intoRustPython:mainfrom
youknowone:ssl-handshake

youknowone commented Mar 13, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Mar 13, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

youknowone commented Mar 13, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

youknowone commented Mar 13, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Mar 13, 2026 •

edited

Loading