Skip to content

Rustls integration improvements#7946

Merged
youknowone merged 11 commits into
RustPython:mainfrom
im-0:fix-rustls
May 22, 2026
Merged

Rustls integration improvements#7946
youknowone merged 11 commits into
RustPython:mainfrom
im-0:fix-rustls

Conversation

@im-0
Copy link
Copy Markdown
Contributor

@im-0 im-0 commented May 21, 2026

Primary goal of this PR is to fix #7891

Changes:

  • Do not call import socket on each send()/recv() when using rustls
  • Implement reading of at most one TLS record from socket. Previous algorithm didn't take into account that recv() may return less data than requested even for blocking sockets
  • Remove special handling of rustls "buffer full" errors. One of that checks is a root cause of a hang with 100% CPU core utilization. This PR also adds a regression test for this particular problem.
  • Replace own TlsConnection with rustls::Connection
  • Dead code removed
  • rustls errors are no longer presented as as OSError(0, "Success")
  • Infinite loop "detection" removed from rustls handshake code. TLS handshake cannot be infinite. Any infinite loop here is a serious bug in implementation and should be fixed properly. Also, this code triggered in some cases (very short reads) with misleading ssl_error.SSLWantReadError: The operation did not complete (read).
  • Deduplicated verify flags / record-size constants
  • Used slightly larger "max encrypted TLS record length" from OpenSSL headers
  • Socket wait fixes:
    • socket_wait() called from TLS glue code now allows threads (vm.allow_threads())
    • socket_wait() called from TLS glue code now properly handles EINTR on *nix
    • select()/poll() error conditions are checked
    • poll() is used on *nix so socket descriptor values are not limited to <FD_SETSIZE

Please note that this is no way near complete and current implementation probably still has a ton of bugs.

Comments

(coderabbit) Looking up send/recv on socket.socket hard-wires base-class dispatch and skips overrides on socket subclasses or instance-attached methods. That changes I/O behavior for wrapped custom sockets compared to resolving the methods from args.sock.

It is a current behavior and it is not that easy to change. I tried to naively replace it with calls to sock.send()/sock.recv() and it resulted in deadlocks.

Summary by CodeRabbit

  • Bug Fixes

    • More reliable TLS handshakes and close-notify behavior; improved handling of short/partial socket reads and connect timeouts.
    • Improved timeout and error mapping for socket and TLS I/O to reduce spurious failures.
  • Refactor

    • Consolidated TLS connection model and standardized socket readiness/waiting for consistent cross-platform I/O behavior.
    • Simplified SSL/TLS state and buffering for more predictable reads/writes.
  • Tests

    • Added regression tests for fragmented/misaligned TLS record receives and short-read scenarios.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

📝 Walkthrough

Walkthrough

Unifies socket waiting with SockWaitKind/sock_wait and deadline handling, updates PySocket call sites, migrates SSL code to rustls::Connection with TLS-record-aware socket reads and cached socket methods, tightens handshake/SNI flows, updates error mapping, and adds two regression tests for short/fragmented recv behavior.

Changes

Socket Wait API and SSL/TLS Integration

