New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix socket leak in TCPConnection/TCPConnectionSsl #2255
Merged
Merged
Conversation
This file contains 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
shaan1337
changed the title
Fix socket leak in TCP connections
Fix socket leak in TCPConnection/TCPConnectionSsl
Jan 27, 2020
shaan1337
force-pushed
the
fix-socket-leak
branch
from
January 28, 2020 05:40
bcf8716
to
6c300c9
Compare
…dd a callback to set its value in the ITcpConnection This ensures that when TcpConnection/Ssl.CloseInternal() is called, the socket is not null and will be properly disposed, preventing socket leaks.
shaan1337
force-pushed
the
fix-socket-leak
branch
from
January 28, 2020 06:38
6c300c9
to
ddd709d
Compare
…if connection is closed before being initialized which can result in null reference exceptions
… for the socket to be disposed at this point (we now call _socket.Dispose() in CloseInternal())
… trigger the race condition. Also verified that the regression test still fails when reverting the associated fix.
shaan1337
force-pushed
the
fix-socket-leak
branch
from
January 29, 2020 08:54
48d84f5
to
c977316
Compare
shaan1337
requested review from
jen20,
hayley-jean,
jageall,
Lougarou,
pgermishuys and
thefringeninja
January 29, 2020 10:08
mat-mcloughlin
removed request for
jen20,
pgermishuys,
Lougarou and
hayley-jean
January 29, 2020 11:14
thefringeninja
approved these changes
Jan 29, 2020
jageall
approved these changes
Jan 30, 2020
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.
Fixes #2010
Description of race condition
It is possible for TcpConnection.Close() -> CloseInternal() to be called before the
onConnectionEstablished
callback is called. Thus_socket
will still benull
(sinceconnection.InitSocket
has not been called yet) and the socket will not be closed due to the null check below, resulting in a socket leak:Normal TCP connection / CloseInternal():
As mentioned on issue #2010, this results in connections staying in the
CLOSE_WAIT
state until the client holding the socket is stopped.The same applies to SSL TCP connections, except that in this case,
_sslStream
will still benull
and the stream will not be disposed:SSL TCP connection / CloseInternal():
Reproduction steps
The following steps mirror what @Zetanova reported in #2010:
dotnet run
ss|grep 1113|grep "CLOSE-WAIT"|wc -l
. You will notice that the count keeps on increasing.Regression tests
4 regression tests have been added, one for each connection type: TcpConnection/TcpConnectionSsl (client & server). All of them fail when the fix is not present.
Fix description
The fix adds a callback called
onSocketAssigned
which is triggered as soon as the socket is created and before it is used to connect usingConnectAsync()
. This ensures that_socket
inTcpConnection
will be properly assigned before the createdITcpConnection
object is returned to the caller and thus ifClose()
is called on theITcpConnection
at any point after this,_socket
is guaranteed to be not null which in turn implies that the socket will properly be disposed inCloseInternal
.The same applies to
TcpConnectionSsl
except that there was no_socket
property (there is an_sslStream
property instead). A_socket
property has been added to ensure that the socket is disposed if the connection is closed before theonConnectionEstablished
callback is called (in this case,_sslStream
will still be null which would previously also result in a socket leak)Consequences of fixing the socket leak
Fixing the socket leak now implies that when
onConnectionEstablished
is called, the socket may have already been disposed. Thus, some new exceptions can now be thrown and these have been fixed in commits: e90c3b5, e7e8eff and 3bc0187.As a consequence of the fix,
onConnectionFailed
callback can now also be triggered inno_data_should_be_dispatched_after_tcp_connection_closed
. This has been fixed in commit: 032efe2.