tcpclient: close the SSLIOStream on TLS handshake timeout so the socket is released#3684
Closed
HrachShah wants to merge 1 commit into
Closed
tcpclient: close the SSLIOStream on TLS handshake timeout so the socket is released#3684HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
When TCPClient.connect is given both ssl_options and a timeout, the handshake is raced against gen.with_timeout. The IOStream is wrapped in an SSLIOStream inside IOStream.start_tls, which transfers socket ownership away from the original stream before the handshake completes. gen.with_timeout does not cancel the inner future on timeout, so the SSLIOStream (now the only thing that holds the underlying socket) is left registered on the IOLoop with no reachable reference, and the socket file descriptor is leaked permanently. Stash a back-reference to the new SSLIOStream on the future returned by IOStream.start_tls, and in TCPClient.connect close the SSLIOStream when the timeout fires so the socket is released. Adds a regression test that runs a TLS handshake against a server that never speaks TLS and asserts no fd is leaked past the timeout. Closes tornadoweb#3614
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
TCPClient.connectis given bothssl_optionsand atimeout, the handshake is raced againstgen.with_timeout.IOStream.start_tlstransfers the underlying socket to a newSSLIOStreambefore the handshake completes and returns a future that resolves when the handshake does.gen.with_timeoutdoes not cancel its inner future on timeout, so theSSLIOStream(which now holds the only reference to the real socket) is left registered on theIOLoopwith no reachable reference, and the socket file descriptor is leaked permanently.This was reported in #3614. The reporter noticed the socket becoming unreachable from the caller and never closed, and worked around it by manually calling
io_loop.remove_handlerandstream.close()on the underlying socket before starting the handshake.Fix
IOStream.start_tlsstashes a back-reference to the newSSLIOStreamon the future it returns, so callers that race the handshake against a timeout can still reach the stream if the timeout fires.TCPClient.connectcatches thegen.TimeoutErrorfrom the wrappedgen.with_timeout, closes theSSLIOStream(which releases the underlying socket and unregisters the handshake handler), then re-raises.Test
Added
test_ssl_handshake_timeout_does_not_leak_socketintornado/test/tcpclient_test.py:do_handshake)TCPClient().connect(host, port, ssl_options=ctx, timeout=0.05)and expectsgen.TimeoutErrorWithout the fix, the assertion fails with
AssertionError: 11 != 10(one client socket leaked). With the fix, it passes.Verification
python -m tornado.test.runtests tornado.test.tcpclient_test-> 28 tests, OK (4 skipped for ipv6)python -m tornado.test.runtests tornado.test.iostream_test-> 142 tests, OK (coversTestIOStreamStartTLS)Closes #3614