Make SslStreamInternal to free resources with SslStream dispose. (#26666

)

* Make SslStreamInternal to free resources with SslStream dispose.

* Fix framework test.

* Respond to PR feedback.

src/System.Net.Security/src/System/Net/Security/SslState.cs

@@ -467,11 +467,9 @@ internal Task FlushAsync(CancellationToken cancellationToken)
         //
         internal void Close()
         {
-            _exception = ExceptionDispatchInfo.Capture(new ObjectDisposedException("SslStream"));
-            if (Context != null)
-            {
-                Context.Close();
-            }
+            _exception = ExceptionDispatchInfo.Capture(new ObjectDisposedException(nameof(SslStream)));
+            Context?.Close();
+            _secureStream?.Dispose();
         }
 
         internal SecurityStatusPal EncryptData(ReadOnlyMemory<byte> buffer, ref byte[] outBuffer, out int outSize)

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

@@ -14,7 +14,7 @@ namespace System.Net.Security
     //
     // This is a wrapping stream that does data encryption/decryption based on a successfully authenticated SSPI context.
     //
-    internal partial class SslStreamInternal
+    internal partial class SslStreamInternal : IDisposable
     {
         private const int FrameOverhead = 32;
         private const int ReadBufferSize = 4096 * 4 + FrameOverhead;         // We read in 16K chunks + headers.
@@ -57,10 +57,36 @@ private void ReturnReadBufferIfEmpty()
 
         ~SslStreamInternal()
         {
-            if (_internalBuffer != null)
+            Dispose(disposing: false);
+        }
+
+        public void Dispose()
+        {
+            Dispose(disposing: true);
+
+            if (_internalBuffer == null)
             {
-                ArrayPool<byte>.Shared.Return(_internalBuffer);
-                _internalBuffer = null;
+                // Suppress finalizer if the read buffer was returned.
+                GC.SuppressFinalize(this);
+            }
+        }
+
+        private void Dispose(bool disposing)
+        {
+            // Ensure a Read operation is not in progress,
+            // block potential reads since SslStream is disposing.
+            // This leaves the _nestedRead = 1, but that's ok, since
+            // subsequent Reads first check if the context is still available.
+            if (Interlocked.CompareExchange(ref _nestedRead, 1, 0) == 0)
+            {
+                byte[] buffer = _internalBuffer;
+                if (buffer != null)
+                {
+                    _internalBuffer = null;
+                    _internalBufferCount = 0;
+                    _internalOffset = 0;
+                    ArrayPool<byte>.Shared.Return(buffer);
+                }
             }
         }
 

src/System.Net.Security/tests/FunctionalTests/SslStreamStreamToStreamTest.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System.IO;
 using System.Linq;
 using System.Net.Test.Common;
 using System.Security.Authentication;
@@ -306,6 +307,51 @@ await serverSslStream.WriteAsync(new byte[] { 1 }, 0, 1)
             }
         }
 
+        [Fact]
+        public async Task SslStream_StreamToStream_Dispose_Throws()
+        {
+            VirtualNetwork network = new VirtualNetwork();
+            using (var clientStream = new VirtualNetworkStream(network, isServer: false))
+            using (var serverStream = new VirtualNetworkStream(network, isServer: true))
+            using (var clientSslStream = new SslStream(clientStream, false, AllowAnyServerCertificate))
+            {
+                var serverSslStream = new SslStream(serverStream);
+                await DoHandshake(clientSslStream, serverSslStream);
+
+                var serverBuffer = new byte[1];
+                Task serverReadTask = serverSslStream.ReadAsync(serverBuffer, 0, serverBuffer.Length);
+                await serverSslStream.WriteAsync(new byte[] { 1 }, 0, 1)
+                    .TimeoutAfter(TestConfiguration.PassingTestTimeoutMilliseconds);
+
+                // Shouldn't throw, the context is diposed now.
+                // Since the server read task is in progress, the read buffer is not returned to ArrayPool.
+                serverSslStream.Dispose();
+
+                // Read in client
+                var clientBuffer = new byte[1];
+                await clientSslStream.ReadAsync(clientBuffer, 0, clientBuffer.Length);
+                Assert.Equal(1, clientBuffer[0]);
+
+                await clientSslStream.WriteAsync(new byte[] { 2 }, 0, 1);
+
+                if (PlatformDetection.IsFullFramework)
+                {
+                    await Assert.ThrowsAsync<ObjectDisposedException>(() => serverReadTask);
+                }
+                else
+                {
+                    IOException serverException = await Assert.ThrowsAsync<IOException>(() => serverReadTask);
+                    Assert.IsType<ObjectDisposedException>(serverException.InnerException);
+                }
+
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => serverSslStream.ReadAsync(serverBuffer, 0, serverBuffer.Length));
+
+                // Now, there is no pending read, so the internal buffer will be returned to ArrayPool.
+                serverSslStream.Dispose();
+                await Assert.ThrowsAsync<ObjectDisposedException>(() => serverSslStream.ReadAsync(serverBuffer, 0, serverBuffer.Length));
+            }
+        }
+
         [Fact]
         public void SslStream_StreamToStream_Flush_Propagated()
         {