Skip to content

Release the in-flight SSLIOStream when its parent IOStream closes#3672

Open
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix-3614-ssl-handshake-timeout-leak
Open

Release the in-flight SSLIOStream when its parent IOStream closes#3672
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix-3614-ssl-handshake-timeout-leak

Conversation

@HrachShah

Copy link
Copy Markdown

Fixes #3614.

IOStream.start_tls transfers ownership of the underlying socket to a new SSLIOStream before the TLS handshake completes and returns a Future that resolves with that SSLIOStream. If a caller (typically TCPClient.connect with a timeout) gives up while the handshake is in flight, the original IOStream is the only reference still reachable, but its .socket is already None and close() on it is a no-op; the SSLIOStream (and the socket it owns) becomes unreachable and the file descriptor leaks permanently. gen.with_timeout callers are hit by this because with_timeout explicitly does not cancel the wrapped future.

This change:

  • Exposes the in-flight SSLIOStream on the original IOStream via _pending_ssl_stream and closes it from IOStream.close so its socket is released.
  • Adds a done callback on the start_tls future that just clears the back-reference (without closing) when the future eventually resolves with the now-owned SSLIOStream.
  • Clears _state in start_tls and tolerates a None socket in IOStream.close_fd so a post-start_tls close() does not raise AttributeError on the detached socket.
  • TCPClient.connect wraps the start_tls future in a try/except so that on TimeoutError the original stream is closed (and the SSLIOStream released via the new close path) before the exception is re-raised.

Regression test (tornado/test/tcpclient_test.py):

  • TCPClientSSLTimeoutTest.test_ssl_handshake_timeout_releases_socket stands up a listener that accepts but never speaks TLS, connects with ssl_options and a short timeout, and asserts that no ESTABLISHED TCP entry remains on the local port after the deadline fires. Without the fix, an ESTABLISHED entry is left behind and the test fails.

IOStream.start_tls transfers ownership of the underlying socket to a
new SSLIOStream before the TLS handshake completes, and returns a
Future that resolves with that SSLIOStream. If a caller (typically
TCPClient.connect with a timeout) gives up while the handshake is
in flight, the original IOStream is the only reference still
reachable, but its .socket is already None and close() on it is a
no-op; the SSLIOStream (and the socket it owns) becomes unreachable
and leaks permanently. A gen.with_timeout caller is hit by this
because with_timeout explicitly does not cancel the wrapped future.

Expose the in-flight SSLIOStream on the original IOStream via
_pending_ssl_stream, and close it from IOStream.close so its socket
is released. When the start_tls future eventually resolves, the
done callback clears the back-reference without closing the now-owned
SSLIOStream. Clear _state in start_tls and tolerate a None socket
in close_fd so a post-start_tls close() does not raise AttributeError
on the detached socket.

TCPClient.connect wraps the start_tls future in a try/except so that
on TimeoutError the original stream is closed (and the SSLIOStream
released via the new close path) before the exception is re-raised.

Regression test in tornado/test/tcpclient_test.py stands up a
listener that accepts but never speaks TLS, connects with
ssl_options and a short timeout, and asserts that no ESTABLISHED
TCP entry remains on the local port after the deadline fires.
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.

Socket leak in TCPClient.connect when TLS handshake times out

1 participant