Layer / File(s) Summary
host_env: remove PollKind/poll_socket
crates/host_env/src/socket.rs
Remove Unix-only PollKind and poll_socket; adjust Unix imports.
SockWait core
crates/stdlib/src/socket.rs
Introduce SockWaitKind, sock_wait and sock_wait_deadline with platform implementations (Unix poll_fds, Windows select) and re-export them.
Socket operations migration
crates/stdlib/src/socket.rs
Update PySocket helpers and all socket operations (connect, accept, recv*, send*, sendall, sendmsg, recvmsg, etc.) to use SockWaitKind and deadline-based waiting; simplify connect completion to rely on sock.take_error().
SSL imports, constants, and context layout
crates/stdlib/src/ssl.rs
Switch imports to rustls::Connection and SockWaitKind; extend OpenSSL-compat sizing/constants; rename session server-name storage to _server_name; remove legacy PySSLContext fields and add cached socket methods, TLS record header buffer, sni_callback_processed, and TLS_RECORD_HEADER_SIZE.
Socket-mode TLS integration
crates/stdlib/src/ssl.rs
Cache Python socket.send/socket.recv, add sock_recv_at_most_one_tls_record to preserve record boundaries, and switch handshake/flush/shutdown waits to sock_wait variants.
ssl/compat.rs: rustls::Connection migration
crates/stdlib/src/ssl/compat.rs
Remove internal TlsConnection enum and drive rustls via rustls::Connection; add record-aware recv helpers, rewrite handshake loop termination and SNI restart gating, update BIO draining sizing, reorganize ssl_read_tls_records, and adjust IO→Python error mapping.
Small cleanups and tests
crates/stdlib/src/ssl/error.rs, crates/stdlib/src/ssl/cert.rs, extra_tests/snippets/*
Remove redundant allow attributes, adjust cert import source, update error mapping (UnexpectedEof→EOF; raw_os_error None→PySSLError), and add two regression tests exercising short/fragmented recv conditions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • RustPython/RustPython#7604: Adds the host_env PollKind/poll_socket infrastructure that this PR removes and replaces with SockWaitKind/sock_wait.

Suggested reviewers

  • youknowone
  • ShaharNaveh

Poem

🐰 From polls and selects I hopped away,
SockWaitKind now waits both night and day.
One record at once, tidy bytes in line,
Handshakes restart once, SNI’s now fine.
Hooray — a small hop toward cleaner time!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Rustls integration improvements' is vague and generic, using non-descriptive terms that don't convey the specific main objective of the changeset. Consider using a more descriptive title that captures the primary fix, such as 'Fix rustls hang by handling short reads and socket waits properly' or similar.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The pull request comprehensively addresses all coding requirements from issue #7891: fixes recv/read behavior with short read handling and one TLS record per socket read, removes problematic buffer-full error handling with regression tests, replaces custom TlsConnection with rustls::Connection, fixes handshake logic, improves socket wait semantics (thread allowance, EINTR handling, error checks, poll on Unix), and properly reports rustls errors.
Out of Scope Changes check ✅ Passed All changes are directly related to rustls integration improvements and addressing issue #7891. No out-of-scope modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py (2)

205-216: 💤 Low value

Document the -17 TLS overhead constant.

max(1, n - 17) derives plaintext sizes from the captured wire record sizes by subtracting an assumed 17 bytes of TLS 1.3 framing overhead (5-byte record header + 1-byte content type + 11-byte AEAD tag minus padding, depending on cipher). A short comment would make the intent obvious to future readers, since the magic number is otherwise opaque.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py` around lines 205
- 216, The code uses a magic constant "-17" when computing plaintext_sizes from
TLS_RECORD_BODY_SIZES (plaintext_sizes = [max(1, n - 17) ...]) which is unclear;
update the code by adding a brief inline comment next to plaintext_sizes (or
above its definition) that explains the origin of 17 (5‑byte TLS record header +
1‑byte content type + ~11‑byte AEAD tag for TLS 1.3 framing) and notes that this
is an approximation for the captured wire record sizes so future readers
understand the subtraction, referencing variables TLS_RECORD_BODY_SIZES and
plaintext_sizes.

175-193: 💤 Low value

Handshake/read loops don't detect peer closing the connection.

If conn.recv(65536) returns b"" (client disconnects mid-handshake or before sending \r\n\r\n), incoming.write(b"") is a no-op and the next tls.do_handshake()/tls.read() re-raises SSLWantReadError, looping forever until guard_timeout aborts the process. The guard_timeout safety net covers this, but the failure mode would be a 20-second hang plus a process abort rather than a clean test failure with a useful traceback. Consider breaking out on empty recv (e.g. raise/return) so a regression in the client surfaces directly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py` around lines 175
- 193, The handshake and read loops using tls.do_handshake() and tls.read()
never detect a peer close because conn.recv(65536) returning b"" is ignored by
incoming.write(), causing infinite SSLWantReadError retries; modify both loops
(the block handling SSLWantReadError in the handshake loop and the
SSLWantReadError handler in the request/read loop) to check the result of
conn.recv(65536) and if it is b"" immediately break/raise an exception (or
return) so the function fails fast instead of looping until guard_timeout; keep
using drain_outgoing(outgoing, conn) as before but ensure you handle the
empty-recv case consistently to surface client disconnects cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@extra_tests/snippets/stdlib_ssl_short_recv.py`:
- Line 2: Remove the unused top-level import "random" from the file (the import
statement referencing the random module in stdlib_ssl_short_recv.py) since it is
not referenced anywhere and causes a Flake8 F401; simply delete that import line
so the module no longer imports random unnecessarily.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py`:
- Around line 144-148: The timeout guard in function guard_timeout prints the
wrong test filename; update the stderr message to reference the current file
name "stdlib_urllib_https_misaligned_recv.py" instead of
"stdlib_urllib_https_short_recv.py" so the printed line in guard_timeout
correctly identifies the failing test when it times out.

---

Nitpick comments:
In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py`:
- Around line 205-216: The code uses a magic constant "-17" when computing
plaintext_sizes from TLS_RECORD_BODY_SIZES (plaintext_sizes = [max(1, n - 17)
...]) which is unclear; update the code by adding a brief inline comment next to
plaintext_sizes (or above its definition) that explains the origin of 17 (5‑byte
TLS record header + 1‑byte content type + ~11‑byte AEAD tag for TLS 1.3 framing)
and notes that this is an approximation for the captured wire record sizes so
future readers understand the subtraction, referencing variables
TLS_RECORD_BODY_SIZES and plaintext_sizes.
- Around line 175-193: The handshake and read loops using tls.do_handshake() and
tls.read() never detect a peer close because conn.recv(65536) returning b"" is
ignored by incoming.write(), causing infinite SSLWantReadError retries; modify
both loops (the block handling SSLWantReadError in the handshake loop and the
SSLWantReadError handler in the request/read loop) to check the result of
conn.recv(65536) and if it is b"" immediately break/raise an exception (or
return) so the function fails fast instead of looping until guard_timeout; keep
using drain_outgoing(outgoing, conn) as before but ensure you handle the
empty-recv case consistently to surface client disconnects cleanly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 36cac236-b9a6-408d-b2e0-d26dd9385242

📥 Commits

Reviewing files that changed from the base of the PR and between c845861 and 6dccca7.

📒 Files selected for processing (8)
  • crates/host_env/src/socket.rs
  • crates/stdlib/src/socket.rs
  • crates/stdlib/src/ssl.rs
  • crates/stdlib/src/ssl/cert.rs
  • crates/stdlib/src/ssl/compat.rs
  • crates/stdlib/src/ssl/error.rs
  • extra_tests/snippets/stdlib_ssl_short_recv.py
  • extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
💤 Files with no reviewable changes (2)
  • crates/stdlib/src/ssl/error.rs
  • crates/host_env/src/socket.rs

Comment thread extra_tests/snippets/stdlib_ssl_short_recv.py Outdated
Comment thread extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/ssl.rs (1)

1894-1895: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cache the bound socket methods from the wrapped object.

Looking up send/recv on socket.socket hard-wires base-class dispatch and skips overrides on socket subclasses or instance-attached methods. That changes I/O behavior for wrapped custom sockets compared to resolving the methods from args.sock.

🐛 Proposed fix
-            let socket_mod = vm.import("socket", 0)?;
-            let socket_class = socket_mod.get_attr("socket", vm)?;
-
             // Create _SSLSocket instance
             let ssl_socket = PySSLSocket {
                 sock: args.sock.clone(),
-                sock_send_method: socket_class.get_attr("send", vm)?,
-                sock_recv_method: socket_class.get_attr("recv", vm)?,
+                sock_send_method: args.sock.get_attr("send", vm)?,
+                sock_recv_method: args.sock.get_attr("recv", vm)?,
                 tls_record_header_buf: vm
                     .ctx
                     .new_bytearray(Vec::with_capacity(TLS_RECORD_HEADER_SIZE))
                     .into(),
-            let socket_mod = vm.import("socket", 0)?;
-            let socket_class = socket_mod.get_attr("socket", vm)?;
-
             // Create _SSLSocket instance with BIO mode
             let ssl_socket = PySSLSocket {
                 sock: vm.ctx.none(), // No socket in BIO mode
-                sock_send_method: socket_class.get_attr("send", vm)?,
-                sock_recv_method: socket_class.get_attr("recv", vm)?,
+                sock_send_method: vm.ctx.none(),
+                sock_recv_method: vm.ctx.none(),
                 tls_record_header_buf: vm.ctx.none(),
-            self.sock_recv_method
-                .call((self.sock.clone(), vm.ctx.new_int(size)), vm)
+            self.sock_recv_method.call((vm.ctx.new_int(size),), vm)
...
-            self.sock_send_method
-                .call((self.sock.clone(), vm.ctx.new_bytes(data.to_vec())), vm)
+            self.sock_send_method
+                .call((vm.ctx.new_bytes(data.to_vec()),), vm)

Also applies to: 1975-1976

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl.rs` around lines 1894 - 1895, The code currently caches
send/recv using socket_class.get_attr which forces base-class lookup and ignores
overrides on the wrapped instance; change the lookup to retrieve the bound
methods from the actual wrapped object (args.sock.get_attr("send", vm) and
args.sock.get_attr("recv", vm)) so instance-attached methods and subclass
overrides are honored, and apply the same change for the other occurrence that
sets sock_send_method/sock_recv_method; preserve the existing error handling
semantics when get_attr returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/stdlib/src/ssl.rs`:
- Around line 1894-1895: The code currently caches send/recv using
socket_class.get_attr which forces base-class lookup and ignores overrides on
the wrapped instance; change the lookup to retrieve the bound methods from the
actual wrapped object (args.sock.get_attr("send", vm) and
args.sock.get_attr("recv", vm)) so instance-attached methods and subclass
overrides are honored, and apply the same change for the other occurrence that
sets sock_send_method/sock_recv_method; preserve the existing error handling
semantics when get_attr returns an error.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 629fa543-a407-4c56-9f0a-d36885bf549f

📥 Commits

Reviewing files that changed from the base of the PR and between 6dccca7 and 53692bf.

📒 Files selected for processing (5)
  • crates/stdlib/src/ssl.rs
  • crates/stdlib/src/ssl/cert.rs
  • crates/stdlib/src/ssl/compat.rs
  • extra_tests/snippets/stdlib_ssl_short_recv.py
  • extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
✅ Files skipped from review due to trivial changes (1)
  • crates/stdlib/src/ssl/cert.rs

im-0 added 4 commits May 21, 2026 15:16
Use method references cached during socket creation.
Previous algorithm didn't take into account that recv() may return less
data than requested even for blocking sockets.
First of all, existing code does not really work and this leads to an
infinite loop: RustPython#7891

Second, this should never happen when rustls used properly (wrt
wants_read() and wants_write()) and thus all such errors are
implementation bugs that must be properly fixed.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/ssl.rs (1)

1842-1867: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reuse validate_hostname() in _wrap_socket.

This path no longer enforces the 253-character limit that _wrap_bio() and set_server_hostname() apply, so oversized hostnames now fail later with a different error surface instead of the expected early ValueError.

Suggested fix
                 Some(hostname_str) => {
                     let hostname = hostname_str.as_str();
-
-                    // Validate hostname
-                    if hostname.is_empty() {
-                        return Err(vm.new_value_error("server_hostname cannot be an empty string"));
-                    }
-
-                    // Check if it starts with a dot
-                    if hostname.starts_with('.') {
-                        return Err(vm.new_value_error("server_hostname cannot start with a dot"));
-                    }
-
-                    // IP addresses are allowed
-                    // SNI will not be sent for IP addresses
-
-                    // Check for NULL bytes
-                    if hostname.contains('\0') {
-                        return Err(vm.new_type_error("embedded null character"));
-                    }
-
+                    validate_hostname(hostname, vm)?;
                     Some(hostname.to_string())
                 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl.rs` around lines 1842 - 1867, The hostname validation
in _wrap_socket currently duplicates checks but omits the 253-character limit
enforced by validate_hostname(); update the _wrap_socket path (the code handling
args.server_hostname in ssl.rs) to call the existing
validate_hostname(hostname_str) utility instead of reimplementing checks so the
same length, dot, empty, and null-byte validations are applied and the correct
ValueError is raised consistently (replace the manual checks in the
Some(hostname_str) branch with a call to validate_hostname and use its result to
produce the Some(String) or appropriate VM error).
🧹 Nitpick comments (1)
crates/stdlib/src/ssl/compat.rs (1)

756-758: ⚡ Quick win

Import VERIFY_X509_PARTIAL_CHAIN instead of redefining 0x80000.

This file already shares verification flags through _ssl; keeping a second literal copy here makes the two paths drift-prone the next time those constants move.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl/compat.rs` around lines 756 - 758, Remove the local
const VERIFY_X509_PARTIAL_CHAIN = 0x80000 and instead reference the shared
constant from the existing _ssl module; update the imports/usages so the
conditional uses _ssl::VERIFY_X509_PARTIAL_CHAIN (or whichever symbol is
exported from the _ssl shim) in the expression options.verify_flags &
VERIFY_X509_PARTIAL_CHAIN != 0 and keep the rest of the PartialChainVerifier
wrapping logic unchanged so the code relies on the single shared constant
instead of a duplicated literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@extra_tests/snippets/stdlib_ssl_short_recv.py`:
- Around line 55-56: Replace the overly broad except BaseException clause with
except Exception so system-level exceptions propagate: change "except
BaseException as exc:" to "except Exception as exc:" (or "except Exception:" if
you prefer not to reuse exc) in the try/except that appends to server_errors,
leaving the server_errors.append(exc) logic intact.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py`:
- Around line 183-196: When handling ssl.SSLWantReadError in the client loop
(locations using tls.read, drain_outgoing, conn.recv and incoming.write), check
the return value of conn.recv(...) and if it returns b"" call
incoming.write_eof() instead of incoming.write(b"") so EOF is propagated into
the MemoryBIO and TLS can progress; keep calling drain_outgoing(outgoing, conn)
as before. Also replace the server-side catch-all "except BaseException as exc:"
with "except Exception as exc:" to satisfy the BLE001 lint warning.
- Around line 221-222: Replace the overly broad except BaseException handler
with except Exception to avoid catching SystemExit/KeyboardInterrupt; locate the
try/except that appends to server_errors (the lines containing "except
BaseException as exc" and "server_errors.append(exc)") and change it to "except
Exception as exc" so only regular exceptions from the test thread are captured.

---

Outside diff comments:
In `@crates/stdlib/src/ssl.rs`:
- Around line 1842-1867: The hostname validation in _wrap_socket currently
duplicates checks but omits the 253-character limit enforced by
validate_hostname(); update the _wrap_socket path (the code handling
args.server_hostname in ssl.rs) to call the existing
validate_hostname(hostname_str) utility instead of reimplementing checks so the
same length, dot, empty, and null-byte validations are applied and the correct
ValueError is raised consistently (replace the manual checks in the
Some(hostname_str) branch with a call to validate_hostname and use its result to
produce the Some(String) or appropriate VM error).

---

Nitpick comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 756-758: Remove the local const VERIFY_X509_PARTIAL_CHAIN =
0x80000 and instead reference the shared constant from the existing _ssl module;
update the imports/usages so the conditional uses
_ssl::VERIFY_X509_PARTIAL_CHAIN (or whichever symbol is exported from the _ssl
shim) in the expression options.verify_flags & VERIFY_X509_PARTIAL_CHAIN != 0
and keep the rest of the PartialChainVerifier wrapping logic unchanged so the
code relies on the single shared constant instead of a duplicated literal.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 66de22b5-fbbb-4a02-9b0b-4db2e1ac1cb6

📥 Commits

Reviewing files that changed from the base of the PR and between 53692bf and e97e483.

📒 Files selected for processing (8)
  • crates/host_env/src/socket.rs
  • crates/stdlib/src/socket.rs
  • crates/stdlib/src/ssl.rs
  • crates/stdlib/src/ssl/cert.rs
  • crates/stdlib/src/ssl/compat.rs
  • crates/stdlib/src/ssl/error.rs
  • extra_tests/snippets/stdlib_ssl_short_recv.py
  • extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
💤 Files with no reviewable changes (2)
  • crates/stdlib/src/ssl/error.rs
  • crates/host_env/src/socket.rs
✅ Files skipped from review due to trivial changes (1)
  • crates/stdlib/src/ssl/cert.rs

Comment on lines +55 to +56
except BaseException as exc:
server_errors.append(exc)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the target lines with context
sed -n '40,75p' extra_tests/snippets/stdlib_ssl_short_recv.py

# Confirm whether Ruff is configured to enforce BLE001 (and any ignore rules)
ls -1 . | head
rg -n "BLE001" -S . || true
rg -n "ruff" -S pyproject.toml setup.cfg .ruff.toml ruff.toml 2>/dev/null || true

# Check if ruff/flake8 is run on extra_tests/snippets python files
rg -n "extra_tests/snippets" -S . || true

Repository: RustPython/RustPython

Length of output: 1999


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ruff.toml ==="
sed -n '1,120p' ruff.toml

echo "=== Does ruff ignore extra_tests/snippets? ==="
rg -n "extra_tests/snippets|snippets" ruff.toml . || true

echo "=== Are ruff runs configured for specific paths? ==="
rg -n "ruff(\s|\.|_)?(check|lint)" -S . || true

Repository: RustPython/RustPython

Length of output: 6776


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find Ruff invocation in repo (CI, scripts)
rg -n --hidden --glob '!.git' "ruff(\s|\.|_)+check|ruff(\s|\.|_)+lint|ruff\.toml|--config ruff\.toml|Ruff" .github scripts . || true

# Look for Ruff configuration exclusions (exclude/extend-exclude)
rg -n "exclude|extend-exclude|per-file-ignores" ruff.toml . || true

# Inspect extra_tests/test_snippets.py if it runs ruff or any lint step
sed -n '1,120p' extra_tests/test_snippets.py 2>/dev/null || true

Repository: RustPython/RustPython

Length of output: 21473


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ruff.toml (tail) ==="
sed -n '120,260p' ruff.toml

echo "=== Any ruff CLI invocations? ==="
rg -n --hidden --glob '!.git' "ruff(\s|\.|_)+check|ruff(\s|\.|_)+lint|--config ruff\.toml" .github scripts . || true

echo "=== Any CI job uses ruff? ==="
rg -n --hidden --glob '!.git' "ruff" .github/workflows . || true

echo "=== Ruff exclude/extend-exclude in config? ==="
rg -n "exclude\s*=|extend-exclude|per-file-ignores" ruff.toml . || true

Repository: RustPython/RustPython

Length of output: 19327


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '1,120p' .pre-commit-config.yaml
rg -n "ruff" .pre-commit-config.yaml

Repository: RustPython/RustPython

Length of output: 2128


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find any CI invocation of `ruff check` / `ruff` that isn't the pre-commit config.
rg -n --hidden --glob '!.git' \
  -S '(ruff\s+check|ruff\s+lint|ruff-pre-commit|--select\s+BLE|--select\s+.*BLE|BLE001|--extend-select.*BLE)' \
  .github/workflows .github/actions scripts . || true

# Also search for ruff usage that explicitly references ruff.toml or passes `check` without --select I.
rg -n --hidden --glob '!.git' -S 'ruff(\s|_)+check|ruff(\s|_)+lint|ruff\.toml' .github/workflows scripts . || true

Repository: RustPython/RustPython

Length of output: 149


🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!.git' -S "(^|\\s)ruff\\s+check\\b|ruff\\s+lint\\b|ruff-pre-commit|pre-commit.*ruff|ruff\\.toml" . || true
rg -n --hidden --glob '!.git' -S "extra_tests/snippets/.*\\.py" . || true
rg -n --hidden --glob '!.git' -S "BLE001|blind-except|--select\\s+BLE" . || true

Repository: RustPython/RustPython

Length of output: 884


Catch Exception instead of BaseException in extra_tests/snippets/stdlib_ssl_short_recv.py (lines 55-56).

BaseException is broader than needed and can swallow SystemExit/KeyboardInterrupt; narrow to Exception so process-level exits still propagate. (This repo’s Ruff pre-commit hook only runs --select I, so BLE001 isn’t enforced automatically here.)

🧰 Tools
🪛 Ruff (0.15.13)

[warning] 55-55: Do not catch blind exception: BaseException

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extra_tests/snippets/stdlib_ssl_short_recv.py` around lines 55 - 56, Replace
the overly broad except BaseException clause with except Exception so
system-level exceptions propagate: change "except BaseException as exc:" to
"except Exception as exc:" (or "except Exception:" if you prefer not to reuse
exc) in the try/except that appends to server_errors, leaving the
server_errors.append(exc) logic intact.

Comment on lines +183 to +196
except ssl.SSLWantReadError:
drain_outgoing(outgoing, conn)
incoming.write(conn.recv(65536))
except ssl.SSLWantWriteError:
pass
drain_outgoing(outgoing, conn)

request = b""
while b"\r\n\r\n" not in request:
try:
request += tls.read(65536)
except ssl.SSLWantReadError:
drain_outgoing(outgoing, conn)
incoming.write(conn.recv(65536))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In Python's sslmodule, when usingssl.MemoryBIOwithSSLObject.wrap_bio, how should EOF from the underlying TCP socket be propagated? Specifically, what is the effect of MemoryBIO.write_eof()versus writingb""afterrecv() returns EOF?

💡 Result:

EOF should be propagated by calling MemoryBIO.write_eof on the “incoming” (readable) BIO you connected to SSLObject.wrap_bio; it writes an explicit EOF marker into the BIO so SSLObject will observe end-of-stream and produce the correct TLS/SSL-level shutdown behavior.[1][2] Effect difference vs writing b"" after recv returns EOF: - MemoryBIO.write_eof: places the EOF marker into the BIO; after it is called, MemoryBIO.write becomes illegal, and the MemoryBIO’s eof property becomes true only after any already-buffered data has been read out. This makes the SSL layer treat the stream as terminated (i.e., a real end-of-file condition, not just “no newly arrived bytes”).[2][3] - MemoryBIO.write(b""): writing an empty bytes object does not signal EOF; it is effectively “no new data”. As a result, the SSL engine may keep waiting for more encrypted input rather than treating the transport as closed (you can end up with SSLEOFError/EOF handling that only triggers once the EOF marker is actually set, not merely when a zero-length chunk is written). This is why the Python memory BIO API includes a dedicated write_eof method rather than using b"" as a sentinel.[2][3] So, when recv returns b"" (TCP EOF), translate that event into SSL input EOF by calling incoming_bio.write_eof (not incoming_bio.write(b"")), then continue the normal loop of reading/writing with the SSLObject so any resulting TLS closure alerts can be handled.[2][3]

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py"
ls -l "$FILE"
python3 - <<PY
import itertools,sys
path="$FILE"
with open(path,'r',encoding='utf-8') as f:
    # Print around 175-210 and 205-240 to cover the cited regions
    lines=list(enumerate(f.readlines(), start=1))
def dump(start,end):
    for i in range(start-1,end):
        if 1 <= i < len(lines):
            n,txt=lines[i]
            print(f"{n}:{txt.rstrip()}")
dump(160,210)
print("----")
dump(210,250)
PY

Repository: RustPython/RustPython

Length of output: 3184


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "write_eof\(" -S .
rg -n "except BaseException" -S extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
rg -n "BaseException" -S .
ls
# show ruff config if present
fd -t f "ruff.*(toml|ini|yml|yaml|json)" . || true
fd -t f "pyproject.toml" . || true

Repository: RustPython/RustPython

Length of output: 50377


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find ruff config
fd -t f "pyproject.toml" . -a --max-results 20
fd -t f "ruff.*(toml|ini|json|yaml|yml)" . --max-results 20 || true

# If pyproject exists, show relevant ruff sections
if [ -f pyproject.toml ]; then
  echo "=== pyproject.toml ruff sections ==="
  rg -n "^\[tool\.ruff" -n pyproject.toml
  rg -n "exception|BaseException|BLE|catch" pyproject.toml || true
  # show likely exception-related rules
  sed -n '1,220p' pyproject.toml | cat
fi

# also search for 'ruff' in extra_tests snippet dir just in case
rg -n "ruff:" -S extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py || true

Repository: RustPython/RustPython

Length of output: 166


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ruff.toml ==="
sed -n '1,260p' ruff.toml | cat

echo "=== search for exception-related rules ==="
rg -n "BLE0|BaseException|catch|except BaseException|E+|BLE001|BLE002|flake8-bugbear" ruff.toml || true

Repository: RustPython/RustPython

Length of output: 2067


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py"

echo "=== watchdog / surrounding lines (140-170) ==="
python3 - <<'PY'
path = "extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py"
with open(path, "r", encoding="utf-8") as f:
    for i, line in enumerate(f, start=1):
        if 130 <= i <= 170:
            print(f"{i}:{line.rstrip()}")
PY

echo "=== ruff check on the snippet ==="
if command -v ruff >/dev/null 2>&1; then
  ruff check "$FILE" --output-format=concise || true
else
  echo "ruff not installed in this environment"
fi

Repository: RustPython/RustPython

Length of output: 1392


Propagate socket EOF into the TLS BIO.

  • In both SSLWantReadError blocks, conn.recv(...) can return b""; writing empty bytes to ssl.MemoryBIO doesn’t signal EOF to wrap_bio(), so the TLS loop can fail to make progress until the watchdog aborts. Call incoming.write_eof() when recv() returns b"".
Suggested fix
             except ssl.SSLWantReadError:
                 drain_outgoing(outgoing, conn)
-                incoming.write(conn.recv(65536))
+                data = conn.recv(65536)
+                if data:
+                    incoming.write(data)
+                else:
+                    incoming.write_eof()
             except ssl.SSLWantWriteError:
                 pass
             drain_outgoing(outgoing, conn)

         request = b""
         while b"\r\n\r\n" not in request:
             try:
                 request += tls.read(65536)
             except ssl.SSLWantReadError:
                 drain_outgoing(outgoing, conn)
-                incoming.write(conn.recv(65536))
+                data = conn.recv(65536)
+                if data:
+                    incoming.write(data)
+                else:
+                    incoming.write_eof()
             drain_outgoing(outgoing, conn)
- `except BaseException as exc:` at the server error handler is flagged by Ruff (`BLE001`); narrow it to `except Exception as exc:`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py` around lines 183
- 196, When handling ssl.SSLWantReadError in the client loop (locations using
tls.read, drain_outgoing, conn.recv and incoming.write), check the return value
of conn.recv(...) and if it returns b"" call incoming.write_eof() instead of
incoming.write(b"") so EOF is propagated into the MemoryBIO and TLS can
progress; keep calling drain_outgoing(outgoing, conn) as before. Also replace
the server-side catch-all "except BaseException as exc:" with "except Exception
as exc:" to satisfy the BLE001 lint warning.

Comment on lines +221 to +222
except BaseException as exc:
server_errors.append(exc)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate repository root and inspect the target snippet around the flagged lines.
FILE="extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py"
echo "== Target file: $FILE =="
test -f "$FILE" && sed -n '190,260p' "$FILE" | cat -n || { echo "MISSING: $FILE"; exit 1; }

# Check whether ruff BLE001 is configured/enabled (and any existing ignores).
echo
echo "== Ruff configuration (search for BLE001 / lint settings) =="
rg -n --hidden --no-ignore-vcs "BLE001" -S . || true
rg -n --hidden --no-ignore-vcs "\\[tool\\.ruff\\]|\\[tool\\.ruff\\.lint\\]|target-version|select|ignore" -S . || true

# Find any ruff usage on this snippet specifically (if documented) and any similar patterns in repo.
echo
echo "== Existing occurrences of BaseException in snippets =="
rg -n "except BaseException" extra_tests/snippets || true

echo
echo "== Existing occurrences of Exception in similar threading/error capture patterns =="
rg -n "server_errors\\.append\\(|thread.*error|watchdog" extra_tests/snippets || true

Repository: RustPython/RustPython

Length of output: 50378


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Find Ruff config files =="
fd -t f "pyproject.toml" . | head
fd -t f "ruff*.toml" . | head
fd -t f ".ruff*.toml" . | head
fd -t f ".pre-commit-config.yaml" . | head

echo
echo "== Search for BLE001 specifically (limit output) =="
rg -n --hidden --no-ignore-vcs "BLE001" . || true

echo
echo "== Search for ruff tool config (limit output) =="
rg -n --hidden --no-ignore-vcs "\\[tool\\.ruff" pyproject.toml ruff.toml .pre-commit-config.yaml **/pyproject.toml **/ruff*.toml 2>/dev/null || true

