diff --git a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs index 1509c15f4ac270..1543f75fa64aa1 100644 --- a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs +++ b/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() { diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs index 073e6b0fec5dda..bdc00a8958b07d 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs +++ b/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; } ///