TCPClient: close the in-flight SSLIOStream on TLS handshake timeout#3673
Open
HrachShah wants to merge 1 commit into
Open
TCPClient: close the in-flight SSLIOStream on TLS handshake timeout#3673HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
IOStream.start_tls transfers ownership of the underlying socket to a new SSLIOStream before the handshake completes and sets self.socket to None on the original stream. The future start_tls returns is the only caller-reachable reference to the new stream; the original stream's close() is a no-op on the None socket. If gen.with_timeout around start_tls fires, the original TimeoutError propagates to the caller and the new SSLIOStream is left in flight. The handshake may eventually fail (which closes the stream anyway) or it may succeed (which leaves the socket held by the SSLIOStream with no caller-side reference), so the socket fd leaks until GC. Expose the new SSLIOStream on the handshake future via future._ssl_stream so the timeout branch in TCPClient.connect can close it. TimeoutError is re-raised after the close so the caller's behaviour is unchanged. Regression test: monkey-patches IOStream.start_tls to return a pending future while recording the new SSLIOStream, runs TCPClient.connect with a short timeout, resolves the pending future, and asserts the recorded SSLIOStream had close() called on it.
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
1 participant
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.
Fixes #3614
When
TCPClient.connectis called with bothssl_optionsand atimeout, a TLS handshake timeout leaks the underlying socket.IOStream.start_tlstransfers socket ownership to a newSSLIOStreambefore the handshake completes, andgen.with_timeoutdeliberately leaves the wrapped future running on TimeoutError. The original stream'ssocketis alreadyNone, so itsclose()is a no-op, and the only caller-reachable reference to the in-flightSSLIOStream(which owns the real socket) is the inner future.This change makes
IOStream.start_tlskeep a back-reference from the returned future to the newSSLIOStream(future._ssl_stream) before returning.TCPClient.connectcaptures that future and, on TimeoutError, callsgetattr(handshake_future, "_ssl_stream", None)andclose()s the stream if present. If the handshake later fails,SSLIOStream's own failure path closes the socket so the late callback is a no-op; if the handshake eventually succeeds, the caller is already gone (TimeoutError was raised) and the now-owned stream is closed at the application layer as usual.TCPClientSSLTimeoutTest.test_tls_handshake_timeout_closes_ssl_streamintornado/test/tcpclient_test.pypatchesIOStream.start_tlswith a fake that does not actually drive the handshake (so the inner future stays pending), runsclient.connect(..., ssl_options=..., timeout=0.05), asserts the TimeoutError, resolves the inner future, and asserts that the patchedSSLIOStream.closewas called exactly once. The test fails on the pre-fix code (the SSL stream is not closed) and passes with the fix.