echo
echo "== Occurrences of except BaseException in extra_tests/snippets =="
rg -n "except BaseException" extra_tests/snippets || true

Repository: RustPython/RustPython

Length of output: 592


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== ruff.toml (relevant BLE* / lint config) =="
rg -n "BLE001|BaseException|bugbear.*exception|except BaseException" ruff.toml || true
echo
echo "== Show linter sections around those (if present) =="
# Print some likely sections without dumping the whole file
# (ruff.toml is usually small; but still cap output)
python3 - <<'PY'
import re
path="ruff.toml"
with open(path,'r',encoding='utf-8') as f:
    s=f.read().splitlines()
keywords=["[lint.","[tool.ruff.","[lint.bugbear.","[lint.pycodestyle.","[lint.mccabe.","BLE001","BaseException","bugbear"]
hits=[i for i,line in enumerate(s,1) if any(k in line for k in keywords)]
print("hits:", hits[:20])
for i in hits[:20]:
    start=max(1,i-10); end=min(len(s),i+10)
    print("\n---",path,"lines",start,"..",end,"(around",i,")---")
    for ln in range(start,end+1):
        print(f"{ln:5d} {s[ln-1]}")
PY

echo
echo "== Any per-file-ignores for snippets / BaseException in ruff.toml =="
rg -n "per-file-ignores|extra_tests/snippets|snippets|BaseException" ruff.toml || true

