improve SslStream exception after disposal (#79329)

wfurt · web-flow · commit 2a2745223e54 · 2023-01-05T21:27:55.000-08:00
* improve SslStream exception after disposal

* add tests

* add StreamUse

* fix cleanup

* fix condition

* avoid casting
diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs
@@ -54,10 +54,10 @@ private void CloseInternal()
 
             // Ensure a Read or Auth operation is not in progress,
             // block potential future read and auth operations since SslStream is disposing.
-            // This leaves the _nestedRead = 1 and _nestedAuth = 1, but that's ok, since
+            // This leaves the _nestedRead = 2 and _nestedAuth = 2, but that's ok, since
             // subsequent operations check the _exception sentinel first
-            if (Interlocked.Exchange(ref _nestedRead, 1) == 0 &&
-                Interlocked.Exchange(ref _nestedAuth, 1) == 0)
+            if (Interlocked.Exchange(ref _nestedRead, StreamDisposed) == StreamNotInUse &&
+                Interlocked.Exchange(ref _nestedAuth, StreamDisposed) == StreamNotInUse)
             {
                 _buffer.ReturnBuffer();
             }
@@ -162,19 +162,22 @@ private async Task ReplyOnReAuthenticationAsync<TIOAdapter>(byte[]? buffer, Canc
         private async Task RenegotiateAsync<TIOAdapter>(CancellationToken cancellationToken)
             where TIOAdapter : IReadWriteAdapter
         {
-            if (Interlocked.Exchange(ref _nestedAuth, 1) == 1)
+            if (Interlocked.CompareExchange(ref _nestedAuth, StreamInUse, StreamNotInUse) != StreamNotInUse)
             {
+                ObjectDisposedException.ThrowIf(_nestedAuth == StreamDisposed, this);
                 throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate"));
             }
 
-            if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
+            if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse)
             {
+                ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this);
                 throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
             }
 
-            if (Interlocked.Exchange(ref _nestedWrite, 1) == 1)
+            // Write is different since we do not do anything special in Dispose
+            if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) != StreamNotInUse)
             {
-                _nestedRead = 0;
+                _nestedRead = StreamNotInUse;
                 throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write"));
             }
 
@@ -231,8 +234,8 @@ private async Task RenegotiateAsync<TIOAdapter>(CancellationToken cancellationTo
                     _buffer.ReturnBuffer();
                 }
 
-                _nestedRead = 0;
-                _nestedWrite = 0;
+                _nestedRead = StreamNotInUse;
+                _nestedWrite = StreamNotInUse;
                 _isRenego = false;
                 // We will not release _nestedAuth at this point to prevent another renegotiation attempt.
             }
@@ -248,7 +251,7 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[
             if (reAuthenticationData == null)
             {
                 // prevent nesting only when authentication functions are called explicitly. e.g. handle renegotiation transparently.
-                if (Interlocked.Exchange(ref _nestedAuth, 1) == 1)
+                if (Interlocked.Exchange(ref _nestedAuth, StreamInUse) == StreamInUse)
                 {
                     throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate"));
                 }
@@ -335,7 +338,7 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[
             {
                 if (reAuthenticationData == null)
                 {
-                    _nestedAuth = 0;
+                    _nestedAuth = StreamNotInUse;
                     _isRenego = false;
                 }
             }
@@ -500,7 +503,7 @@ private bool CompleteHandshake(ref ProtocolToken? alertToken, out SslPolicyError
         {
             ProcessHandshakeSuccess();
 
-            if (_nestedAuth != 1)
+            if (_nestedAuth != StreamInUse)
             {
                 if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"Ignoring unsolicited renegotiated certificate.");
                 // ignore certificates received outside of handshake or requested renegotiation.
@@ -769,13 +772,16 @@ private SecurityStatusPal DecryptData(int frameSize)
         private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer, CancellationToken cancellationToken)
             where TIOAdapter : IReadWriteAdapter
         {
-            if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
+            // Throw first if we already have exception.
+            // Check for disposal is not atomic so we will check again below.
+            ThrowIfExceptionalOrNotAuthenticated();
+
+            if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse)
             {
+                ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this);
                 throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
             }
 
-            ThrowIfExceptionalOrNotAuthenticated();
-
             try
             {
                 int processedLength = 0;
@@ -910,7 +916,7 @@ private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer,
             finally
             {
                 ReturnReadBufferIfEmpty();
-                _nestedRead = 0;
+                _nestedRead = StreamNotInUse;
             }
         }
 
@@ -925,7 +931,7 @@ private async ValueTask WriteAsyncInternal<TIOAdapter>(ReadOnlyMemory<byte> buff
                 return;
             }
 
-            if (Interlocked.Exchange(ref _nestedWrite, 1) == 1)
+            if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) == StreamInUse)
             {
                 throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write"));
             }
@@ -948,7 +954,7 @@ private async ValueTask WriteAsyncInternal<TIOAdapter>(ReadOnlyMemory<byte> buff
             }
             finally
             {
-                _nestedWrite = 0;
+                _nestedWrite = StreamNotInUse;
             }
         }
 
diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs
@@ -170,6 +170,11 @@ public void ReturnBuffer()
             }
         }
 
+        // used to track ussage in _nested* variables bellow
+        private const int StreamNotInUse = 0;
+        private const int StreamInUse = 1;
+        private const int StreamDisposed = 2;
+
         private int _nestedWrite;
         private int _nestedRead;
 
@@ -703,7 +708,7 @@ public override async ValueTask DisposeAsync()
         public override int ReadByte()
         {
             ThrowIfExceptionalOrNotAuthenticated();
-            if (Interlocked.Exchange(ref _nestedRead, 1) == 1)
+            if (Interlocked.Exchange(ref _nestedRead, StreamInUse) == StreamInUse)
             {
                 throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));
             }
@@ -724,7 +729,7 @@ public override int ReadByte()
                 // Regardless of whether we were able to read a byte from the buffer,
                 // reset the read tracking.  If we weren't able to read a byte, the
                 // subsequent call to Read will set the flag again.
-                _nestedRead = 0;
+                _nestedRead = StreamNotInUse;
             }
 
             // Otherwise, fall back to reading a byte via Read, the same way Stream.ReadByte does.
diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamDisposeTest.cs
@@ -2,8 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.IO;
-using System.Net.Test.Common;
 using System.Security.Cryptography.X509Certificates;
+using System.Threading;
 using System.Threading.Tasks;
 
 using Xunit;
@@ -12,13 +12,13 @@ namespace System.Net.Security.Tests
 {
     using Configuration = System.Net.Test.Common.Configuration;
 
-    public abstract class SslStreamDisposeTest
+    public class SslStreamDisposeTest
     {
         [Fact]
         public async Task DisposeAsync_NotConnected_ClosesStream()
         {
             bool disposed = false;
-            var stream = new SslStream(new DelegateStream(disposeFunc: _ => disposed = true), false, delegate { return true; });
+            var stream = new SslStream(new DelegateStream(disposeFunc: _ => disposed = true, canReadFunc: () => true, canWriteFunc: () => true), false, delegate { return true; });
 
             Assert.False(disposed);
             await stream.DisposeAsync();
@@ -50,5 +50,57 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
             await serverStream.DisposeAsync();
             Assert.NotEqual(0, trackingStream2.TimesCalled(nameof(Stream.DisposeAsync)));
         }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task Dispose_PendingReadAsync_ThrowsODE(bool bufferedRead)
+        {
+            using CancellationTokenSource cts = new CancellationTokenSource();
+            cts.CancelAfter(TestConfiguration.PassingTestTimeout);
+
+            (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(leaveInnerStreamOpen: true);
+            using (client)
+            using (server)
+            using (X509Certificate2 serverCertificate = Configuration.Certificates.GetServerCertificate())
+            using (X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate())
+            {
+                SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions()
+                {
+                    TargetHost = Guid.NewGuid().ToString("N"),
+                };
+                clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true;
+
+                SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions()
+                {
+                    ServerCertificate = serverCertificate,
+                };
+
+                await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
+                                client.AuthenticateAsClientAsync(clientOptions, default),
+                                server.AuthenticateAsServerAsync(serverOptions, default));
+
+                await TestHelper.PingPong(client, server, cts.Token);
+
+                await server.WriteAsync("PINGPONG"u8.ToArray(), cts.Token);
+                var readBuffer = new byte[1024];
+
+                Task<int>? task = null;
+                if (bufferedRead)
+                {
+                    // This will read everything into internal buffer. Following ReadAsync will not need IO.
+                    task = client.ReadAsync(readBuffer, 0, 4, cts.Token);
+                    client.Dispose();
+                    int readLength = await task.ConfigureAwait(false);
+                    Assert.Equal(4, readLength);
+                }
+                else
+                {
+                    client.Dispose();
+                }
+
+                await Assert.ThrowsAnyAsync<ObjectDisposedException>(() => client.ReadAsync(readBuffer, cts.Token).AsTask());
+            }
+        }
     }
 }
diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/TestHelper.cs
@@ -51,10 +51,10 @@ public static bool AllowAnyServerCertificate(object sender, X509Certificate cert
             return true;
         }
 
-        public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams()
+        public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams(bool leaveInnerStreamOpen = false)
         {
             (Stream clientStream, Stream serverStream) = GetConnectedStreams();
-            return (new SslStream(clientStream), new SslStream(serverStream));
+            return (new SslStream(clientStream, leaveInnerStreamOpen), new SslStream(serverStream, leaveInnerStreamOpen));
         }
 
         public static (Stream ClientStream, Stream ServerStream) GetConnectedStreams()

Original file line number	Diff line number	Diff line change
`@@ -54,10 +54,10 @@ private void CloseInternal()`
`54`	`54`
`55`	`55`	`// Ensure a Read or Auth operation is not in progress,`
`56`	`56`	`// block potential future read and auth operations since SslStream is disposing.`
`57`		`- // This leaves the _nestedRead = 1 and _nestedAuth = 1, but that's ok, since`
	`57`	`+ // This leaves the _nestedRead = 2 and _nestedAuth = 2, but that's ok, since`
`58`	`58`	`// subsequent operations check the _exception sentinel first`
`59`		`- if (Interlocked.Exchange(ref _nestedRead, 1) == 0 &&`
`60`		`- Interlocked.Exchange(ref _nestedAuth, 1) == 0)`
	`59`	`+ if (Interlocked.Exchange(ref _nestedRead, StreamDisposed) == StreamNotInUse &&`
	`60`	`+ Interlocked.Exchange(ref _nestedAuth, StreamDisposed) == StreamNotInUse)`
`61`	`61`	`{`
`62`	`62`	`_buffer.ReturnBuffer();`
`63`	`63`	`}`
`@@ -162,19 +162,22 @@ private async Task ReplyOnReAuthenticationAsync<TIOAdapter>(byte[]? buffer, Canc`
`162`	`162`	`private async Task RenegotiateAsync<TIOAdapter>(CancellationToken cancellationToken)`
`163`	`163`	`where TIOAdapter : IReadWriteAdapter`
`164`	`164`	`{`
`165`		`- if (Interlocked.Exchange(ref _nestedAuth, 1) == 1)`
	`165`	`+ if (Interlocked.CompareExchange(ref _nestedAuth, StreamInUse, StreamNotInUse) != StreamNotInUse)`
`166`	`166`	`{`
	`167`	`+ ObjectDisposedException.ThrowIf(_nestedAuth == StreamDisposed, this);`
`167`	`168`	`throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate"));`
`168`	`169`	`}`
`169`	`170`
`170`		`- if (Interlocked.Exchange(ref _nestedRead, 1) == 1)`
	`171`	`+ if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse)`
`171`	`172`	`{`
	`173`	`+ ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this);`
`172`	`174`	`throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));`
`173`	`175`	`}`
`174`	`176`
`175`		`- if (Interlocked.Exchange(ref _nestedWrite, 1) == 1)`
	`177`	`+ // Write is different since we do not do anything special in Dispose`
	`178`	`+ if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) != StreamNotInUse)`
