From ba723399c481cdd1e139f60cded88c3b01e0f729 Mon Sep 17 00:00:00 2001 From: Zo Bot Date: Wed, 1 Jul 2026 09:38:44 +0000 Subject: [PATCH 1/2] TCPClient.connect: close the new SSLIOStream on TLS handshake timeout (issue #3614) When TCPClient.connect is called with both ssl_options and a timeout, a TLS handshake timeout leaks the underlying socket. IOStream.start_tls transfers socket ownership to a new SSLIOStream *before* the handshake completes and sets self.socket to None on the original stream. The gen.with_timeout call around start_tls only raises TimeoutError to the caller; the original stream is now a no-op (its socket is gone) and the new SSLIOStream that actually owns the socket is reachable only through the inner future -- which gen.with_timeout deliberately leaves running, per its docstring. Capture the future returned by start_tls so that, on timeout, we register a done-callback that closes the resulting SSLIOStream. If the handshake eventually fails, SSLIOStream already closes its socket on the failure path, so the callback only acts when the future resolved successfully. Regression test in tcpclient_test.py replaces start_tls with a stub that builds a real SSLIOStream around the underlying socket but never completes the handshake, instruments close() to record the stream, and asserts that the SSLIOStream is closed when the timeout fires and the future is subsequently resolved. All 28 existing tcpclient tests still pass. --- tornado/tcpclient.py | 37 +++++++++++++++++--- tornado/test/tcpclient_test.py | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/tornado/tcpclient.py b/tornado/tcpclient.py index 04a0c84f9..05c6708be 100644 --- a/tornado/tcpclient.py +++ b/tornado/tcpclient.py @@ -273,12 +273,39 @@ async def connect( # the same host. (http://tools.ietf.org/html/rfc6555#section-4.2) if ssl_options is not None: if timeout is not None: - stream = await gen.with_timeout( - timeout, - stream.start_tls( - False, ssl_options=ssl_options, server_hostname=host - ), + # Issue #3614: IOStream.start_tls transfers socket ownership + # to a new SSLIOStream *before* the handshake completes and + # sets self.socket to None on the original stream. If the + # TLS handshake times out, gen.with_timeout raises + # TimeoutError to the caller; the original stream is now a + # no-op (its socket is gone) and the new SSLIOStream that + # actually owns the socket is only reachable through the + # inner future -- which gen.with_timeout deliberately + # leaves running. Without an explicit close, the socket + # is leaked. + # + # Capture the future returned by start_tls so that, on + # timeout, we can close the SSLIOStream that owns the + # underlying socket. + tls_future = stream.start_tls( + False, ssl_options=ssl_options, server_hostname=host ) + try: + stream = await gen.with_timeout(timeout, tls_future) + except TimeoutError: + # The handshake is still pending (with_timeout does not + # cancel the wrapped future). When it eventually + # completes, the result is the SSLIOStream that owns + # the underlying socket -- closing it releases the fd. + # If the future ends up failing, the SSLIOStream + # already closes its socket as part of the failure + # path, so there is nothing to do. + def _close_if_succeeded(f: Future[IOStream]) -> None: + if f.exception() is None: + f.result().close() + + future_add_done_callback(tls_future, _close_if_succeeded) + raise else: stream = await stream.start_tls( False, ssl_options=ssl_options, server_hostname=host diff --git a/tornado/test/tcpclient_test.py b/tornado/test/tcpclient_test.py index ffe65d322..e9633ec9c 100644 --- a/tornado/test/tcpclient_test.py +++ b/tornado/test/tcpclient_test.py @@ -14,10 +14,12 @@ # under the License. import getpass import socket +import ssl import typing import unittest from contextlib import closing +from tornado import gen from tornado.concurrent import Future from tornado.gen import TimeoutError from tornado.iostream import IOStream @@ -173,6 +175,68 @@ def resolve(self, *args, **kwargs): "1.2.3.4", 12345, timeout=timeout ) + @gen_test + def test_tls_handshake_timeout_closes_ssl_stream(self): + # Regression test for issue #3614: when TCPClient.connect is + # called with both ssl_options and a timeout, a TLS handshake + # timeout must close the SSLIOStream that owns the underlying + # socket, not leak it. + port = self.start_server(socket.AF_INET) + + original_start_tls = IOStream.start_tls + closed_streams: list[IOStream] = [] + tls_future_holder: list[Future[IOStream]] = [] + + def fake_start_tls( + self: IOStream, + server_side: bool, + ssl_options: typing.Any = None, + server_hostname: typing.Any = None, + ) -> Future[IOStream]: + from tornado.iostream import SSLIOStream + + real_socket = self.socket + self.io_loop.remove_handler(real_socket) + self.socket = None # type: ignore[assignment] + ssl_stream = SSLIOStream(real_socket, ssl_options=ssl_options) + original = self._close_callback + if original is not None: + ssl_stream.set_close_callback(original) + real_close = ssl_stream.close + + def tracking_close(*args: typing.Any, **kwargs: typing.Any) -> None: + closed_streams.append(ssl_stream) + real_close(*args, **kwargs) + + ssl_stream.close = tracking_close # type: ignore[method-assign] + tls_future: Future[IOStream] = Future() + tls_future_holder.append(tls_future) + return tls_future + + IOStream.start_tls = fake_start_tls # type: ignore[method-assign] + try: + with self.assertRaises(TimeoutError): + yield self.client.connect( + "127.0.0.1", + port, + ssl_options=dict(cert_reqs=ssl.CERT_NONE), + timeout=0.05, + ) + # The handshake future is still pending. Resolve it so the + # cleanup callback registered by the timeout handler runs + # and closes the SSLIOStream that owns the socket. + self.assertEqual(len(tls_future_holder), 1) + tls_future_holder[0].set_result(None) # type: ignore[arg-type] + yield gen.sleep(0) + finally: + IOStream.start_tls = original_start_tls # type: ignore[method-assign] + self.assertEqual( + len(closed_streams), + 1, + "SSLIOStream owning the underlying socket was not closed on " + "TLS handshake timeout (issue #3614)", + ) + class TestConnectorSplit(unittest.TestCase): def test_one_family(self): From d339ea5588b683cef77dcd727944e7bffe4ba556 Mon Sep 17 00:00:00 2001 From: Zo Bot Date: Wed, 1 Jul 2026 09:43:08 +0000 Subject: [PATCH 2/2] send_error: skip the default error page for 204 No Content (issue #3360) send_error unconditionally called write_error(), which on the default RequestHandler writes a small HTML page like "204: No Content...". That buffer then propagated into the `_status_code in (204, 304) or (100 <= _status_code < 200)` assertion in finish(), raising "Cannot send body with 204" -- and worse, when run under `python -O` (which strips assertions) the client received a 204 response with a body and no Content-Length header, in violation of RFC 9110. Skip write_error for 204 (and keep the existing 304 skip) so the default branch produces a header-only 204 response. --- tornado/test/web_test.py | 21 +++++++++++++++++++++ tornado/web.py | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py index 36049e26b..de798c38d 100644 --- a/tornado/test/web_test.py +++ b/tornado/test/web_test.py @@ -1809,6 +1809,27 @@ def test_204_headers(self): self.assertNotIn("Transfer-Encoding", response.headers) +class HTTPError204Test(SimpleHandlerTestCase): + """Regression test for issue #3360: raising HTTPError(204) used to + route through send_error -> write_error -> default HTML body, then + trip the "Cannot send body with 204" assertion in finish() (or, under + ``python -O``, produce a malformed response with a body and no + Content-Length header). send_error must skip write_error for status + codes that must not carry a body. + """ + + class Handler(RequestHandler): + def get(self): + raise HTTPError(204) + + def test_http_error_204(self): + response = self.fetch("/") + self.assertEqual(response.code, 204) + self.assertNotIn("Content-Length", response.headers) + self.assertNotIn("Transfer-Encoding", response.headers) + self.assertEqual(response.body, b"") + + class Header304Test(SimpleHandlerTestCase): class Handler(RequestHandler): def get(self): diff --git a/tornado/web.py b/tornado/web.py index b572beac0..4b2b45bf2 100644 --- a/tornado/web.py +++ b/tornado/web.py @@ -1357,7 +1357,7 @@ def send_error(self, status_code: int = 500, **kwargs: Any) -> None: reason = exception.reason self.set_status(status_code, reason=reason) try: - if status_code != 304: + if status_code != 304 and status_code != 204: self.write_error(status_code, **kwargs) except Exception: app_log.error("Uncaught exception in write_error", exc_info=True)