Repository: RustPython/RustPython

Length of output: 1522


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show surrounding code for all except BaseException occurrences in extra_tests/snippets
for f in \
  extra_tests/snippets/stdlib_ssl_short_recv.py \
  extra_tests/snippets/syntax_try.py \
  extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
do
  echo "==== $f (around except BaseException) ===="
  rg -n "except BaseException as" "$f"
  # print a small window around each match line
  for ln in $(rg -n "except BaseException as" "$f" | cut -d: -f1); do
    start=$((ln-10)); end=$((ln+10))
    sed -n "${start},${end}p" "$f" | cat -n
  done
  echo
done

Repository: RustPython/RustPython

Length of output: 2810


🏁 Script executed:

#!/bin/bash
set -euo pipefail

print_window() {
  local file="$1"; local match_line="$2"; local before="${3:-10}"; local after="${4:-10}"
  local start=$((match_line-before))
  local end=$((match_line+after))
  if [ "$start" -lt 1 ]; then start=1; fi
  sed -n "${start},${end}p" "$file" | cat -n
}

echo "== stdlib_urllib_https_misaligned_recv.py (around BaseException) =="
FILE="extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py"
rg -n "except BaseException as" "$FILE"
ln="$(rg -n "except BaseException as" "$FILE" | head -n1 | cut -d: -f1)"
print_window "$FILE" "$ln" 12 12

