-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 lock during SslStream renegotiation request #56470
Conversation
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsWhen server request renegotiation on client stream with pending data, the server trow exception as expected but lock is not released. This PR fix this behaviour and extend related test to cover this case. Close #56345
|
@@ -252,6 +252,11 @@ private async Task ReplyOnReAuthenticationAsync<TIOAdapter>(TIOAdapter adapter, | |||
private async Task RenegotiateAsync<TIOAdapter>(TIOAdapter adapter) | |||
where TIOAdapter : IReadWriteAdapter | |||
{ | |||
if (_decryptedBytesCount is not 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we allowed to read _decryptedBytesCount when we don't hold the locks?
Rather than moving this earlier, should it instead be moved down into the try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting it down would be safer. We try to prevent people from using this concurrently with IO so it should not be valid case but since it is easy I think it is ok to do little bit more work e.g. do the increments/decrements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the try block
0e899aa
to
2f3f500
Compare
@@ -323,7 +324,7 @@ private async Task RenegotiateAsync<TIOAdapter>(TIOAdapter adapter) | |||
_nestedRead = 0; | |||
_nestedWrite = 0; | |||
_isRenego = false; | |||
// We will not release _nestedAuth at this point to prevent another renegotiation attempt. | |||
_nestedAuth = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renegotiation should be done we don't need postpone clear this lock
} | ||
if (_decryptedBytesCount is not 0) | ||
{ | ||
_nestedAuth = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should clear it here. This was purposely left set in the finally so the RenegotiateAsync can be called at most once.
I think especially if we throw the rest would be unpredictable IMHO.
Also if we throw for _nestedRead or _nestedWrite this would still be set. I think that is ok as the intention is not to allow multiple calls but it would be inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I was tempted by wrong change in test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
iOSSimulator and tvOSSimulator tests are hanging, however this change is not for those platforms. Merging. |
When server request renegotiation on client stream with pending data, the server trow exception as expected but lock is not released.
This PR fix this behaviour and extend related test to cover this case.
Close #56345