Skip to content

Conversation

@youknowone
Copy link
Member

@youknowone youknowone commented Jan 29, 2026

Summary by CodeRabbit

  • Bug Fixes

    • Improved SSL/TLS socket shutdown behavior to reliably distinguish encrypted records from post-TLS cleartext, preventing data loss and ensuring correct connection close handling.
  • Tests

    • Added tests ensuring sorting uses the expected comparison method (verifying lt is used and gt is not), including reverse-order scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds socket-peek-based TLS shutdown/read logic to the SSL module to distinguish TLS records from post-TLS cleartext during close-notify handling, plus a private helper to consume exactly one TLS record when appropriate. Also adds tests tracking comparison calls in sorting.

Changes

Cohort / File(s) Summary
TLS socket & TLS shutdown logic
crates/stdlib/src/ssl.rs
Added sock_peek (pub(crate)) and try_read_close_notify_socket (private). Updated non-BIO shutdown/read paths to use MSG_PEEK to detect TLS records vs post-TLS cleartext, consume exactly one TLS record when needed, and invoke process_new_packets after reading TLS data. Adjusted socket recv/send usage in shutdown flows.
Sorting comparison tests
extra_tests/snippets/builtin_list.py
Added TrackComparison helper and tests asserting sorted()/list.sort() use __lt__ (not __gt__), including reverse=True cases; updated assertion formatting.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant PySSLSocket
    participant Socket
    participant TlsConnection
    participant VM

    Client->>Socket: send data (may be TLS record or cleartext)
    PySSLSocket->>Socket: sock_peek(size)  -- MSG_PEEK
    Socket-->>PySSLSocket: peeked bytes
    PySSLSocket->>TlsConnection: analyze peeked bytes (record header)
    alt TLS record present
        PySSLSocket->>Socket: sock_recv(exact_record_len)
        Socket-->>PySSLSocket: TLS record bytes
        PySSLSocket->>TlsConnection: feed bytes, process_new_packets()
        TlsConnection-->>PySSLSocket: decrypted/plain data
        PySSLSocket->>VM: return decrypted data / handle close-notify
    else Post-TLS cleartext
        PySSLSocket-->>VM: treat peeked bytes as cleartext (do not consume TLS)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • ShaharNaveh

Poem

"I'm a rabbit with a curious peek,
I nibble bytes but never take a sweep—
Close-notify whispers, I listen right,
I pass along records, keep the rest light.
Hooray for sorts that choose lt to leap!"

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix ssl MSG_PEEK' directly corresponds to the primary change: implementing MSG_PEEK-based socket peeking for SSL/TLS shutdown handling.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

@github-actions
Copy link
Contributor

Code has been automatically formatted

The code in this PR has been formatted using:

  • cargo fmt --all
  • ruff format
    Please pull the latest changes before pushing again:
git pull origin fix-ssl

@youknowone youknowone marked this pull request as ready for review January 29, 2026 07:00
@youknowone youknowone merged commit e363b14 into RustPython:main Jan 29, 2026
11 of 13 checks passed
@youknowone youknowone deleted the fix-ssl branch January 29, 2026 07:48
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.

1 participant