echo
echo "== stdlib_ssl_short_recv.py (around BaseException) =="
FILE="extra_tests/snippets/stdlib_ssl_short_recv.py"
rg -n "except BaseException as" "$FILE"
ln="$(rg -n "except BaseException as" "$FILE" | head -n1 | cut -d: -f1)"
print_window "$FILE" "$ln" 12 12

echo
echo "== syntax_try.py (around BaseException) =="
FILE="extra_tests/snippets/syntax_try.py"
rg -n "except BaseException" "$FILE"
ln="$(rg -n "except BaseException" "$FILE" | head -n1 | cut -d: -f1)"
print_window "$FILE" "$ln" 10 10

Repository: RustPython/RustPython

Length of output: 2521


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search ruff invocation in .pre-commit-config.yaml =="
rg -n "ruff" .pre-commit-config.yaml || true

echo
echo "== Show ruff-related pre-commit entries (small window) =="
# print some lines around the first few ruff matches
for m in $(rg -n "ruff" .pre-commit-config.yaml | cut -d: -f1 | head -n 10); do
  start=$((m-10)); end=$((m+40))
  sed -n "${start},${end}p" .pre-commit-config.yaml | cat
  echo "----"
done

Repository: RustPython/RustPython

Length of output: 4365


🌐 Web query:

Ruff BLE001 BaseException Exception flake8-blind-except meaning

💡 Result:

In the Ruff linter, BLE001 is the code for the "blind-except" rule [1][2], which is derived from the flake8-blind-except linter [3]. Its purpose is to flag overly broad except clauses that catch all exceptions, specifically those using BaseException or Exception [3]. Meaning and Rationale: Catching BaseException or Exception is considered "blind" because it can unintentionally swallow critical system signals or programming errors [3]. For example, catching these broad types can intercept KeyboardInterrupt (preventing a user from stopping the program with Ctrl+C) or SystemExit (preventing graceful exits) [3]. By flagging these, BLE001 encourages developers to catch only specific, expected exception types [3]. Exceptions to the Rule: Ruff will not trigger a BLE001 violation if the caught exception is either: 1. Re-raised: The code explicitly uses raise to propagate the exception after potentially performing some action [3][4]. 2. Logged with exc_info: The exception is logged using a standard logger method (e.g., logging.exception) with exc_info enabled, which is a common and accepted pattern for reporting errors while maintaining the traceback [3][5]. If you encounter this error and believe the catch is intentional, it is best practice to either narrow the exception type or, if the broad catch is truly necessary (e.g., at a top-level error boundary), use a # noqa: BLE001 comment to suppress the warning [6].

