Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tornado/iostream.py
Original file line number Diff line number Diff line change
Expand Up @@ -1256,6 +1256,9 @@ def start_tls(
ssl_stream._ssl_connect_future = future
ssl_stream.max_buffer_size = self.max_buffer_size
ssl_stream.read_chunk_size = self.read_chunk_size
# Keep a reference so callers can clean up if the handshake
# is abandoned (e.g. due to timeout). See tornado#3614.
self._tls_stream = ssl_stream
return future

def _handle_connect(self) -> None:
Expand Down
27 changes: 18 additions & 9 deletions tornado/tcpclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,17 +272,26 @@ async def connect(
# information here and re-use it on subsequent connections to
# the same host. (http://tools.ietf.org/html/rfc6555#section-4.2)
if ssl_options is not None:
tls_future = stream.start_tls(
False, ssl_options=ssl_options, server_hostname=host
)
if timeout is not None:
stream = await gen.with_timeout(
timeout,
stream.start_tls(
False, ssl_options=ssl_options, server_hostname=host
),
)
try:
stream = await gen.with_timeout(timeout, tls_future)
except gen.TimeoutError:
# Cancel the TLS handshake coroutine so it doesn't
# continue running with a stale _tls_stream reference.
tls_future.cancel()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need both tls_future.cancel and tls_stream.close? I think we'd only need tls_future.cancel if start_tls had a try/finally to do its own cleanup. Or, as I said in #3636 (comment), I think it might be cleaner to move timeout awareness into start_tls instead of trying to spread it across layers like this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — tls_future.cancel() is a no-op here since gen.with_timeout resolves the future with TimeoutError (doesn't cancel it). Removed it. The stream.close() is what actually prevents the fd leak.

I agree that moving timeout awareness into start_tls would be cleaner architecturally — that feels like a follow-up/refactor since it's a bigger change. Want me to do that in this PR or keep it scoped to the fix?

# start_tls() transferred socket ownership to a new
# SSLIOStream (self._tls_stream). Close it to prevent
# a file descriptor leak. See tornado#3614.
tls_stream = getattr(stream, "_tls_stream", None)
if tls_stream is not None:
tls_stream.close()
stream._tls_stream = None
raise
else:
stream = await stream.start_tls(
False, ssl_options=ssl_options, server_hostname=host
)
stream = await tls_future
return stream

def _create_stream(
Expand Down
61 changes: 61 additions & 0 deletions tornado/test/tcpclient_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
# License for the specific language governing permissions and limitations
# under the License.
import getpass
import os
import ssl as _ssl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just import ssl?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — changed import ssl as _ssl to import ssl to match the codebase convention (all other files use plain import ssl).

import socket
import typing
import unittest
Expand Down Expand Up @@ -428,3 +430,62 @@ def test_two_family_timeout_after_connect_timeout(self):
self.assertEqual(len(conn.streams), 1)
self.assert_connector_streams_closed(conn)
self.assertRaises(TimeoutError, future.result)


class TestTLSHandshakeTimeoutCleanup(AsyncTestCase):
"""Regression test: TLS handshake timeout must not leak file descriptors.

When TCPClient.connect() times out during the TLS handshake phase,
start_tls() has already transferred socket ownership to a new SSLIOStream.
The timeout handler must close that SSLIOStream to prevent an fd leak.
See tornado#3614.
"""

@skipIfNonUnix
@gen_test
def test_tls_handshake_timeout_closes_stream(self) -> None:
# Set up a server socket that accepts TCP but never completes TLS
server_sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
server_sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
server_sock.bind(("127.0.0.1", 0))
port = server_sock.getsockname()[1]
server_sock.listen(1)

ctx = _ssl.SSLContext(_ssl.PROTOCOL_TLS_CLIENT)
ctx.check_hostname = False
ctx.verify_mode = _ssl.CERT_NONE

# Count open fds before (Linux /proc/self/fd)
try:
fds_before = len(os.listdir("/proc/self/fd"))
except OSError:
fds_before = 0

client = TCPClient()
error_raised = False
try:
yield client.connect(
"127.0.0.1",
port,
ssl_options=ctx,
timeout=0.01,
)
except TimeoutError:
error_raised = True

self.assertTrue(error_raised, "Expected TimeoutError from TLS handshake timeout")

server_sock.close()

# Let IOLoop process cleanup callbacks
from tornado.gen import moment
yield moment

# Verify no fd leaked
if fds_before > 0:
fds_after = len(os.listdir("/proc/self/fd"))
self.assertLessEqual(
fds_after, fds_before + 2,
"File descriptor leaked after TLS handshake timeout "
f"(before={fds_before}, after={fds_after})",
)
Loading