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

[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.

[bartonjs]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging (link to workflow run)

[bartonjs]

/backport to release/9.0-staging

[bartonjs]

/backport to release/10.0

[github-actions]

Started backporting to release/9.0-staging (link to workflow run)

[github-actions]

Started backporting to release/10.0 (link to workflow run)

[github-actions]

@bartonjs backporting to release/8.0-staging failed, the patch most likely resulted in conflicts. Please backport manually!

git am output

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Creating an empty commit: Initial plan
Applying: Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle and add test
Using index info to reconstruct a base tree...
M	src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs
CONFLICT (content): Merge conflict in src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Fix thread safety in SafeEvpPKeyHandle.DuplicateHandle and add test
Error: The process '/usr/bin/git' failed with exit code 128

Link to workflow output