`176`	`179`	`{`
`177`		`- _nestedRead = 0;`
	`180`	`+ _nestedRead = StreamNotInUse;`
`178`	`181`	`throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write"));`
`179`	`182`	`}`
`180`	`183`
`@@ -231,8 +234,8 @@ private async Task RenegotiateAsync<TIOAdapter>(CancellationToken cancellationTo`
`231`	`234`	`_buffer.ReturnBuffer();`
`232`	`235`	`}`
`233`	`236`
`234`		`- _nestedRead = 0;`
`235`		`- _nestedWrite = 0;`
	`237`	`+ _nestedRead = StreamNotInUse;`
	`238`	`+ _nestedWrite = StreamNotInUse;`
`236`	`239`	`_isRenego = false;`
`237`	`240`	`// We will not release _nestedAuth at this point to prevent another renegotiation attempt.`
`238`	`241`	`}`
`@@ -248,7 +251,7 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[`
`248`	`251`	`if (reAuthenticationData == null)`
`249`	`252`	`{`
`250`	`253`	`// prevent nesting only when authentication functions are called explicitly. e.g. handle renegotiation transparently.`
`251`		`- if (Interlocked.Exchange(ref _nestedAuth, 1) == 1)`
	`254`	`+ if (Interlocked.Exchange(ref _nestedAuth, StreamInUse) == StreamInUse)`
`252`	`255`	`{`
`253`	`256`	`throw new InvalidOperationException(SR.Format(SR.net_io_invalidnestedcall, "authenticate"));`
`254`	`257`	`}`
`@@ -335,7 +338,7 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[`
`335`	`338`	`{`
`336`	`339`	`if (reAuthenticationData == null)`
`337`	`340`	`{`
`338`		`- _nestedAuth = 0;`
	`341`	`+ _nestedAuth = StreamNotInUse;`
`339`	`342`	`_isRenego = false;`
`340`	`343`	`}`
`341`	`344`	`}`
`@@ -500,7 +503,7 @@ private bool CompleteHandshake(ref ProtocolToken? alertToken, out SslPolicyError`
`500`	`503`	`{`
`501`	`504`	`ProcessHandshakeSuccess();`
`502`	`505`
`503`		`- if (_nestedAuth != 1)`
	`506`	`+ if (_nestedAuth != StreamInUse)`
`504`	`507`	`{`
`505`	`508`	`if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"Ignoring unsolicited renegotiated certificate.");`
`506`	`509`	`// ignore certificates received outside of handshake or requested renegotiation.`
`@@ -769,13 +772,16 @@ private SecurityStatusPal DecryptData(int frameSize)`
`769`	`772`	`private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer, CancellationToken cancellationToken)`
`770`	`773`	`where TIOAdapter : IReadWriteAdapter`
`771`	`774`	`{`
`772`		`- if (Interlocked.Exchange(ref _nestedRead, 1) == 1)`
	`775`	`+ // Throw first if we already have exception.`
	`776`	`+ // Check for disposal is not atomic so we will check again below.`
	`777`	`+ ThrowIfExceptionalOrNotAuthenticated();`
	`778`	`+`
	`779`	`+ if (Interlocked.CompareExchange(ref _nestedRead, StreamInUse, StreamNotInUse) != StreamNotInUse)`
`773`	`780`	`{`
	`781`	`+ ObjectDisposedException.ThrowIf(_nestedRead == StreamDisposed, this);`
`774`	`782`	`throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));`
`775`	`783`	`}`
`776`	`784`
`777`		`- ThrowIfExceptionalOrNotAuthenticated();`
`778`		`-`
`779`	`785`	`try`
`780`	`786`	`{`
`781`	`787`	`int processedLength = 0;`
`@@ -910,7 +916,7 @@ private async ValueTask<int> ReadAsyncInternal<TIOAdapter>(Memory<byte> buffer,`
`910`	`916`	`finally`
`911`	`917`	`{`
`912`	`918`	`ReturnReadBufferIfEmpty();`
`913`		`- _nestedRead = 0;`
	`919`	`+ _nestedRead = StreamNotInUse;`
