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): 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)