Rustls integration improvements#7946
Conversation
📝 WalkthroughWalkthroughUnifies 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. ChangesSocket Wait API and SSL/TLS Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
extra_tests/snippets/stdlib_urllib_https_misaligned_recv.py (2)
205-216: 💤 Low valueDocument the
-17TLS 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 valueHandshake/read loops don't detect peer closing the connection.
If
conn.recv(65536)returnsb""(client disconnects mid-handshake or before sending\r\n\r\n),incoming.write(b"")is a no-op and the nexttls.do_handshake()/tls.read()re-raisesSSLWantReadError, looping forever untilguard_timeoutaborts the process. Theguard_timeoutsafety 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
📒 Files selected for processing (8)
crates/host_env/src/socket.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/stdlib/src/ssl/error.rsextra_tests/snippets/stdlib_ssl_short_recv.pyextra_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
There was a problem hiding this comment.
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 winCache the bound socket methods from the wrapped object.
Looking up
send/recvonsocket.sockethard-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 fromargs.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
📒 Files selected for processing (5)
crates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rsextra_tests/snippets/stdlib_ssl_short_recv.pyextra_tests/snippets/stdlib_urllib_https_misaligned_recv.py
✅ Files skipped from review due to trivial changes (1)
- crates/stdlib/src/ssl/cert.rs
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.
There was a problem hiding this comment.
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 winReuse
validate_hostname()in_wrap_socket.This path no longer enforces the 253-character limit that
_wrap_bio()andset_server_hostname()apply, so oversized hostnames now fail later with a different error surface instead of the expected earlyValueError.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 winImport
VERIFY_X509_PARTIAL_CHAINinstead of redefining0x80000.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
📒 Files selected for processing (8)
crates/host_env/src/socket.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/stdlib/src/ssl/error.rsextra_tests/snippets/stdlib_ssl_short_recv.pyextra_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
| except BaseException as exc: | ||
| server_errors.append(exc) |
There was a problem hiding this comment.
🧩 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 . || trueRepository: 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 . || trueRepository: 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 || trueRepository: 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 . || trueRepository: 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.yamlRepository: 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 . || trueRepository: 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" . || trueRepository: 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.
| 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)) |
There was a problem hiding this comment.
🧩 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:
- 1: https://runebook.dev/en/docs/python/library/ssl/ssl.MemoryBIO.eof
- 2: https://bugs.python.org/file36806/ssl-memory-bio-5.patch
- 3: https://docs.python.org/release/3.13.2/library/ssl.html
🏁 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)
PYRepository: 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" . || trueRepository: 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 || trueRepository: 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 || trueRepository: 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"
fiRepository: RustPython/RustPython
Length of output: 1392
Propagate socket EOF into the TLS BIO.
- In both
SSLWantReadErrorblocks,conn.recv(...)can returnb""; writing empty bytes tossl.MemoryBIOdoesn’t signal EOF towrap_bio(), so the TLS loop can fail to make progress until the watchdog aborts. Callincoming.write_eof()whenrecv()returnsb"".
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)🤖 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.
| except BaseException as exc: | ||
| server_errors.append(exc) |
There was a problem hiding this comment.
🧩 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 || trueRepository: 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 || trueRepository: 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 || trueRepository: 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
doneRepository: 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 10Repository: 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 "----"
doneRepository: 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:
- 1: https://docs.astral.sh/ruff/rules/?featured_on=talkpython
- 2: https://docs.astral.sh/ruff/rules/
- 3: https://docs.astral.sh/ruff/rules/blind-except/
- 4: [Question] Re-raising errors and BLE001 astral-sh/ruff#1119
- 5: BLE001 rule with blind exception
Exceptionis passing the check astral-sh/ruff#6421 - 6: [CHORE][PYTHON]: Enable ruff BLE001 rule to tighten bare exception handling IBM/mcp-context-forge#3348
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.
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"
There was a problem hiding this comment.
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 winDon't treat EOF/reset as a successful
close_notify.This helper now returns
truefor empty reads and arbitrary socket errors, and it dropsread_tls()/process_new_packets()failures.shutdown()breaks on thattrue, so a ragged EOF or TCP reset can completeunwrap()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
📒 Files selected for processing (8)
crates/host_env/src/socket.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/stdlib/src/ssl/error.rsextra_tests/snippets/stdlib_ssl_short_recv.pyextra_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) { |
There was a problem hiding this comment.
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.
ShaharNaveh
left a comment
There was a problem hiding this comment.
TYSM!
thank you for looking into this:)
Primary goal of this PR is to fix #7891
Changes:
import socketon each send()/recv() when using rustlsssl_error.SSLWantReadError: The operation did not complete (read).vm.allow_threads())Please note that this is no way near complete and current implementation probably still has a ton of bugs.
Comments
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
Refactor
Tests