`914`	`920`	`}`
`915`	`921`	`}`
`916`	`922`
`@@ -925,7 +931,7 @@ private async ValueTask WriteAsyncInternal<TIOAdapter>(ReadOnlyMemory<byte> buff`
`925`	`931`	`return;`
`926`	`932`	`}`
`927`	`933`
`928`		`- if (Interlocked.Exchange(ref _nestedWrite, 1) == 1)`
	`934`	`+ if (Interlocked.Exchange(ref _nestedWrite, StreamInUse) == StreamInUse)`
`929`	`935`	`{`
`930`	`936`	`throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "write"));`
`931`	`937`	`}`
`@@ -948,7 +954,7 @@ private async ValueTask WriteAsyncInternal<TIOAdapter>(ReadOnlyMemory<byte> buff`
`948`	`954`	`}`
`949`	`955`	`finally`
`950`	`956`	`{`
`951`		`- _nestedWrite = 0;`
	`957`	`+ _nestedWrite = StreamNotInUse;`
`952`	`958`	`}`
`953`	`959`	`}`
`954`	`960`
Original file line number	Diff line number	Diff line change
`@@ -170,6 +170,11 @@ public void ReturnBuffer()`
`170`	`170`	`}`
`171`	`171`	`}`
`172`	`172`
	`173`	`+ // used to track ussage in _nested* variables bellow`
	`174`	`+ private const int StreamNotInUse = 0;`
	`175`	`+ private const int StreamInUse = 1;`
	`176`	`+ private const int StreamDisposed = 2;`
	`177`	`+`
`173`	`178`	`private int _nestedWrite;`
`174`	`179`	`private int _nestedRead;`
`175`	`180`
`@@ -703,7 +708,7 @@ public override async ValueTask DisposeAsync()`
`703`	`708`	`public override int ReadByte()`
`704`	`709`	`{`
`705`	`710`	`ThrowIfExceptionalOrNotAuthenticated();`
`706`		`- if (Interlocked.Exchange(ref _nestedRead, 1) == 1)`
	`711`	`+ if (Interlocked.Exchange(ref _nestedRead, StreamInUse) == StreamInUse)`
`707`	`712`	`{`
`708`	`713`	`throw new NotSupportedException(SR.Format(SR.net_io_invalidnestedcall, "read"));`
`709`	`714`	`}`
`@@ -724,7 +729,7 @@ public override int ReadByte()`
`724`	`729`	`// Regardless of whether we were able to read a byte from the buffer,`
`725`	`730`	`// reset the read tracking. If we weren't able to read a byte, the`
`726`	`731`	`// subsequent call to Read will set the flag again.`
`727`		`- _nestedRead = 0;`
	`732`	`+ _nestedRead = StreamNotInUse;`
`728`	`733`	`}`
`729`	`734`
`730`	`735`	`// Otherwise, fall back to reading a byte via Read, the same way Stream.ReadByte does.`
Original file line number	Diff line number	Diff line change
`@@ -51,10 +51,10 @@ public static bool AllowAnyServerCertificate(object sender, X509Certificate cert`
`51`	`51`	`return true;`
`52`	`52`	`}`
`53`	`53`
`54`		`- public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams()`
	`54`	`+ public static (SslStream ClientStream, SslStream ServerStream) GetConnectedSslStreams(bool leaveInnerStreamOpen = false)`
`55`	`55`	`{`
`56`	`56`	`(Stream clientStream, Stream serverStream) = GetConnectedStreams();`
`57`		`- return (new SslStream(clientStream), new SslStream(serverStream));`
	`57`	`+ return (new SslStream(clientStream, leaveInnerStreamOpen), new SslStream(serverStream, leaveInnerStreamOpen));`
`58`	`58`	`}`
`59`	`59`
`60`	`60`	`public static (Stream ClientStream, Stream ServerStream) GetConnectedStreams()`