SSLStream Fixing GC Hole (#24799)

* Fixing GC Hole

* Moved back to original pin location

* Added Finializer

* Make tests async

* Reacting to review
Putting back the init code in the cpp file for the out params (lost in a reset)

* Added active issue on the failure test

* React to review

* Added Active Issue for Linux success tests

4 files changed, 33 insertions, 16 deletions

diff --git a/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
index f3ee43be8d..7374827c24 100644
--- a/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
+++ b/src/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs
@@ -102,7 +102,7 @@ internal static partial class Interop
                     if (sslAuthenticationOptions.IsServer)
                     {
                         byte[] protos = Interop.Ssl.ConvertAlpnProtocolListToByteArray(sslAuthenticationOptions.ApplicationProtocols);
-                        sslAuthenticationOptions.AlpnProtocolsHandle = GCHandle.Alloc(protos);
+                        sslAuthenticationOptions.AlpnProtocolsHandle = GCHandle.Alloc(protos, GCHandleType.Pinned);
                         Interop.Ssl.SslCtxSetAlpnSelectCb(innerContext, s_alpnServerCallback, GCHandle.ToIntPtr(sslAuthenticationOptions.AlpnProtocolsHandle));
                     }
                     else
@@ -332,14 +332,16 @@ internal static partial class Interop
 
         private static unsafe int AlpnServerSelectCallback(IntPtr ssl, out IntPtr outp, out byte outlen, IntPtr inp, uint inlen, IntPtr arg)
         {
+            outp = IntPtr.Zero;
+            outlen = 0;
+                        
             GCHandle protocols = GCHandle.FromIntPtr(arg);
+            Debug.Assert(protocols.IsAllocated && protocols.Target != null);
+
             byte[] server = (byte[])protocols.Target;
 
-            fixed (byte* sp = server)
-            {
-                return Interop.Ssl.SslSelectNextProto(out outp, out outlen, (IntPtr)sp, (uint)server.Length, inp, inlen) == Interop.Ssl.OPENSSL_NPN_NEGOTIATED ?
-                    Interop.Ssl.SSL_TLSEXT_ERR_OK : Interop.Ssl.SSL_TLSEXT_ERR_NOACK;
-            }
+            return Ssl.SslSelectNextProto(out outp, out outlen, protocols.AddrOfPinnedObject(), (uint)server.Length, inp, inlen) == Ssl.OPENSSL_NPN_NEGOTIATED ?
+                    Ssl.SSL_TLSEXT_ERR_OK : Ssl.SSL_TLSEXT_ERR_NOACK;
         }
 
         private static void UpdateCAListFromRootStore(SafeSslContextHandle context)
diff --git a/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp b/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
index dc4602437d..d056e22ed0 100644
--- a/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
+++ b/src/Native/Unix/System.Security.Cryptography.Native/pal_ssl.cpp
@@ -570,5 +570,10 @@ extern "C" void CryptoNative_SslGet0AlpnSelected(SSL* ssl, const uint8_t** proto
     {
         SSL_get0_alpn_selected(ssl, protocol, len);
     }
+    else
 #endif
+    {
+        *protocol = nullptr;
+        *len = 0;
+    }
 }
diff --git a/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs b/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs
index 2045f66284..f3a8d0ac05 100644
--- a/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs
+++ b/src/System.Net.Security/src/System/Net/Security/SecureChannel.cs
@@ -183,6 +183,14 @@ namespace System.Net.Security
             _refreshCredentialNeeded = true;
         }
 
+        ~SecureChannel()
+        {
+            if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
+            {
+                _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
+            }
+        }
+
         internal void Close()
         {
             if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
@@ -199,6 +207,8 @@ namespace System.Net.Security
             {
                 _credentialsHandle.Dispose();
             }
+
+            GC.SuppressFinalize(this);
         }
 
         //
@@ -837,11 +847,6 @@ namespace System.Net.Security
             }
             finally
             {
-                if (_sslAuthenticationOptions.AlpnProtocolsHandle.IsAllocated)
-                {
-                    _sslAuthenticationOptions.AlpnProtocolsHandle.Free();
-                }
-
                 if (_refreshCredentialNeeded)
                 {
                     _refreshCredentialNeeded = false;
diff --git a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
index bcf10160df..68d7e1836e 100644
--- a/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
+++ b/src/System.Net.Security/tests/FunctionalTests/SslStreamAlpnTests.cs
@@ -81,13 +81,15 @@ namespace System.Net.Security.Tests
                 serverOptions.ServerCertificate = certificate;
                 serverOptions.RemoteCertificateValidationCallback = AllowAnyServerCertificate;
 
-                Assert.ThrowsAsync<InvalidOperationException>(() => { return client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); });
-                Assert.ThrowsAsync<InvalidOperationException>(() => { return server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); });
+                Task t1 = Assert.ThrowsAsync<InvalidOperationException>(() => client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None));
+                Task t2 = Assert.ThrowsAsync<InvalidOperationException>(() => server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None));
+
+                Assert.True(Task.WaitAll(new[] { t1, t2 }, TestConfiguration.PassingTestTimeoutMilliseconds));
             }
         }
 
         [Theory]
-        [ActiveIssue(24722, TestPlatforms.Linux)]
+        [ActiveIssue(24866, TestPlatforms.Linux)]
         [MemberData(nameof(Alpn_TestData))]
         public void SslStream_StreamToStream_Alpn_Success(List<SslApplicationProtocol> clientProtocols, List<SslApplicationProtocol> serverProtocols, SslApplicationProtocol expected)
         {
@@ -115,6 +117,7 @@ namespace System.Net.Security.Tests
         }
 
         [Fact]
+        [ActiveIssue(24853)]
         [PlatformSpecific(~TestPlatforms.OSX)]
         public void SslStream_StreamToStream_Alpn_NonMatchingProtocols_Fail()
         {
@@ -137,9 +140,11 @@ namespace System.Net.Security.Tests
                     ApplicationProtocols = new List<SslApplicationProtocol> { SslApplicationProtocol.Http2 },
                     ServerCertificate = certificate,
                 };
+                
+                Task t1 = Assert.ThrowsAsync<AuthenticationException>(() => client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None));
+                Task t2 = Assert.ThrowsAsync<AuthenticationException>(() => server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None));
 
-                Assert.ThrowsAsync<AuthenticationException>(() => { return client.AuthenticateAsClientAsync(clientOptions, CancellationToken.None); });
-                Assert.ThrowsAsync<AuthenticationException>(() => { return server.AuthenticateAsServerAsync(serverOptions, CancellationToken.None); });
+                Assert.True(Task.WaitAll(new[] { t1, t2 }, TestConfiguration.PassingTestTimeoutMilliseconds));
             }
         }