From dae55227ff3c8dc909fe738aabfa05031be36ee0 Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 15 Mar 2021 09:45:34 -0700 Subject: [PATCH 1/2] delay disposal of array buffres if operation is in progress --- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 72 ++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index 06ecc8d9ed1d72..a4083b93230ca9 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -7,6 +7,7 @@ using System.Net.Security; using System.Security.Authentication; using System.Security.Cryptography.X509Certificates; +using System.Threading; using Microsoft.Win32.SafeHandles; namespace System.Net @@ -19,6 +20,8 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext private Interop.AppleCrypto.SSLWriteFunc _writeCallback; private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize); private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize); + private int _pendingInput; + private int _pendingOutput; public SafeSslHandle SslContext => _sslContext; @@ -144,8 +147,16 @@ protected override void Dispose(bool disposing) SafeSslHandle sslContext = _sslContext; if (null != sslContext) { - _inputBuffer.Dispose(); - _outputBuffer.Dispose(); + if (Interlocked.Exchange(ref _pendingInput, 2) == 0) + { + _inputBuffer.Dispose(); + } + + if (Interlocked.Exchange(ref _pendingOutput, 2) == 0) + { + _outputBuffer.Dispose(); + } + sslContext.Dispose(); } } @@ -158,6 +169,12 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe ulong length = (ulong)*dataLength; Debug.Assert(length <= int.MaxValue); + if (Interlocked.Exchange(ref _pendingOutput, 1) == 2) + { + const int writErr = -20; + return writErr; + } + int toWrite = (int)length; var inputBuffer = new ReadOnlySpan(data, toWrite); @@ -165,6 +182,11 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe inputBuffer.CopyTo(_outputBuffer.AvailableSpan); _outputBuffer.Commit(toWrite); + if (Interlocked.Exchange(ref _pendingOutput, 0) == 2) + { + _outputBuffer.Dispose(); + } + // Since we can enqueue everything, no need to re-assign *dataLength. const int noErr = 0; return noErr; @@ -181,10 +203,21 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL return noErr; } + if (Interlocked.Exchange(ref _pendingInput, 1) == 2) + { + const int readErr = -19; + return readErr; + } + uint transferred = 0; if (_inputBuffer.ActiveLength == 0) { + if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + { + _inputBuffer.Dispose(); + } + *dataLength = (void*)0; return errSSLWouldBlock; } @@ -195,21 +228,41 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL _inputBuffer.Discard(limit); transferred = (uint)limit; + if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + { + _inputBuffer.Dispose(); + } + *dataLength = (void*)transferred; return noErr; } internal void Write(ReadOnlySpan buf) { + if (Interlocked.Exchange(ref _pendingInput, 3) == 2) + { + throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); + } + _inputBuffer.EnsureAvailableSpace(buf.Length); buf.CopyTo(_inputBuffer.AvailableSpan); _inputBuffer.Commit(buf.Length); + + if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + { + _inputBuffer.Dispose(); + } } internal int BytesReadyForConnection => _outputBuffer.ActiveLength; internal byte[]? ReadPendingWrites() { + if (Interlocked.Exchange(ref _pendingOutput, 3) == 2) + { + throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); + } + if (_outputBuffer.ActiveLength == 0) { return null; @@ -218,6 +271,11 @@ internal void Write(ReadOnlySpan buf) byte[] buffer = _outputBuffer.ActiveSpan.ToArray(); _outputBuffer.Discard(_outputBuffer.ActiveLength); + if (Interlocked.Exchange(ref _pendingOutput, 0) == 2) + { + _outputBuffer.Dispose(); + } + return buffer; } @@ -228,11 +286,21 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count) Debug.Assert(count >= 0); Debug.Assert(count <= buf.Length - offset); + if (Interlocked.Exchange(ref _pendingOutput, 3) == 2) + { + throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); + } + int limit = Math.Min(count, _outputBuffer.ActiveLength); _outputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(buf, offset, limit)); _outputBuffer.Discard(limit); + if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + { + _outputBuffer.Dispose(); + } + return limit; } From f62248c8245aba58da0b6b3fad9ae55ca23f4d4c Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 15 Mar 2021 13:43:10 -0700 Subject: [PATCH 2/2] feedback from review --- .../Security/Pal.OSX/SafeDeleteSslContext.cs | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs index a4083b93230ca9..6b917e154a8fc3 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/Pal.OSX/SafeDeleteSslContext.cs @@ -20,6 +20,11 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext private Interop.AppleCrypto.SSLWriteFunc _writeCallback; private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize); private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize); + // state values for the _pendingXXX varaibles bellow + private const int NoIOPending = 0; + private const int NativeIOPending =1; + private const int ManagedIOPending = 2; + private const int Disposed = 3; private int _pendingInput; private int _pendingOutput; @@ -147,12 +152,12 @@ protected override void Dispose(bool disposing) SafeSslHandle sslContext = _sslContext; if (null != sslContext) { - if (Interlocked.Exchange(ref _pendingInput, 2) == 0) + if (Interlocked.Exchange(ref _pendingInput, Disposed) == NoIOPending) { _inputBuffer.Dispose(); } - if (Interlocked.Exchange(ref _pendingOutput, 2) == 0) + if (Interlocked.Exchange(ref _pendingOutput, Disposed) == NoIOPending) { _outputBuffer.Dispose(); } @@ -169,7 +174,7 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe ulong length = (ulong)*dataLength; Debug.Assert(length <= int.MaxValue); - if (Interlocked.Exchange(ref _pendingOutput, 1) == 2) + if (Interlocked.Exchange(ref _pendingOutput, NativeIOPending) == Disposed) { const int writErr = -20; return writErr; @@ -182,7 +187,7 @@ private unsafe int WriteToConnection(void* connection, byte* data, void** dataLe inputBuffer.CopyTo(_outputBuffer.AvailableSpan); _outputBuffer.Commit(toWrite); - if (Interlocked.Exchange(ref _pendingOutput, 0) == 2) + if (Interlocked.CompareExchange(ref _pendingOutput, NoIOPending, NativeIOPending) == Disposed) { _outputBuffer.Dispose(); } @@ -203,7 +208,7 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL return noErr; } - if (Interlocked.Exchange(ref _pendingInput, 1) == 2) + if (Interlocked.Exchange(ref _pendingInput, NativeIOPending) == Disposed) { const int readErr = -19; return readErr; @@ -213,7 +218,7 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL if (_inputBuffer.ActiveLength == 0) { - if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + if (Interlocked.Exchange(ref _pendingInput, NoIOPending) == Disposed) { _inputBuffer.Dispose(); } @@ -228,7 +233,7 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL _inputBuffer.Discard(limit); transferred = (uint)limit; - if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + if (Interlocked.Exchange(ref _pendingInput, NoIOPending) == Disposed) { _inputBuffer.Dispose(); } @@ -239,7 +244,7 @@ private unsafe int ReadFromConnection(void* connection, byte* data, void** dataL internal void Write(ReadOnlySpan buf) { - if (Interlocked.Exchange(ref _pendingInput, 3) == 2) + if (Interlocked.Exchange(ref _pendingInput, ManagedIOPending) == Disposed) { throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); } @@ -248,7 +253,7 @@ internal void Write(ReadOnlySpan buf) buf.CopyTo(_inputBuffer.AvailableSpan); _inputBuffer.Commit(buf.Length); - if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + if (Interlocked.CompareExchange(ref _pendingInput, NoIOPending, ManagedIOPending) == Disposed) { _inputBuffer.Dispose(); } @@ -258,20 +263,19 @@ internal void Write(ReadOnlySpan buf) internal byte[]? ReadPendingWrites() { - if (Interlocked.Exchange(ref _pendingOutput, 3) == 2) + if (Interlocked.Exchange(ref _pendingOutput, ManagedIOPending) == Disposed) { throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); } - if (_outputBuffer.ActiveLength == 0) + byte[]? buffer = null; + if (_outputBuffer.ActiveLength != 0) { - return null; + buffer = _outputBuffer.ActiveSpan.ToArray(); + _outputBuffer.Discard(_outputBuffer.ActiveLength); } - byte[] buffer = _outputBuffer.ActiveSpan.ToArray(); - _outputBuffer.Discard(_outputBuffer.ActiveLength); - - if (Interlocked.Exchange(ref _pendingOutput, 0) == 2) + if (Interlocked.CompareExchange(ref _pendingOutput, NoIOPending, ManagedIOPending) == Disposed) { _outputBuffer.Dispose(); } @@ -286,7 +290,7 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count) Debug.Assert(count >= 0); Debug.Assert(count <= buf.Length - offset); - if (Interlocked.Exchange(ref _pendingOutput, 3) == 2) + if (Interlocked.Exchange(ref _pendingOutput, ManagedIOPending) == Disposed) { throw new ObjectDisposedException(nameof(SafeDeleteSslContext)); } @@ -296,7 +300,7 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count) _outputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span(buf, offset, limit)); _outputBuffer.Discard(limit); - if (Interlocked.Exchange(ref _pendingInput, 0) == 2) + if (Interlocked.CompareExchange(ref _pendingInput, NoIOPending, ManagedIOPending) == Disposed) { _outputBuffer.Dispose(); }