From 136850f1152971a5181e98f5b5c71122ebcb26bf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Feb 2026 22:43:39 +0000 Subject: [PATCH 1/8] Initial plan From 3ea87ead18aa53cb6935a706b410efb52363b942 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Feb 2026 23:10:00 +0000 Subject: [PATCH 2/8] Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle and add test Use DangerousAddRef/DangerousRelease to prevent the source handle from being disposed by another thread while DuplicateHandle reads the handle value. Capture handle and ExtraHandle values into local variables while the ref count is elevated, so a concurrent Dispose cannot zero them out before they are copied to the new SafeEvpPKeyHandle. Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../tests/SafeEvpPKeyHandleTests.cs | 50 +++++++++++++++++++ .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 45 ++++++++++++----- 2 files changed, 83 insertions(+), 12 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..bc0c71711acdad 100644 --- a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs +++ b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs @@ -1,12 +1,62 @@ // 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 { } + }); + + 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..ded8f14fe00461 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,25 +67,46 @@ 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(); - int success = Interop.Crypto.UpRefEvpPkey(this); + // 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; - if (success != 1) + try + { + DangerousAddRef(ref addedRef); + + IntPtr handleSnapshot = DangerousGetHandle(); + IntPtr extraHandleSnapshot = ExtraHandle; + + int uprefResult = Interop.Crypto.UpRefEvpPkey(this); + + if (uprefResult != 1) + { + Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction"); + throw Interop.Crypto.CreateOpenSslCryptographicException(); + } + + // Use the captured snapshots so that a concurrent Dispose + // cannot cause us to copy a zeroed-out handle value. + safeHandle.SetHandle(handleSnapshot); + // ExtraHandle is upref'd by UpRefEvpPkey + safeHandle.ExtraHandle = extraHandleSnapshot; + } + catch { - Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction"); - Exception e = Interop.Crypto.CreateOpenSslCryptographicException(); safeHandle.Dispose(); - throw e; + throw; + } + finally + { + 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; } From bffea02fd1a1c90bd36329674b33c9aa3dbb0e7e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Feb 2026 23:47:29 +0000 Subject: [PATCH 3/8] Address review: remove snapshot locals, restructure try/finally without catch Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) 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 ded8f14fe00461..93a3522097964e 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,6 +67,8 @@ 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 @@ -77,27 +79,22 @@ public SafeEvpPKeyHandle DuplicateHandle() { DangerousAddRef(ref addedRef); - IntPtr handleSnapshot = DangerousGetHandle(); - IntPtr extraHandleSnapshot = ExtraHandle; + int success = Interop.Crypto.UpRefEvpPkey(this); - int uprefResult = Interop.Crypto.UpRefEvpPkey(this); - - if (uprefResult != 1) + if (success != 1) { Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction"); - throw Interop.Crypto.CreateOpenSslCryptographicException(); + Exception e = Interop.Crypto.CreateOpenSslCryptographicException(); + safeHandle.Dispose(); + throw e; } - // Use the captured snapshots so that a concurrent Dispose - // cannot cause us to copy a zeroed-out handle value. - safeHandle.SetHandle(handleSnapshot); + // 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 = extraHandleSnapshot; - } - catch - { - safeHandle.Dispose(); - throw; + safeHandle.ExtraHandle = ExtraHandle; } finally { From 9f72b95344a17ae1478884a20fe12b5525953769 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Feb 2026 00:22:27 +0000 Subject: [PATCH 4/8] Simplify fix: capture handle into locals before UpRefEvpPkey, fix test formatting Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../tests/SafeEvpPKeyHandleTests.cs | 11 ++++- .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 43 +++++++------------ 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs index bc0c71711acdad..58006cea96848c 100644 --- a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs +++ b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs @@ -27,7 +27,14 @@ public static void DuplicateHandle_ConcurrentWithDispose_DoesNotProduceInvalidHa Thread duplicateThread = new Thread(() => { Thread.Sleep(Random.Shared.Next(0, 3)); - try { duplicated = keyHandle.DuplicateHandle(); } catch { } + try + { + duplicated = keyHandle.DuplicateHandle(); + } + catch + { + // We are only interested in crashes, not managed exceptions. + } }); disposeThread.Start(); @@ -35,6 +42,8 @@ public static void DuplicateHandle_ConcurrentWithDispose_DoesNotProduceInvalidHa disposeThread.Join(); duplicateThread.Join(); + keyHandle.Dispose(); + if (duplicated is not null) { bool refAdded = false; 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 93a3522097964e..6912ba3add3aef 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 @@ -71,39 +71,26 @@ public SafeEvpPKeyHandle DuplicateHandle() // 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; + // Capture handle and ExtraHandle before UpRefEvpPkey so that a concurrent + // Dispose cannot zero the fields between UpRef and the copy below. + IntPtr currentHandle = handle; + IntPtr currentExtraHandle = ExtraHandle; - try - { - DangerousAddRef(ref addedRef); - - int success = Interop.Crypto.UpRefEvpPkey(this); + 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); - // ExtraHandle is upref'd by UpRefEvpPkey - safeHandle.ExtraHandle = ExtraHandle; - } - finally + if (success != 1) { - if (addedRef) - { - DangerousRelease(); - } + 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. + safeHandle.SetHandle(currentHandle); + // ExtraHandle is upref'd by UpRefEvpPkey + safeHandle.ExtraHandle = currentExtraHandle; return safeHandle; } From 5c810a4bb07a61827fa6eaff7f201427ac8b0886 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Feb 2026 00:51:29 +0000 Subject: [PATCH 5/8] Revert to DangerousAddRef/DangerousRelease approach for DuplicateHandle Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) 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 6912ba3add3aef..93a3522097964e 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 @@ -71,26 +71,39 @@ public SafeEvpPKeyHandle DuplicateHandle() // that we don't lose a tracked reference in low-memory situations. SafeEvpPKeyHandle safeHandle = new SafeEvpPKeyHandle(); - // Capture handle and ExtraHandle before UpRefEvpPkey so that a concurrent - // Dispose cannot zero the fields between UpRef and the copy below. - IntPtr currentHandle = handle; - IntPtr currentExtraHandle = ExtraHandle; + // 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); + + 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; + } + 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(currentHandle); - // ExtraHandle is upref'd by UpRefEvpPkey - safeHandle.ExtraHandle = currentExtraHandle; return safeHandle; } From 1f2129e62e33a64f8c52e2c11e72f424c0022c37 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 17 Feb 2026 00:56:22 +0000 Subject: [PATCH 6/8] Remove redundant keyHandle.Dispose() from test Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../tests/SafeEvpPKeyHandleTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs index 58006cea96848c..1543f75fa64aa1 100644 --- a/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs +++ b/src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs @@ -42,8 +42,6 @@ public static void DuplicateHandle_ConcurrentWithDispose_DoesNotProduceInvalidHa disposeThread.Join(); duplicateThread.Join(); - keyHandle.Dispose(); - if (duplicated is not null) { bool refAdded = false; From 00fe694e17da2319dab52e5b97d96115b7d53b17 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Mon, 16 Feb 2026 23:33:44 -0500 Subject: [PATCH 7/8] Dispose of handle in error path. --- .../Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 5 +++++ 1 file changed, 5 insertions(+) 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 93a3522097964e..417f2fe2ce332d 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 @@ -96,6 +96,11 @@ public SafeEvpPKeyHandle DuplicateHandle() // ExtraHandle is upref'd by UpRefEvpPkey safeHandle.ExtraHandle = ExtraHandle; } + catch + { + safeHandle.Dispose(); + throw; + } finally { if (addedRef) From e06975efd3064884fd297c0914a31e02859aaa50 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 18 Feb 2026 02:51:15 +0000 Subject: [PATCH 8/8] Move safeHandle declaration after DangerousAddRef, remove catch block Co-authored-by: vcsjones <361677+vcsjones@users.noreply.github.com> --- .../Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) 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 417f2fe2ce332d..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,10 +67,6 @@ 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; @@ -79,6 +75,10 @@ public SafeEvpPKeyHandle DuplicateHandle() { 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) @@ -95,11 +95,7 @@ public SafeEvpPKeyHandle DuplicateHandle() safeHandle.SetHandle(handle); // ExtraHandle is upref'd by UpRefEvpPkey safeHandle.ExtraHandle = ExtraHandle; - } - catch - { - safeHandle.Dispose(); - throw; + return safeHandle; } finally { @@ -108,8 +104,6 @@ public SafeEvpPKeyHandle DuplicateHandle() DangerousRelease(); } } - - return safeHandle; } ///