gh-142352: Fix start_tls() to transfer buffered data from StreamReader

[kasimov-maxim]

Fixes #142352

Summary

When using StreamWriter.start_tls() to upgrade a connection to TLS mid-stream, any data already buffered in the StreamReader was lost. This commonly occurs when implementing servers that support the PROXY protocol.

Changes

• Added _transfer_buffered_data_to_ssl() method to BaseEventLoop that transfers buffered data from StreamReader._buffer to SSLProtocol._incoming before the TLS handshake begins
• Added tests for the PROXY protocol + TLS scenario

Generative AI usage

Some parts of this PR were assisted by a generative AI tool. Specifically:

• help with drafting English-language comments and documentation
• suggestions for test refactoring
• assistance with wording and structuring the PR description

All functional logic and final edits were reviewed, verified, and authored by
me, in accordance with the Python devguide guidelines.

Test plan

• Added new tests in test_streams.py
• All asyncio tests pass
• make patchcheck passes

• Issue: asyncio: start_tls() loses buffered data from StreamReader #142352

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[kumaraditya303]

@ambv Can you review this? You reviewed the original PR which added this API #91453

[picnixz]

Has this PR been generated using an LLM? if so, indicate which places were so and read https://devguide.python.org/getting-started/generative-ai/.

[picnixz]

Remove docstrings from tests. It would be shown on failure. Use regular comments and shorten it. We don't want to restate the entire issue so simply link the issue's number as well.

[kasimov-maxim]

Understood, will update the tests accordingly.

[picnixz]

Do we really need this class? don't we already have some helpers elsewhere? The test also looks overly complex. Why not just patching existing TCPServer? (if possible; I don't know if there isn't some class that could be used as is with some minimal modifications)

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[picnixz]

Also, I think reading and checking if we're compliant with https://datatracker.ietf.org/doc/html/rfc2817 should be done (even if the RFC is only proposed and not in the standard tracks) or whatever RFC should be considered (I do think it's correct to fix this but I don't know if we should only send the possible hello for tls or generally write whatever was buffered)

[kasimov-maxim]

Has this PR been generated using an LLM? if so, indicate which places were so

Noted in PR.

[webknjaz]

I wonder if this could better belong @

cpython/Lib/asyncio/sslproto.py

Line 322 in b2ed3c3

self._incoming = ssl.MemoryBIO()

.

Technically, there could be something like

-         self._incoming = ssl.MemoryBIO()
+         self._incoming = ssl.MemoryBIO()
+         try:
+             underlying_raw_stream_buffer = app_protocol._stream_reader._buffer
+         except AttributeError:
+             pass
+         else:
+             self._incoming.write(underlying_raw_stream_buffer)
+             underlying_raw_stream_buffer.clear()

But I don't know how other places that initialize sslproto.SSLProtocol() would be affected. So it's just an idea to check out and see if other instances should have the same buffer transfer logic (looks like it's used in two other modules).

[kasimov-maxim]

@picnixz

I think reading and checking if we're compliant with https://datatracker.ietf.org/doc/html/rfc2817 should be done

RFC 2817 isn’t relevant here; it covers HTTP/1.1 Upgrade rather than asyncio.start_tls(). At this point we’ve already decided to switch to TLS, so any buffered bytes belong to the TLS layer and should be passed as‑is to the SSL BIO. If non‑TLS bytes are still buffered, that means start_tls() was called too early in the protocol, which is outside asyncio’s responsibility.

[kasimov-maxim]

I have made the requested changes; please review again

[bedevere-app]

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

[kasimov-maxim]

@fantix

Conclusion first: I think loop.start_tls() should take an extra parameter forward_handshake_data: bytes | None instead of actively looking into the protocol.

I agree that explicitly passing the bytes to be forwarded is the cleanest model, since only the application knows its buffer semantics. A parameter like forward_handshake_data: bytes | None (or shorter handshake_data: bytes | None) would make that explicit.
It’s also symmetric with cases where the application knows the required ordering (e.g., PROXY header first, then TLS), which the event loop cannot infer.

[kasimov-maxim]

@fantix

The root trigger of this bug is, as you stated in the bug report, "the TLS ClientHello (arrived) in the same TCP segment (as some application flag did)".

The new regression test (test_start_tls_buffered_data) shows the trigger is broader than “ClientHello in the same TCP segment as some application flag.” The issue is any buffered TLS data that reaches StreamReader before start_tls() is called. That can happen even without proxy‑protocol or segment coalescing.

[kumaraditya303]

Please resolve the Github comments once they are addressed, otherwise it makes reviewing difficult.

[fantix]

LGTM!

[miss-islington-app]

Thanks @kasimov-maxim for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @kasimov-maxim and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 0598f4a8999b96409e0a2bf9c480afc76a876860 3.13

[bedevere-app]

GH-145363 is a backport of this pull request to the 3.14 branch.

[bedevere-app]

GH-145364 is a backport of this pull request to the 3.13 branch.