Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle

[Copilot]

• Analyze the thread safety issue in SafeEvpPKeyHandle.DuplicateHandle
• Fix DuplicateHandle using DangerousAddRef/DangerousRelease to prevent concurrent Dispose from zeroing handle fields
• Add regression test for concurrent DuplicateHandle/Dispose
• Address review: format try/catch with braces on own lines and add comment about ignoring managed exceptions
• Address review: revert to DangerousAddRef/DangerousRelease approach
• Address review: remove redundant keyHandle.Dispose() (the dispose thread already handles it)
• Address review: move safeHandle declaration after DangerousAddRef, remove catch block
• Build and test pass (1867 tests, 0 errors, 0 failures)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>SafeEvpPKeyHandle.DuplicateHandle is not thread safe</issue_title>
<issue_description>If a SafeEvpPKeyHandle is disposed by one thread while another thread is calling DuplicateHandle, the DuplicateHandle call can succeed, but the resulting SafeEvpPKeyHandle will not be closed, but the handle value will be zero.

To demonstrate:

using System.Security.Cryptography;

using ECDsaOpenSsl ecdsa = new ECDsaOpenSsl();

for (int i = 0; i < int.MaxValue - 1; i++)
{
    SafeEvpPKeyHandle handle = ecdsa.DuplicateKeyHandle();
    SafeEvpPKeyHandle? duplicate = null;

    Thread t1 = new Thread(() => {
        Thread.Sleep(Random.Shared.Next(0, 10));
        try { duplicate = handle.DuplicateHandle(); } catch { }
    });
    Thread t2 = new Thread(() => {
        Thread.Sleep(Random.Shared.Next(0, 10));
        handle.Dispose();
    });

    t1.Start();
    t2.Start();
    t1.Join();
    t2.Join();

    if (duplicate is null)
    {
        continue;
    }

    bool addedRef = false;

    try
    {
        duplicate.DangerousAddRef(ref addedRef);

        IntPtr value = duplicate.DangerousGetHandle();

        if (value == IntPtr.Zero)
        {
            throw new Exception($"We somehow got a handle that was not closed but invalid on iteration {i}.");
        }
    }
    finally
    {
        if (addedRef)
        {
            duplicate.DangerousRelease();
        }
    }

    duplicate.Dispose();
}

Console.WriteLine("Didn't crash?");

After a few thousand runs, this will throw an exception:

Unhandled exception. System.Exception: We somehow got a handle that was not closed but invalid on iteration 1887.
at Program.

$(String[] args) in /home/vcsjones/Projects/scratch-cs/Program.cs:line 39

This should not happen. This means DuplicateHandle returned a SafeEvpPKeyHandle that is not closed, but is invalid.

This is likely the underlying issue that is causing #116307.

In that case, we are calling DuplicateHandle on the key supplied, then passing the safe handle to EvpPKeyBits which gets marshaled as NULL

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/ECDiffieHellmanOpenSsl.cs

Lines 39 to 40 in f92f9de

	_key = new Lazy<SafeEvpPKeyHandle>(pkeyHandle.DuplicateHandle());
	KeySizeValue = Interop.Crypto.EvpPKeyBits(_key.Value);

The race is likely here:

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

Lines 74 to 86 in f92f9de

	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.
	safeHandle.SetHandle(handle);

If the current instance is disposed right before line 86, then handle will be IntPtr.Zero. That is because ReleaseHandle explicitly zeros the handle:

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

Line 51 in f92f9de

SetHandle(IntPtr.Zero);

The fix is probably that DuplicateHandle should do a DangerousAddRef and DangerousRelease around itself so the handle cannot be destroyed while it is duplicating. This is currently only being done implicitly on line 74 when up-ref-ing the key handle, but if its disposed after the up-ref but before the handle field is read, the resulting duplicated handle will be NULL.
</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes SafeEvpPKeyHandle.DuplicateHandle is not thread safe #124484

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR fixes a thread-safety race condition in SafeEvpPKeyHandle.DuplicateHandle() where concurrent calls to Dispose() could zero the handle field between the UpRef operation and copying the handle to the new SafeHandle, resulting in an undisposed handle with an invalid IntPtr.Zero value.

Changes:

• Wrap DuplicateHandle operation in DangerousAddRef/DangerousRelease to prevent concurrent Dispose from zeroing handle fields mid-operation
• Add regression test exercising concurrent DuplicateHandle/Dispose calls

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs	Added DangerousAddRef/DangerousRelease protection around UpRefEvpPkey and handle copy operations in DuplicateHandle method
src/libraries/System.Security.Cryptography.OpenSsl/tests/SafeEvpPKeyHandleTests.cs	Added test that runs 1000 iterations of concurrent DuplicateHandle/Dispose to verify the race condition is fixed

[vcsjones]

@copilot Please revert this file to the previous commit. It turns out we want to keep the add ref / release approach. The test changes are fine.

