Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle

src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs

@@ -1,12 +1,69 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Threading;
 using Xunit;
 
 namespace System.Security.Cryptography.OpenSsl.Tests
 {
     public static class SafeEvpPKeyHandleTests
     {
+        [Fact]
+        public static void DuplicateHandle_ConcurrentWithDispose_DoesNotProduceInvalidHandle()
+        {
+            using ECDsaOpenSsl ecdsa = new ECDsaOpenSsl();
+
+            for (int i = 0; i < 1000; i++)
+            {
+                SafeEvpPKeyHandle keyHandle = ecdsa.DuplicateKeyHandle();
+                SafeEvpPKeyHandle? duplicated = null;
+
+                Thread disposeThread = new Thread(() =>
+                {
+                    Thread.Sleep(Random.Shared.Next(0, 3));
+                    keyHandle.Dispose();
+                });
+
+                Thread duplicateThread = new Thread(() =>
+                {
+                    Thread.Sleep(Random.Shared.Next(0, 3));
+                    try
+                    {
+                        duplicated = keyHandle.DuplicateHandle();
+                    }
+                    catch
+                    {
+                        // We are only interested in crashes, not managed exceptions.
+                    }
+                });
+
+                disposeThread.Start();
+                duplicateThread.Start();
+                disposeThread.Join();
+                duplicateThread.Join();
+
+                if (duplicated is not null)
+                {
+                    bool refAdded = false;
+
+                    try
+                    {
+                        duplicated.DangerousAddRef(ref refAdded);
+                        Assert.NotEqual(IntPtr.Zero, duplicated.DangerousGetHandle());
+                    }
+                    finally
+                    {
+                        if (refAdded)
+                        {
+                            duplicated.DangerousRelease();
+                        }
+                    }
+
+                    duplicated.Dispose();
+                }
+            }
+        }
+
         [Fact]
         public static void TestOpenSslVersion()
         {

src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs

@@ -67,26 +67,43 @@ public SafeEvpPKeyHandle DuplicateHandle()
             if (IsInvalid)
                 throw new InvalidOperationException(SR.Cryptography_OpenInvalidHandle);
 
-            // Reliability: Allocate the SafeHandle before calling UpRefEvpPkey so
-            // that we don't lose a tracked reference in low-memory situations.
-            SafeEvpPKeyHandle safeHandle = new SafeEvpPKeyHandle();
+            // Keep the source handle alive so that a concurrent Dispose on another
+            // thread does not zero the handle field between UpRef and the copy below.
+            bool addedRef = false;
 
-            int success = Interop.Crypto.UpRefEvpPkey(this);
+            try
+            {
+                DangerousAddRef(ref addedRef);
+
+                // Reliability: Allocate the SafeHandle before calling UpRefEvpPkey so
+                // that we don't lose a tracked reference in low-memory situations.
+                SafeEvpPKeyHandle safeHandle = new SafeEvpPKeyHandle();
+
+                int success = Interop.Crypto.UpRefEvpPkey(this);
 
-            if (success != 1)
+                if (success != 1)
+                {
+                    Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction");
+                    Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
+                    safeHandle.Dispose();
+                    throw e;
+                }
+
+                // Since we didn't actually create a new handle, copy the handle
+                // to the new SafeHandle. DangerousAddRef prevents ReleaseHandle
+                // from being called, so handle and ExtraHandle are stable here.
+                safeHandle.SetHandle(handle);
+                // ExtraHandle is upref'd by UpRefEvpPkey
+                safeHandle.ExtraHandle = ExtraHandle;
+                return safeHandle;
+            }
+            finally
             {
-                Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction");
-                Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
-                safeHandle.Dispose();
-                throw e;
+                if (addedRef)
+                {
+                    DangerousRelease();
+                }
             }
-
-            // Since we didn't actually create a new handle, copy the handle
-            // to the new SafeHandle.
-            safeHandle.SetHandle(handle);
-            // ExtraHandle is upref'd by UpRefEvpPkey
-            safeHandle.ExtraHandle = ExtraHandle;
-            return safeHandle;
         }
 
         /// <summary>