From 82229cc83fb9d5636c96fee3aa29d72c945279db Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 19 Feb 2026 19:50:31 +0000 Subject: [PATCH 1/2] Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle --- .../tests/SafeEvpPKeyHandleTests.cs | 57 +++++++++++++++++++ .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 47 ++++++++++----- 2 files changed, 89 insertions(+), 15 deletions(-) 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 d8cb18f205ad8d..5a0dc25fa3b946 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 @@ -53,24 +53,41 @@ 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); - - if (success != 1) + try { - Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction"); - Exception e = Interop.Crypto.CreateOpenSslCryptographicException(); - safeHandle.Dispose(); - throw e; + 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) + { + 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); + return safeHandle; + } + finally + { + if (addedRef) + { + DangerousRelease(); + } } - - // Since we didn't actually create a new handle, copy the handle - // to the new SafeHandle. - safeHandle.SetHandle(handle); - return safeHandle; } /// From dc36a24367d7847be729c007b2e9bcdb37d00c43 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 19 Feb 2026 20:00:16 +0000 Subject: [PATCH 2/2] Remove "future" comment from backport --- .../System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 5a0dc25fa3b946..79cb7718796ade 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 @@ -77,7 +77,7 @@ public SafeEvpPKeyHandle DuplicateHandle() // 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. + // from being called, so handle is stable here. safeHandle.SetHandle(handle); return safeHandle; }