Citations:


Use Exception rather than BaseException for test-thread error capture (extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py:221-222)

    except BaseException as exc:
        server_errors.append(exc)

Catching BaseException can mask SystemExit/KeyboardInterrupt from that thread, and Ruff’s BLE001 (blind-except) flags this since ruff.toml selects BLE.

🧰 Tools
🪛 Ruff (0.15.13)

[warning] 221-221: Do not catch blind exception: BaseException

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py` around lines 221
- 222, Replace the overly broad except BaseException handler with except
Exception to avoid catching SystemExit/KeyboardInterrupt; locate the try/except
that appends to server_errors (the lines containing "except BaseException as
exc" and "server_errors.append(exc)") and change it to "except Exception as exc"
so only regular exceptions from the test thread are captured.

im-0 added 7 commits May 21, 2026 16:18
1) Ensure that socket_wait() called from TLS glue code allows threads
2) Ensure that socket_wait() called from TLS glue code properly handles
   EINTR on *nix
3) Ensure that select() or poll() error conditions are checked
4) Use poll() on *nix so socket descriptor values are not limited
TLS handshake cannot be infinite. Any infinite loop here is a serious
bug in implementation and should be fixed properly.

This code triggers in some cases (very short reads) with misleading
`ssl_error.SSLWantReadError: The operation did not complete (read)`.
* Deduplicate verify flags / record-size constants
* Larger "max encrypted TLS record length"
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/ssl.rs (1)

4414-4433: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't treat EOF/reset as a successful close_notify.

This helper now returns true for empty reads and arbitrary socket errors, and it drops read_tls()/process_new_packets() failures. shutdown() breaks on that true, so a ragged EOF or TCP reset can complete unwrap() successfully instead of surfacing the protocol error.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl.rs` around lines 4414 - 4433, The helper
try_read_close_notify_socket currently treats an empty read and arbitrary socket
errors as a successful close_notify; instead, when
sock_recv_at_most_one_tls_record returns Ok(data) and data.is_empty() you must
NOT return true — return false (or propagate an error) so shutdown() does not
consider the TLS shutdown complete; likewise, do not ignore errors from
conn.read_tls() and conn.process_new_packets(): check their Results and if they
Err, propagate that error to the VM (or return false) instead of swallowing it;
finally, for Err(e) from sock_recv_at_most_one_tls_record, only return false for
blocking IO errors (is_blocking_io_error), but for other socket errors
propagate/raise the error rather than returning true. Ensure you update the
logic in try_read_close_notify_socket to examine conn.read_tls and
conn.process_new_packets results and to treat EOF/reset and non-blocking socket
errors as errors (not successful close_notify).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Line 1081: The BIO-mode read in compat.rs currently calls
socket.sock_recv(SSL3_RT_MAX_PACKET_SIZE, vm) and assumes the entire chunk is
consumed, which can return multiple TLS records from MemoryBIO; update the code
that handles socket.sock_recv (and the surrounding BIO-path) to apply the same
record-boundary logic used for socket reads: parse the returned buffer for a
single TLS record, consume only the first record and preserve any unread suffix
bytes back into the MemoryBIO (or buffer) for future reads instead of discarding
them; ensure the fix references the same handling around SSL3_RT_MAX_PACKET_SIZE
and the MemoryBIO consumer so BIO callers maintain the one-record-per-read
invariant.

