Skip to content
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

prevent concurrent access to IO buffers on macOS #61723

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Nov 17, 2021

In general, the read and write path is separate and since we do not allow parallel reads or parallel writes there was no need to worry about concurrent access.
However, calling Dispose while handshake is happening can clear the buffers and we would get the Assert (in debug builds).

Process terminated. Assertion failed.
Expected 164 <= 0
   at System.Net.ArrayBuffer.Discard(Int32 byteCount) in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 67
   at System.Net.SafeDeleteSslContext.ReadPendingWrites() in /_/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs:line 259
   at System.Net.Security.SslStreamPal.HandshakeInternal(SafeFreeCredentials credential, SafeDeleteSslContext& context, ReadOnlySpan`1 inputBuffer, Byte[]& outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:line 263
   at System.Net.Security.SslStreamPal.InitializeSecurityContext(SafeFreeCredentials& credential, SafeDeleteSslContext& context, String targetName, ReadOnlySpan`1 inputBuffer, Byte[]& outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:line 52
   at System.Net.Security.SecureChannel.GenerateToken(ReadOnlySpan`1 inputBuffer, Byte[]& output) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 803

This change adds lock to prevent Dispose while in IO functions. (and vice versa)
We really only manipulate the buffers - there is no real IO under the lock.

I was not able to reproduce the Assert locally so this is based on code inspection.
I did ~ 100 runs (and running) and I did not see any side effects. e.g. all test are passing normally.

fixes #43686

@wfurt wfurt added this to the 7.0.0 milestone Nov 17, 2021
@wfurt wfurt requested a review from a team November 17, 2021 08:01
@wfurt wfurt self-assigned this Nov 17, 2021
@ghost
Copy link

ghost commented Nov 17, 2021

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

In general, the read and write path is separate and since we do not allow parallel reads or parallel writes there was no need to worry about concurrent access.
However, calling Dispose while handshake is happening can clear the buffers and we would get the Assert (in debug builds).

Process terminated. Assertion failed.
Expected 164 <= 0
   at System.Net.ArrayBuffer.Discard(Int32 byteCount) in /_/src/libraries/Common/src/System/Net/ArrayBuffer.cs:line 67
   at System.Net.SafeDeleteSslContext.ReadPendingWrites() in /_/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs:line 259
   at System.Net.Security.SslStreamPal.HandshakeInternal(SafeFreeCredentials credential, SafeDeleteSslContext& context, ReadOnlySpan`1 inputBuffer, Byte[]& outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:line 263
   at System.Net.Security.SslStreamPal.InitializeSecurityContext(SafeFreeCredentials& credential, SafeDeleteSslContext& context, String targetName, ReadOnlySpan`1 inputBuffer, Byte[]& outputBuffer, SslAuthenticationOptions sslAuthenticationOptions) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.OSX.cs:line 52
   at System.Net.Security.SecureChannel.GenerateToken(ReadOnlySpan`1 inputBuffer, Byte[]& output) in /_/src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs:line 803

This change adds lock to prevent Dispose while in IO functions. (and vice versa)
We really only manipulate the buffers - there is no real IO under the lock.

I was not able to reproduce the Assert locally so this is based on code inspection.
I did ~ 100 runs (and running) and I did not see any side effects. e.g. all test are passing normally.

fixes #43686

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, os-mac-os-x

Milestone: 7.0.0

Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Net test suites asserts in CI on ArrayBuffer.Discard
2 participants