From b5d293a8192f6626c594762f75826996b66ded50 Mon Sep 17 00:00:00 2001 From: wfurt Date: Wed, 26 Feb 2020 17:27:56 -0800 Subject: [PATCH 1/6] initial locking --- .../Interop.OpenSsl.cs | 36 ++- .../SslStream.Implementation.Adapters.cs | 14 - .../Net/Security/SslStream.Implementation.cs | 244 +++--------------- 3 files changed, 51 insertions(+), 243 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 75aa0d4aa8f60b..8aa069d43d6c1b 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -332,14 +332,11 @@ internal static int Encrypt(SafeSslHandle context, ReadOnlySpan input, ref int retVal; Exception innerError = null; - lock (context) - { - retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length); + retVal = Ssl.SslWrite(context, ref MemoryMarshal.GetReference(input), input.Length); - if (retVal != input.Length) - { - errorCode = GetSslError(context, retVal, out innerError); - } + if (retVal != input.Length) + { + errorCode = GetSslError(context, retVal, out innerError); } if (retVal != input.Length) @@ -389,30 +386,27 @@ internal static int Decrypt(SafeSslHandle context, byte[] outBuffer, int offset, int retVal = BioWrite(context.InputBio, outBuffer, offset, count); Exception innerError = null; - lock (context) + if (retVal == count) { - if (retVal == count) + unsafe { - unsafe + fixed (byte* fixedBuffer = outBuffer) { - fixed (byte* fixedBuffer = outBuffer) - { - retVal = Ssl.SslRead(context, fixedBuffer + offset, outBuffer.Length); - } - } - - if (retVal > 0) - { - count = retVal; + retVal = Ssl.SslRead(context, fixedBuffer + offset, outBuffer.Length); } } - if (retVal != count) + if (retVal > 0) { - errorCode = GetSslError(context, retVal, out innerError); + count = retVal; } } + if (retVal != count) + { + errorCode = GetSslError(context, retVal, out innerError); + } + if (retVal != count) { retVal = 0; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs index 982837e7fc93d9..beaff1c859b0be 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs @@ -12,8 +12,6 @@ public partial class SslStream private interface ISslIOAdapter { ValueTask ReadAsync(Memory buffer); - ValueTask ReadLockAsync(Memory buffer); - Task WriteLockAsync(); ValueTask WriteAsync(byte[] buffer, int offset, int count); CancellationToken CancellationToken { get; } } @@ -31,10 +29,6 @@ public AsyncSslIOAdapter(SslStream sslStream, CancellationToken cancellationToke public ValueTask ReadAsync(Memory buffer) => _sslStream.InnerStream.ReadAsync(buffer, _cancellationToken); - public ValueTask ReadLockAsync(Memory buffer) => _sslStream.CheckEnqueueReadAsync(buffer); - - public Task WriteLockAsync() => _sslStream.CheckEnqueueWriteAsync(); - public ValueTask WriteAsync(byte[] buffer, int offset, int count) => _sslStream.InnerStream.WriteAsync(new ReadOnlyMemory(buffer, offset, count), _cancellationToken); public CancellationToken CancellationToken => _cancellationToken; @@ -48,20 +42,12 @@ public AsyncSslIOAdapter(SslStream sslStream, CancellationToken cancellationToke public ValueTask ReadAsync(Memory buffer) => new ValueTask(_sslStream.InnerStream.Read(buffer.Span)); - public ValueTask ReadLockAsync(Memory buffer) => new ValueTask(_sslStream.CheckEnqueueRead(buffer)); - public ValueTask WriteAsync(byte[] buffer, int offset, int count) { _sslStream.InnerStream.Write(buffer, offset, count); return default; } - public Task WriteLockAsync() - { - _sslStream.CheckEnqueueWrite(); - return Task.CompletedTask; - } - public CancellationToken CancellationToken => default; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index af703e0481be66..88b6b2551ee22f 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -43,24 +43,14 @@ private enum FrameType : byte AppData = 23 } - // - // This block is used to rule the >>re-handshakes<< that are concurrent with read/write I/O requests. - // - private const int LockNone = 0; - private const int LockWrite = 1; - private const int LockHandshake = 2; - private const int LockPendingWrite = 3; - private const int LockRead = 4; - private const int LockPendingRead = 6; + private object _handshakeLock = new object(); + private TaskCompletionSource _handshakeWaiter = null; private const int FrameOverhead = 32; private const int ReadBufferSize = 4096 * 4 + FrameOverhead; // We read in 16K chunks + headers. private const int InitialHandshakeBufferSize = 4096 + FrameOverhead; // try to fit at least 4K ServerCertificate private ArrayBuffer _handshakeBuffer; - private int _lockWriteState; - private int _lockReadState; - private void ValidateCreateContext(SslClientAuthenticationOptions sslClientAuthenticationOptions, RemoteCertValidationCallback remoteCallback, LocalCertSelectionCallback localCallback) { ThrowIfExceptional(); @@ -175,7 +165,29 @@ private void CloseInternal() private SecurityStatusPal EncryptData(ReadOnlyMemory buffer, ref byte[] outBuffer, out int outSize) { ThrowIfExceptionalOrNotAuthenticated(); - return _context.Encrypt(buffer, ref outBuffer, out outSize); + + TaskCompletionSource waiter = null; + while (true) + { + if (waiter != null) + { + waiter.Task.Wait(); + _handshakeWaiter = null; + } + + lock (_handshakeLock) + { + waiter = _handshakeWaiter; + if (_handshakeWaiter != null) + { + // avoid waiting under lock. + continue; + } + + var ret = _context.Encrypt(buffer, ref outBuffer, out outSize); + return ret; + } + } } private SecurityStatusPal DecryptData() @@ -217,14 +229,7 @@ private Task ProcessAuthentication(bool isAsync = false, bool isApm = false, Can private async Task ReplyOnReAuthenticationAsync(TIOAdapter adapter, byte[] buffer) where TIOAdapter : ISslIOAdapter { - lock (SyncLock) - { - // Note we are already inside the read, so checking for already going concurrent handshake. - _lockReadState = LockHandshake; - } - await ForceAuthenticationAsync(adapter, receiveFirst: false, buffer).ConfigureAwait(false); - FinishHandshakeRead(LockNone); } // reAuthenticationData is only used on Windows in case of renegotiation. @@ -395,169 +400,6 @@ private bool CompleteHandshake(ref ProtocolToken alertToken) return true; } - private void FinishHandshakeRead(int newState) - { - lock (SyncLock) - { - // Lock is redundant here. Included for clarity. - int lockState = Interlocked.Exchange(ref _lockReadState, newState); - - if (lockState != LockPendingRead) - { - return; - } - - _lockReadState = LockRead; - } - } - - // Returns: - // -1 - proceed - // 0 - queued - // X - some bytes are ready, no need for IO - private int CheckEnqueueRead(Memory buffer) - { - ThrowIfExceptionalOrNotAuthenticated(); - - int lockState = Interlocked.CompareExchange(ref _lockReadState, LockRead, LockNone); - if (lockState != LockHandshake) - { - // Proceed, no concurrent handshake is ongoing so no need for a lock. - return -1; - } - - LazyAsyncResult lazyResult = null; - lock (SyncLock) - { - // Check again under lock. - if (_lockReadState != LockHandshake) - { - // The other thread has finished before we grabbed the lock. - _lockReadState = LockRead; - return -1; - } - - _lockReadState = LockPendingRead; - } - // Need to exit from lock before waiting. - lazyResult.InternalWaitForCompletion(); - ThrowIfExceptionalOrNotAuthenticated(); - return -1; - } - - private ValueTask CheckEnqueueReadAsync(Memory buffer) - { - ThrowIfExceptionalOrNotAuthenticated(); - - int lockState = Interlocked.CompareExchange(ref _lockReadState, LockRead, LockNone); - if (lockState != LockHandshake) - { - // Proceed, no concurrent handshake is ongoing so no need for a lock. - return new ValueTask(-1); - } - - lock (SyncLock) - { - // Check again under lock. - if (_lockReadState != LockHandshake) - { - // The other thread has finished before we grabbed the lock. - _lockReadState = LockRead; - return new ValueTask(-1); - } - - _lockReadState = LockPendingRead; - TaskCompletionSource taskCompletionSource = new TaskCompletionSource(buffer, TaskCreationOptions.RunContinuationsAsynchronously); - return new ValueTask(taskCompletionSource.Task); - } - } - - private Task CheckEnqueueWriteAsync() - { - ThrowIfExceptionalOrNotAuthenticated(); - - // Clear previous request. - int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockWrite, LockNone); - if (lockState != LockHandshake) - { - return Task.CompletedTask; - } - - lock (SyncLock) - { - if (_lockWriteState != LockHandshake) - { - ThrowIfExceptionalOrNotAuthenticated(); - return Task.CompletedTask; - } - - _lockWriteState = LockPendingWrite; - TaskCompletionSource completionSource = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - return completionSource.Task; - } - } - - private void CheckEnqueueWrite() - { - // Clear previous request. - int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockWrite, LockNone); - if (lockState != LockHandshake) - { - // Proceed with write. - return; - } - - LazyAsyncResult lazyResult = null; - lock (SyncLock) - { - if (_lockWriteState != LockHandshake) - { - // Handshake has completed before we grabbed the lock. - ThrowIfExceptionalOrNotAuthenticated(); - return; - } - - _lockWriteState = LockPendingWrite; - } - - // Need to exit from lock before waiting. - lazyResult.InternalWaitForCompletion(); - ThrowIfExceptionalOrNotAuthenticated(); - return; - } - - private void FinishWrite() - { - int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockNone, LockWrite); - if (lockState != LockHandshake) - { - return; - } - } - - private void FinishHandshake(Exception e) - { - lock (SyncLock) - { - if (e != null) - { - SetException(e); - } - - // Release read if any. - FinishHandshakeRead(LockNone); - - // If there is a pending write we want to keep it's lock state. - int lockState = Interlocked.CompareExchange(ref _lockWriteState, LockNone, LockHandshake); - if (lockState != LockPendingWrite) - { - return; - } - - _lockWriteState = LockWrite; - } - } - private async ValueTask WriteAsyncChunked(TIOAdapter writeAdapter, ReadOnlyMemory buffer) where TIOAdapter : struct, ISslIOAdapter { @@ -572,14 +414,6 @@ private async ValueTask WriteAsyncChunked(TIOAdapter writeAdapter, R private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnlyMemory buffer) where TIOAdapter : struct, ISslIOAdapter { - // Request a write IO slot. - Task ioSlot = writeAdapter.WriteLockAsync(); - if (!ioSlot.IsCompletedSuccessfully) - { - // Operation is async and has been queued, return. - return WaitForWriteIOSlot(writeAdapter, ioSlot, buffer); - } - byte[] rentedBuffer = ArrayPool.Shared.Rent(buffer.Length + FrameOverhead); byte[] outBuffer = rentedBuffer; @@ -596,7 +430,6 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly if (t.IsCompletedSuccessfully) { ArrayPool.Shared.Return(rentedBuffer); - FinishWrite(); return t; } else @@ -604,12 +437,6 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly return CompleteAsync(t, rentedBuffer); } - async ValueTask WaitForWriteIOSlot(TIOAdapter wAdapter, Task lockTask, ReadOnlyMemory buff) - { - await lockTask.ConfigureAwait(false); - await WriteSingleChunk(wAdapter, buff).ConfigureAwait(false); - } - async ValueTask CompleteAsync(ValueTask writeTask, byte[] bufferToReturn) { try @@ -619,7 +446,6 @@ async ValueTask CompleteAsync(ValueTask writeTask, byte[] bufferToReturn) finally { ArrayPool.Shared.Return(bufferToReturn); - FinishWrite(); } } } @@ -687,12 +513,6 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M return CopyDecryptedData(buffer); } - int copyBytes = await adapter.ReadLockAsync(buffer).ConfigureAwait(false); - if (copyBytes > 0) - { - return copyBytes; - } - ResetReadBuffer(); // Read the next frame header. @@ -734,7 +554,17 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M // DecryptData will decrypt in-place and modify these to point to the actual decrypted data, which may be smaller. _decryptedBytesOffset = _internalOffset; _decryptedBytesCount = payloadBytes; - SecurityStatusPal status = DecryptData(); + + + SecurityStatusPal status; + lock (_handshakeLock) + { + status = DecryptData(); + if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) + { + _handshakeWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + } + } // Treat the bytes we just decrypted as consumed // Note, we won't do another buffer read until the decrypted bytes are processed @@ -887,8 +717,6 @@ private async ValueTask WriteAsyncInternal(TIOAdapter writeAdapter, } catch (Exception e) { - FinishWrite(); - if (e is IOException || (e is OperationCanceledException && writeAdapter.CancellationToken.IsCancellationRequested)) { throw; From 06bfbc5b211fd3f738ee066868fd4c67ecd4cfd8 Mon Sep 17 00:00:00 2001 From: wfurt Date: Thu, 27 Feb 2020 21:18:36 -0800 Subject: [PATCH 2/6] feedback from review --- .../src/System/Net/SecurityStatusPal.cs | 1 + .../SslStream.Implementation.Adapters.cs | 12 ++ .../Net/Security/SslStream.Implementation.cs | 112 ++++++++++++++---- 3 files changed, 99 insertions(+), 26 deletions(-) diff --git a/src/libraries/Common/src/System/Net/SecurityStatusPal.cs b/src/libraries/Common/src/System/Net/SecurityStatusPal.cs index 595aec0970aba1..d0f7b26c038d5e 100644 --- a/src/libraries/Common/src/System/Net/SecurityStatusPal.cs +++ b/src/libraries/Common/src/System/Net/SecurityStatusPal.cs @@ -35,6 +35,7 @@ internal enum SecurityStatusPalErrorCode ContextExpired, CredentialsNeeded, Renegotiate, + TryAgain, // Errors OutOfMemory, diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs index beaff1c859b0be..71efe1c99e98e6 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs @@ -13,6 +13,7 @@ private interface ISslIOAdapter { ValueTask ReadAsync(Memory buffer); ValueTask WriteAsync(byte[] buffer, int offset, int count); + ValueTask LockAsync(TaskCompletionSource waiter); CancellationToken CancellationToken { get; } } @@ -31,6 +32,11 @@ public AsyncSslIOAdapter(SslStream sslStream, CancellationToken cancellationToke public ValueTask WriteAsync(byte[] buffer, int offset, int count) => _sslStream.InnerStream.WriteAsync(new ReadOnlyMemory(buffer, offset, count), _cancellationToken); + public async ValueTask LockAsync(TaskCompletionSource waiter) + { + await waiter.Task.ConfigureAwait(false); + } + public CancellationToken CancellationToken => _cancellationToken; } @@ -48,6 +54,12 @@ public ValueTask WriteAsync(byte[] buffer, int offset, int count) return default; } + public ValueTask LockAsync(TaskCompletionSource waiter) + { + waiter.Task.Wait(); + return default; + } + public CancellationToken CancellationToken => default; } } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index f65256e64c47af..406aa93ca7d680 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -43,8 +43,8 @@ private enum FrameType : byte AppData = 23 } - private object _handshakeLock = new object(); - private TaskCompletionSource? _handshakeWaiter = null; + private readonly object _handshakeLock = new object(); + private TaskCompletionSource? _handshakeWaiter; private const int FrameOverhead = 32; private const int ReadBufferSize = 4096 * 4 + FrameOverhead; // We read in 16K chunks + headers. @@ -166,27 +166,17 @@ private SecurityStatusPal EncryptData(ReadOnlyMemory buffer, ref byte[] ou { ThrowIfExceptionalOrNotAuthenticated(); - TaskCompletionSource? waiter = null; - while (true) + outSize = 0; + + lock (_handshakeLock) { - if (waiter != null) + if (_handshakeWaiter != null) { - waiter.Task.Wait(); - _handshakeWaiter = null; + // avoid waiting under lock. + return new SecurityStatusPal(SecurityStatusPalErrorCode.TryAgain); } - lock (_handshakeLock) - { - waiter = _handshakeWaiter; - if (_handshakeWaiter != null) - { - // avoid waiting under lock. - continue; - } - - var ret = _context!.Encrypt(buffer, ref outBuffer, out outSize); - return ret; - } + return _context!.Encrypt(buffer, ref outBuffer, out outSize); } } @@ -230,7 +220,14 @@ private SecurityStatusPal PrivateDecryptData(byte[]? buffer, ref int offset, ref private async Task ReplyOnReAuthenticationAsync(TIOAdapter adapter, byte[]? buffer) where TIOAdapter : ISslIOAdapter { - await ForceAuthenticationAsync(adapter, receiveFirst: false, buffer).ConfigureAwait(false); + try + { + await ForceAuthenticationAsync(adapter, receiveFirst: false, buffer).ConfigureAwait(false); + } + finally + { + _handshakeWaiter?.SetResult(true); + } } // reAuthenticationData is only used on Windows in case of renegotiation. @@ -418,16 +415,43 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly byte[] rentedBuffer = ArrayPool.Shared.Rent(buffer.Length + FrameOverhead); byte[] outBuffer = rentedBuffer; - SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes); + SecurityStatusPal status; + int encryptedBytes; + ValueTask t; + while (true) + { + status = EncryptData(buffer, ref outBuffer, out encryptedBytes); + + // This should be rate when renegotiation happens exactly when we want to write. + if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) + { + TaskCompletionSource? waiter = _handshakeWaiter; + if (waiter != null) + { + t = writeAdapter.LockAsync(waiter); + // We finished synchronously waiting for renegotiation. Try it again. + if (t.IsCompletedSuccessfully) + { + continue; + } + + return CompleteLockAndWriteAsync(t, rentedBuffer); + } + + continue; + // We are on Async path now. We need to wait for the Lock as well as for the write when it is finished. + } + + break; + } if (status.ErrorCode != SecurityStatusPalErrorCode.OK) { - // Re-handshake status is not supported. ArrayPool.Shared.Return(rentedBuffer); return new ValueTask(Task.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new IOException(SR.net_io_encrypt, SslStreamPal.GetException(status))))); } - ValueTask t = writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); + t = writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); if (t.IsCompletedSuccessfully) { ArrayPool.Shared.Return(rentedBuffer); @@ -435,10 +459,47 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly } else { - return CompleteAsync(t, rentedBuffer); + return CompleteWriteAsync(t, rentedBuffer); } - async ValueTask CompleteAsync(ValueTask writeTask, byte[] bufferToReturn) + async ValueTask CompleteLockAndWriteAsync(ValueTask t, byte[] bufferToReturn) + { + SecurityStatusPal status; + int encryptedBytes; + + try + { + await t; + while (true) + { + status = EncryptData(buffer, ref outBuffer, out encryptedBytes); + if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) + { + TaskCompletionSource? waiter = _handshakeWaiter; + if (waiter != null) + { + await writeAdapter.LockAsync(waiter); + } + + continue; + } + else if (status.ErrorCode != SecurityStatusPalErrorCode.OK) + { + throw new IOException(SR.net_io_encrypt, SslStreamPal.GetException(status)); + } + + break; + } + + await writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); + } + finally + { + ArrayPool.Shared.Return(bufferToReturn); + } + } + + async ValueTask CompleteWriteAsync(ValueTask writeTask, byte[] bufferToReturn) { try { @@ -556,7 +617,6 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M _decryptedBytesOffset = _internalOffset; _decryptedBytesCount = payloadBytes; - SecurityStatusPal status; lock (_handshakeLock) { From b13ceb98768aa1e89674e3568099adeab676e668 Mon Sep 17 00:00:00 2001 From: wfurt Date: Thu, 27 Feb 2020 21:27:15 -0800 Subject: [PATCH 3/6] update _handshakeWaiter --- .../src/System/Net/Security/SslStream.Implementation.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index 406aa93ca7d680..cf863fc7b5ac25 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -227,6 +227,7 @@ private async Task ReplyOnReAuthenticationAsync(TIOAdapter adapter, finally { _handshakeWaiter?.SetResult(true); + _handshakeWaiter = null; } } From e583da8882f5d5b3c7e6d9c6b39121c632b8c650 Mon Sep 17 00:00:00 2001 From: wfurt Date: Fri, 28 Feb 2020 12:07:24 -0800 Subject: [PATCH 4/6] feedback from review --- .../SslStream.Implementation.Adapters.cs | 11 +++---- .../Net/Security/SslStream.Implementation.cs | 32 +++++++++---------- 2 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs index 71efe1c99e98e6..d53d78ee57e339 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs @@ -13,7 +13,7 @@ private interface ISslIOAdapter { ValueTask ReadAsync(Memory buffer); ValueTask WriteAsync(byte[] buffer, int offset, int count); - ValueTask LockAsync(TaskCompletionSource waiter); + Task WaitAsync (TaskCompletionSource waiter); CancellationToken CancellationToken { get; } } @@ -32,10 +32,7 @@ public AsyncSslIOAdapter(SslStream sslStream, CancellationToken cancellationToke public ValueTask WriteAsync(byte[] buffer, int offset, int count) => _sslStream.InnerStream.WriteAsync(new ReadOnlyMemory(buffer, offset, count), _cancellationToken); - public async ValueTask LockAsync(TaskCompletionSource waiter) - { - await waiter.Task.ConfigureAwait(false); - } + public Task WaitAsync(TaskCompletionSource waiter) => waiter.Task; public CancellationToken CancellationToken => _cancellationToken; } @@ -54,10 +51,10 @@ public ValueTask WriteAsync(byte[] buffer, int offset, int count) return default; } - public ValueTask LockAsync(TaskCompletionSource waiter) + public Task WaitAsync(TaskCompletionSource waiter) { waiter.Task.Wait(); - return default; + return Task.CompletedTask;; } public CancellationToken CancellationToken => default; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index cf863fc7b5ac25..ea7d9816487c3d 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -44,7 +44,7 @@ private enum FrameType : byte } private readonly object _handshakeLock = new object(); - private TaskCompletionSource? _handshakeWaiter; + private volatile TaskCompletionSource? _handshakeWaiter; private const int FrameOverhead = 32; private const int ReadBufferSize = 4096 * 4 + FrameOverhead; // We read in 16K chunks + headers. @@ -166,12 +166,11 @@ private SecurityStatusPal EncryptData(ReadOnlyMemory buffer, ref byte[] ou { ThrowIfExceptionalOrNotAuthenticated(); - outSize = 0; - lock (_handshakeLock) { if (_handshakeWaiter != null) { + outSize = 0; // avoid waiting under lock. return new SecurityStatusPal(SecurityStatusPalErrorCode.TryAgain); } @@ -418,25 +417,24 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly SecurityStatusPal status; int encryptedBytes; - ValueTask t; while (true) { status = EncryptData(buffer, ref outBuffer, out encryptedBytes); - // This should be rate when renegotiation happens exactly when we want to write. + // This should be rare, when renegotiation happens exactly when we want to write. if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) { TaskCompletionSource? waiter = _handshakeWaiter; if (waiter != null) { - t = writeAdapter.LockAsync(waiter); + Task waiterTask = writeAdapter.WaitAsync(waiter); // We finished synchronously waiting for renegotiation. Try it again. - if (t.IsCompletedSuccessfully) + if (waiterTask.IsCompletedSuccessfully) { continue; } - return CompleteLockAndWriteAsync(t, rentedBuffer); + return WaitAndWriteAsync(waiterTask, rentedBuffer); } continue; @@ -452,7 +450,7 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly return new ValueTask(Task.FromException(ExceptionDispatchInfo.SetCurrentStackTrace(new IOException(SR.net_io_encrypt, SslStreamPal.GetException(status))))); } - t = writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); + ValueTask t = writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); if (t.IsCompletedSuccessfully) { ArrayPool.Shared.Return(rentedBuffer); @@ -463,23 +461,20 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly return CompleteWriteAsync(t, rentedBuffer); } - async ValueTask CompleteLockAndWriteAsync(ValueTask t, byte[] bufferToReturn) + async ValueTask WaitAndWriteAsync(Task t, byte[] bufferToReturn) { - SecurityStatusPal status; - int encryptedBytes; - try { - await t; + await t.ConfigureAwait(false); while (true) { - status = EncryptData(buffer, ref outBuffer, out encryptedBytes); + SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes); if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) { TaskCompletionSource? waiter = _handshakeWaiter; if (waiter != null) { - await writeAdapter.LockAsync(waiter); + await writeAdapter.WaitAsync(waiter).ConfigureAwait(false); } continue; @@ -492,7 +487,7 @@ async ValueTask CompleteLockAndWriteAsync(ValueTask t, byte[] bufferToReturn) break; } - await writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes); + await writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes).ConfigureAwait(false); } finally { @@ -650,6 +645,9 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M { if (!_sslAuthenticationOptions!.AllowRenegotiation) { + _handshakeWaiter?.SetResult(false); + _handshakeWaiter = null; + if (NetEventSource.IsEnabled) NetEventSource.Fail(this, "Renegotiation was requested but it is disallowed"); throw new IOException(SR.net_ssl_io_renego); } From 6b5443968be03f5f05d9b3b560aa9432d74a7f5a Mon Sep 17 00:00:00 2001 From: wfurt Date: Tue, 10 Mar 2020 12:48:09 -0700 Subject: [PATCH 5/6] feedback from review --- .../SslStream.Implementation.Adapters.cs | 4 +- .../Net/Security/SslStream.Implementation.cs | 76 +++++++++---------- 2 files changed, 37 insertions(+), 43 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs index d53d78ee57e339..4995f3fd09bf0e 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.Adapters.cs @@ -13,7 +13,7 @@ private interface ISslIOAdapter { ValueTask ReadAsync(Memory buffer); ValueTask WriteAsync(byte[] buffer, int offset, int count); - Task WaitAsync (TaskCompletionSource waiter); + Task WaitAsync(TaskCompletionSource waiter); CancellationToken CancellationToken { get; } } @@ -54,7 +54,7 @@ public ValueTask WriteAsync(byte[] buffer, int offset, int count) public Task WaitAsync(TaskCompletionSource waiter) { waiter.Task.Wait(); - return Task.CompletedTask;; + return Task.CompletedTask; } public CancellationToken CancellationToken => default; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index ea7d9816487c3d..918d9442ed170d 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -225,7 +225,7 @@ private async Task ReplyOnReAuthenticationAsync(TIOAdapter adapter, } finally { - _handshakeWaiter?.SetResult(true); + _handshakeWaiter!.SetResult(true); _handshakeWaiter = null; } } @@ -421,27 +421,25 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly { status = EncryptData(buffer, ref outBuffer, out encryptedBytes); - // This should be rare, when renegotiation happens exactly when we want to write. - if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) + // TryAgain should be rare, when renegotiation happens exactly when we want to write. + if (status.ErrorCode != SecurityStatusPalErrorCode.TryAgain) { - TaskCompletionSource? waiter = _handshakeWaiter; - if (waiter != null) - { - Task waiterTask = writeAdapter.WaitAsync(waiter); - // We finished synchronously waiting for renegotiation. Try it again. - if (waiterTask.IsCompletedSuccessfully) - { - continue; - } + break; + } - return WaitAndWriteAsync(waiterTask, rentedBuffer); + TaskCompletionSource? waiter = _handshakeWaiter; + if (waiter != null) + { + Task waiterTask = writeAdapter.WaitAsync(waiter); + // We finished synchronously waiting for renegotiation. We can try again immediately. + if (waiterTask.IsCompletedSuccessfully) + { + continue; } - continue; - // We are on Async path now. We need to wait for the Lock as well as for the write when it is finished. + // We need to wait asynchronously as well as for the write when EncryptData is finished. + return WaitAndWriteAsync(writeAdapter, buffer, waiterTask, rentedBuffer); } - - break; } if (status.ErrorCode != SecurityStatusPalErrorCode.OK) @@ -461,37 +459,33 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly return CompleteWriteAsync(t, rentedBuffer); } - async ValueTask WaitAndWriteAsync(Task t, byte[] bufferToReturn) + async ValueTask WaitAndWriteAsync(TIOAdapter writeAdapter, ReadOnlyMemory buffer, Task waitTask, byte[] rentedBuffer) { + byte[] outBuffer = rentedBuffer; try { - await t.ConfigureAwait(false); - while (true) - { - SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes); - if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) - { - TaskCompletionSource? waiter = _handshakeWaiter; - if (waiter != null) - { - await writeAdapter.WaitAsync(waiter).ConfigureAwait(false); - } + // Wait for renegotiation to finish. + await waitTask.ConfigureAwait(false); - continue; - } - else if (status.ErrorCode != SecurityStatusPalErrorCode.OK) - { - throw new IOException(SR.net_io_encrypt, SslStreamPal.GetException(status)); - } - - break; + SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes); + if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) + { + // Call WriteSingleChunk() recursively to avoid code duplication. + // This should be extremely rare in cases when second renegotiation happens concurrently with Write. + await WriteSingleChunk(writeAdapter, buffer); + } + else if (status.ErrorCode == SecurityStatusPalErrorCode.OK) + { + await writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes).ConfigureAwait(false); + } + else + { + throw new IOException(SR.net_io_encrypt, SslStreamPal.GetException(status)); } - - await writeAdapter.WriteAsync(outBuffer, 0, encryptedBytes).ConfigureAwait(false); } finally { - ArrayPool.Shared.Return(bufferToReturn); + ArrayPool.Shared.Return(rentedBuffer); } } @@ -645,7 +639,7 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M { if (!_sslAuthenticationOptions!.AllowRenegotiation) { - _handshakeWaiter?.SetResult(false); + _handshakeWaiter!.SetResult(false); _handshakeWaiter = null; if (NetEventSource.IsEnabled) NetEventSource.Fail(this, "Renegotiation was requested but it is disallowed"); From fea08fc60f8bd540273329206642c504e480755f Mon Sep 17 00:00:00 2001 From: wfurt Date: Thu, 19 Mar 2020 22:39:57 -0700 Subject: [PATCH 6/6] feedback from review --- .../Net/Security/SslStream.Implementation.cs | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs index 918d9442ed170d..9c714992182952 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Implementation.cs @@ -461,6 +461,7 @@ private ValueTask WriteSingleChunk(TIOAdapter writeAdapter, ReadOnly async ValueTask WaitAndWriteAsync(TIOAdapter writeAdapter, ReadOnlyMemory buffer, Task waitTask, byte[] rentedBuffer) { + byte[]? bufferToReturn = rentedBuffer; byte[] outBuffer = rentedBuffer; try { @@ -470,9 +471,12 @@ async ValueTask WaitAndWriteAsync(TIOAdapter writeAdapter, ReadOnlyMemory SecurityStatusPal status = EncryptData(buffer, ref outBuffer, out int encryptedBytes); if (status.ErrorCode == SecurityStatusPalErrorCode.TryAgain) { + // No need to hold on the buffer any more. + ArrayPool.Shared.Return(bufferToReturn); + bufferToReturn = null; // Call WriteSingleChunk() recursively to avoid code duplication. // This should be extremely rare in cases when second renegotiation happens concurrently with Write. - await WriteSingleChunk(writeAdapter, buffer); + await WriteSingleChunk(writeAdapter, buffer).ConfigureAwait(false); } else if (status.ErrorCode == SecurityStatusPalErrorCode.OK) { @@ -485,7 +489,10 @@ async ValueTask WaitAndWriteAsync(TIOAdapter writeAdapter, ReadOnlyMemory } finally { - ArrayPool.Shared.Return(rentedBuffer); + if (bufferToReturn != null) + { + ArrayPool.Shared.Return(bufferToReturn); + } } } @@ -613,6 +620,17 @@ private async ValueTask ReadAsyncInternal(TIOAdapter adapter, M status = DecryptData(); if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate) { + // The status indicates that peer wants to renegotiate. (Windows only) + // In practice, there can be some other reasons too - like TLS1.3 session creation + // of alert handling. We need to pass the data to lsass and it is not safe to do parallel + // write any more as that can change TLS state and the EncryptData() can fail in strange ways. + + // To handle this we call DecryptData() under lock and we create TCS waiter. + // EncryptData() checks that under same lock and if it exist it will not call low-level crypto. + // Instead it will wait synchronously or asynchronously and it will try again after the wait. + // The result will be set when ReplyOnReAuthenticationAsync() is finished e.g. lsass business is over + // or if we bail to continue. If either one happen before EncryptData(), _handshakeWaiter will be set to null + // and EncryptData() will work normally e.g. no waiting, just exclusion with DecryptData() _handshakeWaiter = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); } }