---

Outside diff comments:
In `@crates/stdlib/src/ssl.rs`:
- Around line 4414-4433: The helper try_read_close_notify_socket currently
treats an empty read and arbitrary socket errors as a successful close_notify;
instead, when sock_recv_at_most_one_tls_record returns Ok(data) and
data.is_empty() you must NOT return true — return false (or propagate an error)
so shutdown() does not consider the TLS shutdown complete; likewise, do not
ignore errors from conn.read_tls() and conn.process_new_packets(): check their
Results and if they Err, propagate that error to the VM (or return false)
instead of swallowing it; finally, for Err(e) from
sock_recv_at_most_one_tls_record, only return false for blocking IO errors
(is_blocking_io_error), but for other socket errors propagate/raise the error
rather than returning true. Ensure you update the logic in
try_read_close_notify_socket to examine conn.read_tls and
conn.process_new_packets results and to treat EOF/reset and non-blocking socket
errors as errors (not successful close_notify).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 6b063cdf-afc5-4a66-9280-e74cb3ff1b50

📥 Commits

Reviewing files that changed from the base of the PR and between e97e483 and 6c0e292.

📒 Files selected for processing (8)
  • crates/host_env/src/socket.rs
  • crates/stdlib/src/socket.rs
  • crates/stdlib/src/ssl.rs
  • crates/stdlib/src/ssl/cert.rs
  • crates/stdlib/src/ssl/compat.rs
  • crates/stdlib/src/ssl/error.rs
  • extra_tests/snippets/stdlib_ssl_short_recv.py
  • extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
💤 Files with no reviewable changes (2)
  • crates/stdlib/src/ssl/error.rs
  • crates/host_env/src/socket.rs
✅ Files skipped from review due to trivial changes (1)
  • crates/stdlib/src/ssl/cert.rs

recv_at_most_one_tls_record(socket, vm)?
} else {
match socket.sock_recv(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
match socket.sock_recv(SSL3_RT_MAX_PACKET_SIZE, vm) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

BIO mode still bypasses the one-record read invariant.

These reads can pull multiple TLS records from MemoryBIO in one chunk, so the rustls buffering edge case this PR fixes for sockets is still reachable for asyncio/BIO callers. Please apply the same record-boundary logic here, or preserve unread suffix bytes instead of assuming the whole chunk can be consumed safely.

Also applies to: 1918-1918

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/stdlib/src/ssl/compat.rs` at line 1081, The BIO-mode read in compat.rs
currently calls socket.sock_recv(SSL3_RT_MAX_PACKET_SIZE, vm) and assumes the
entire chunk is consumed, which can return multiple TLS records from MemoryBIO;
update the code that handles socket.sock_recv (and the surrounding BIO-path) to
apply the same record-boundary logic used for socket reads: parse the returned
buffer for a single TLS record, consume only the first record and preserve any
unread suffix bytes back into the MemoryBIO (or buffer) for future reads instead
of discarding them; ensure the fix references the same handling around
SSL3_RT_MAX_PACKET_SIZE and the MemoryBIO consumer so BIO callers maintain the
one-record-per-read invariant.

Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYSM!

thank you for looking into this:)

Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much!

@youknowone youknowone merged commit 2a16360 into RustPython:main May 22, 2026
26 checks passed
@im-0 im-0 deleted the fix-rustls branch May 22, 2026 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

urlopen() hangs and eats 100% CPU on some https urls (git bisect found problematic commit)

3 participants