[Copilot]

Reverted to the DangerousAddRef/DangerousRelease approach from bffea02. All 1867 tests pass. Fixed in 5c810a4.

[vcsjones]

@copilot please invoke the code-review skill and post the results as a comment on this PR.

[Copilot]

@copilot please invoke the code-review skill and post the results as a comment on this PR.

🤖 Copilot Code Review

Holistic Assessment

Motivation: The race condition is real and well-demonstrated by the reproduction in the issue. Between UpRefEvpPkey(this) returning and SetHandle(handle) executing, a concurrent Dispose() can invoke ReleaseHandle(), which zeros the handle field. The new SafeEvpPKeyHandle then gets IntPtr.Zero — it's not closed (owns a ref) but holds an invalid value, leaking an OpenSSL reference.

Approach: Wrapping the critical section in DangerousAddRef/DangerousRelease is the correct and idiomatic fix. It prevents ReleaseHandle from executing until the copy is complete. This is the same pattern used by FixedMemoryKeyBox.UseKey and other SafeHandle consumers in this codebase.

Summary: ⚠️ Needs Human Review. The core fix is correct and well-structured, but there are two points worth human attention: (1) a subtle secondary race in Interop.Crypto.UpRefEvpPkey accessing handle.ExtraHandle without protection, and (2) the test is inherently non-deterministic.

---

Detailed Findings

✅ Core fix — DangerousAddRef/DangerousRelease is correct

The fix correctly prevents ReleaseHandle from running concurrently with the handle copy. The structure is:

1. DangerousAddRef — elevates ref count, prevents ReleaseHandle from being called even if Dispose() is called concurrently.
2. UpRefEvpPkey — increments OpenSSL's ref count on the EVP_PKEY (the P/Invoke marshalling does its own DangerousAddRef/DangerousRelease, but the explicit one here protects the subsequent field reads).
3. Copy handle and ExtraHandle to the new SafeEvpPKeyHandle — these are stable because ReleaseHandle can't run.
4. DangerousRelease in finally — allows deferred ReleaseHandle to proceed if Dispose was called during the window.

The error path for UpRefEvpPkey failure correctly preserves the original pattern: capture the OpenSSL exception, dispose the pre-allocated safeHandle, then throw. The finally block only releases the ref if addedRef is true, which is correct since DangerousAddRef sets it atomically.

⚠️ Secondary race in Interop.Crypto.UpRefEvpPkey wrapper

In Interop.EvpPkey.cs:42-44:

internal static int UpRefEvpPkey(SafeEvpPKeyHandle handle)
{
    return UpRefEvpPkey(handle, handle.ExtraHandle);
}

This reads handle.ExtraHandle before the P/Invoke marshalling does its DangerousAddRef. In the original code (without this PR's fix), a concurrent Dispose → ReleaseHandle could zero ExtraHandle between the read at line 44 and the P/Invoke call. With this PR's fix, the explicit DangerousAddRef at line 80 of the caller already protects this read, so the race is closed — but only because DuplicateHandle is the sole caller that could be subject to this race. This is worth noting for awareness but is not a blocking issue for this PR.

✅ Error handling preserved correctly

The UpRef failure path preserves the original pattern: CreateOpenSslCryptographicException() → safeHandle.Dispose() → throw. The exception is captured before Dispose() in case Dispose() clears error state. The DangerousRelease in finally will still execute, which is correct — the ref was added, so it must be released regardless of whether UpRef succeeded.

⚠️ safeHandle leak on DangerousAddRef failure

If DangerousAddRef throws ObjectDisposedException (which it does when the handle has already been fully released), safeHandle is allocated but never disposed. This is a minor concern — safeHandle wraps IntPtr.Zero (the default from the constructor) so its ReleaseHandle would call EvpPkeyDestroy(IntPtr.Zero, IntPtr.Zero), which should be a no-op. But for cleanliness, consider whether wrapping safeHandle in a using or disposing it in a catch would be warranted. However, since the original code also didn't handle this (and the IsInvalid check at line 67 would normally prevent reaching this code for a disposed handle), this is advisory rather than blocking.

💡 Test — non-deterministic but reasonable

The test creates 1000 iterations with random sleep timings to exercise the race. This is the standard approach for concurrency regression tests in this codebase (matching the pattern from the issue reproduction). The catch block correctly documents that managed exceptions are expected and only native crashes/asserts would indicate a problem. The DangerousAddRef/Assert.NotEqual(IntPtr.Zero, ...) pattern correctly validates the invariant.

One observation: the test validates DangerousGetHandle() != IntPtr.Zero when duplicated is not null, but there's no assertion counting how many times duplication succeeded vs. failed. This is fine — the test's purpose is to verify no invalid handles escape, not to verify a specific success rate.

[dotnet-policy-service]

Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security
See info in area-owners.md if you want to be subscribed.