More reliably clean up more SafeHandle instances

src/coreclr/vm/corelib.h

@@ -792,6 +792,7 @@ DEFINE_CLASS(CALLCONV_SUPPRESSGCTRANSITION,  CompilerServices,       CallConvSup
 DEFINE_CLASS(CALLCONV_MEMBERFUNCTION,        CompilerServices,       CallConvMemberFunction)
 
 DEFINE_CLASS_U(Interop,                SafeHandle,         SafeHandle)
+DEFINE_FIELD_U(_ctorStackTrace,            SafeHandle,            m_ctorStackTrace)
 DEFINE_FIELD_U(handle,                     SafeHandle,            m_handle)
 DEFINE_FIELD_U(_state,                     SafeHandle,            m_state)
 DEFINE_FIELD_U(_ownsHandle,                SafeHandle,            m_ownsHandle)

src/coreclr/vm/object.h

@@ -1913,6 +1913,9 @@ class SafeHandle : public Object
     //   Modifying the order or fields of this object may require
     //   other changes to the classlib class definition of this
     //   object or special handling when loading this system class.
+#if DEBUG
+    STRINGREF m_ctorStackTrace; // Debug-only stack trace captured when the SafeHandle was constructed
+#endif
     Volatile<LPVOID> m_handle;
     Volatile<INT32> m_state;        // Combined ref count and closed/disposed state (for atomicity)
     Volatile<CLR_BOOL> m_ownsHandle;

src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Err.cs

@@ -94,7 +94,9 @@ internal static void CheckValidOpenSslHandle(SafeHandle handle)
         {
             if (handle == null || handle.IsInvalid)
             {
-                throw CreateOpenSslCryptographicException();
+                Exception e = CreateOpenSslCryptographicException();
+                handle?.Dispose();
+                throw e;
             }
         }
 

src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.macOS.cs

@@ -54,10 +54,13 @@ internal static SafeSecKeyRefHandle ImportEphemeralKey(ReadOnlySpan<byte> keyBlo
 
             if (ret == 0)
             {
-                throw CreateExceptionForOSStatus(osStatus);
+                Exception e = CreateExceptionForOSStatus(osStatus);
+                keyHandle.Dispose();
+                throw e;
             }
 
             Debug.Fail($"SecKeyImportEphemeral returned {ret}");
+            keyHandle.Dispose();
             throw new CryptographicException();
         }
 

src/libraries/Common/src/Interop/Unix/Interop.IOErrors.cs

@@ -88,7 +88,9 @@ internal static TSafeHandle CheckIo<TSafeHandle>(TSafeHandle handle, string? pat
     {
         if (handle.IsInvalid)
         {
-            ThrowExceptionForIoErrno(Sys.GetLastErrorInfo(), path, isDirectory);
+            Exception e = Interop.GetExceptionForIoErrno(Sys.GetLastErrorInfo(), path, isDirectory);
+            handle.Dispose();
+            throw e;
         }
 
         return handle;

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.ERR.cs

@@ -91,7 +91,9 @@ internal static void CheckValidOpenSslHandle(SafeHandle handle)
         {
             if (handle == null || handle.IsInvalid)
             {
-                throw CreateOpenSslCryptographicException();
+                Exception e = CreateOpenSslCryptographicException();
+                handle?.Dispose();
+                throw e;
             }
         }
 

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EcDsa.ImportExport.cs

@@ -84,8 +84,9 @@ internal static SafeEcKeyHandle EcKeyCreateByExplicitCurve(ECCurve curve)
 
             if (key == null || key.IsInvalid)
             {
+                Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
                 key?.Dispose();
-                throw Interop.Crypto.CreateOpenSslCryptographicException();
+                throw e;
             }
 
             // EcKeyCreateByExplicitParameters may have polluted the error queue, but key was good in the end.

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.X509.cs

@@ -151,7 +151,9 @@ internal static SafeX509StoreHandle X509ChainNew(SafeX509StackHandle systemTrust
 
             if (store.IsInvalid)
             {
-                throw CreateOpenSslCryptographicException();
+                Exception e = CreateOpenSslCryptographicException();
+                store.Dispose();
+                throw e;
             }
 
             return store;

src/libraries/Common/src/Interop/Windows/Advapi32/Interop.OpenThreadToken.cs

@@ -30,6 +30,7 @@ internal static bool OpenThreadToken(TokenAccessLevels desiredAccess, WinSecurit
             if (openAs == WinSecurityContext.Both)
             {
                 openAsSelf = false;
+                tokenHandle.Dispose();
                 if (OpenThreadToken(Kernel32.GetCurrentThread(), desiredAccess, openAsSelf, out tokenHandle))
                     return true;
             }

src/libraries/Common/src/Interop/Windows/BCrypt/BCryptAlgorithmCache.cs

@@ -3,7 +3,7 @@
 
 using System;
 using System.Diagnostics;
-using System.Collections.Generic;
+using System.Collections.Concurrent;
 using System.Security.Cryptography;
 
 using Microsoft.Win32.SafeHandles;
@@ -14,79 +14,54 @@ internal static partial class BCrypt
     {
         internal static class BCryptAlgorithmCache
         {
+            private static readonly ConcurrentDictionary<(string HashAlgorithmId, BCryptOpenAlgorithmProviderFlags Flags), (SafeBCryptAlgorithmHandle Handle, int HashSizeInBytes)> s_handles = new();
+
             /// <summary>
-            ///     Return a SafeBCryptAlgorithmHandle of the desired algorithm and flags. This is a shared handle so do not dispose it!
+            /// Returns a SafeBCryptAlgorithmHandle of the desired algorithm and flags. This is a shared handle so do not dispose it!
             /// </summary>
-            public static SafeBCryptAlgorithmHandle GetCachedBCryptAlgorithmHandle(string hashAlgorithmId, BCryptOpenAlgorithmProviderFlags flags, out int hashSizeInBytes)
+            public static unsafe SafeBCryptAlgorithmHandle GetCachedBCryptAlgorithmHandle(string hashAlgorithmId, BCryptOpenAlgorithmProviderFlags flags, out int hashSizeInBytes)
             {
-                // There aren't that many hash algorithms around so rather than use a LowLevelDictionary and guard it with a lock,
-                // we'll use a simple list. To avoid locking, we'll recreate the entire list each time an entry is added and replace it atomically.
-                //
-                // This does mean that on occasion, racing threads may create two handles of the same type, but this is ok.
+                var key = (hashAlgorithmId, flags);
 
-                // Latch the _cache value into a local so we aren't disrupted by concurrent changes to it.
-                Entry[] cache = _cache;
-                foreach (Entry entry in cache)
+                while (true)
                 {
-                    if (entry.HashAlgorithmId == hashAlgorithmId && entry.Flags == flags)
+                    if (s_handles.TryGetValue(key, out (SafeBCryptAlgorithmHandle Handle, int HashSizeInBytes) result))
                     {
-                        hashSizeInBytes = entry.HashSizeInBytes;
-                        return entry.Handle;
+                        hashSizeInBytes = result.HashSizeInBytes;
+                        return result.Handle;
                     }
-                }
-
-                SafeBCryptAlgorithmHandle safeBCryptAlgorithmHandle;
-                NTSTATUS ntStatus = Interop.BCrypt.BCryptOpenAlgorithmProvider(out safeBCryptAlgorithmHandle, hashAlgorithmId, null, flags);
-                if (ntStatus != NTSTATUS.STATUS_SUCCESS)
-                    throw Interop.BCrypt.CreateCryptographicException(ntStatus);
-
-                Array.Resize(ref cache, cache.Length + 1);
-                Entry newEntry = new Entry(hashAlgorithmId, flags, safeBCryptAlgorithmHandle);
-                cache[^1] = new Entry(hashAlgorithmId, flags, safeBCryptAlgorithmHandle);
-
-                // Atomically overwrite the cache with our new cache. It's possible some other thread raced to add a new entry with us - if so, one of the new entries
-                // will be lost and the next request will have to allocate it again. That's considered acceptable collateral damage.
-                _cache = cache;
 
-                hashSizeInBytes = newEntry.HashSizeInBytes;
-                return newEntry.Handle;
-            }
-
-            private static volatile Entry[] _cache = Array.Empty<Entry>();
-
-            private struct Entry
-            {
-                public unsafe Entry(string hashAlgorithmId, BCryptOpenAlgorithmProviderFlags flags, SafeBCryptAlgorithmHandle handle)
-                    : this()
-                {
-                    HashAlgorithmId = hashAlgorithmId;
-                    Flags = flags;
-                    Handle = handle;
+                    NTSTATUS ntStatus = Interop.BCrypt.BCryptOpenAlgorithmProvider(out SafeBCryptAlgorithmHandle handle, key.hashAlgorithmId, null, key.flags);
+                    if (ntStatus != NTSTATUS.STATUS_SUCCESS)
+                    {
+                        Exception e = Interop.BCrypt.CreateCryptographicException(ntStatus);
+                        handle.Dispose();
+                        throw e;
+                    }
 
                     int hashSize;
-                    NTSTATUS ntStatus = Interop.BCrypt.BCryptGetProperty(
+                    ntStatus = Interop.BCrypt.BCryptGetProperty(
                         handle,
                         Interop.BCrypt.BCryptPropertyStrings.BCRYPT_HASH_LENGTH,
                         &hashSize,
                         sizeof(int),
                         out int cbHashSize,
                         0);
-
                     if (ntStatus != NTSTATUS.STATUS_SUCCESS)
                     {
-                        throw Interop.BCrypt.CreateCryptographicException(ntStatus);
+                        Exception e = Interop.BCrypt.CreateCryptographicException(ntStatus);
+                        handle.Dispose();
+                        throw e;
                     }
 
                     Debug.Assert(cbHashSize == sizeof(int));
                     Debug.Assert(hashSize > 0);
 
-                    HashSizeInBytes = hashSize;
+                    if (!s_handles.TryAdd(key, (handle, hashSize)))
+                    {
+                        handle.Dispose();
+                    }
                 }
-
-                public string HashAlgorithmId { get; private set; }
-                public BCryptOpenAlgorithmProviderFlags Flags { get; private set; }
-                public SafeBCryptAlgorithmHandle Handle { get; private set; }
-                public int HashSizeInBytes { get; private set; }
             }
         }
     }

src/libraries/Common/src/Interop/Windows/Crypt32/Interop.CryptEncodeDecodeWrappers.cs

@@ -29,11 +29,17 @@ internal static unsafe SafeHandle CryptDecodeObjectToMemory(CryptDecodeObjectStr
             int cbRequired = 0;
 
             if (!CryptDecodeObject(MsgEncodingType.All, (IntPtr)lpszStructType, pbEncoded, cbEncoded, 0, null, ref cbRequired))
+            {
                 throw Marshal.GetLastWin32Error().ToCryptographicException();
+            }
 
             SafeHandle sh = SafeHeapAllocHandle.Alloc(cbRequired);
             if (!CryptDecodeObject(MsgEncodingType.All, (IntPtr)lpszStructType, pbEncoded, cbEncoded, 0, (void*)sh.DangerousGetHandle(), ref cbRequired))
-                throw Marshal.GetLastWin32Error().ToCryptographicException();
+            {
+                Exception e = Marshal.GetLastWin32Error().ToCryptographicException();
+                sh.Dispose();
+                throw e;
+            }
 
             return sh;
         }
@@ -42,11 +48,15 @@ internal static unsafe byte[] CryptEncodeObjectToByteArray(CryptDecodeObjectStru
         {
             int cb = 0;
             if (!CryptEncodeObject(MsgEncodingType.All, lpszStructType, decoded, null, ref cb))
+            {
                 throw Marshal.GetLastWin32Error().ToCryptographicException();
+            }
 
             byte[] encoded = new byte[cb];
             if (!CryptEncodeObject(MsgEncodingType.All, lpszStructType, decoded, encoded, ref cb))
+            {
                 throw Marshal.GetLastWin32Error().ToCryptographicException();
+            }
 
             return encoded.Resize(cb);
         }

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.DuplicateHandle_SafeAccessTokenHandle.cs

@@ -15,7 +15,7 @@ internal static partial bool DuplicateHandle(
             IntPtr hSourceProcessHandle,
             IntPtr hSourceHandle,
             IntPtr hTargetProcessHandle,
-            ref SafeAccessTokenHandle lpTargetHandle,
+            out SafeAccessTokenHandle lpTargetHandle,
             uint dwDesiredAccess,
             [MarshalAs(UnmanagedType.Bool)] bool bInheritHandle,
             uint dwOptions);

src/libraries/Common/src/Interop/Windows/NCrypt/Interop.NCryptDeriveSecretAgreement.cs

@@ -44,6 +44,7 @@ internal static SafeNCryptSecretHandle DeriveSecretAgreement(
 
             if (error != ErrorCode.ERROR_SUCCESS)
             {
+                secretAgreement.Dispose();
                 throw error.ToCryptographicException();
             }
 

src/libraries/Common/src/System/Net/ContextAwareResult.Windows.cs

@@ -10,7 +10,6 @@ internal sealed partial class ContextAwareResult
     {
         private WindowsIdentity? _windowsIdentity;
 
-        // Security: We need an assert for a call into WindowsIdentity.GetCurrent.
         private void SafeCaptureIdentity()
         {
             Debug.Assert(OperatingSystem.IsWindows());

src/libraries/Common/src/System/Security/Cryptography/DSAOpenSsl.cs

@@ -126,7 +126,9 @@ public override void ImportParameters(DSAParameters parameters)
                 parameters.Y, parameters.Y.Length,
                 parameters.X, parameters.X != null ? parameters.X.Length : 0))
             {
-                throw Interop.Crypto.CreateOpenSslCryptographicException();
+                Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
+                key.Dispose();
+                throw e;
             }
 
             SetKey(key);
@@ -183,7 +185,9 @@ private SafeDsaHandle GenerateKey()
 
             if (!Interop.Crypto.DsaGenerateKey(out key, KeySize))
             {
-                throw Interop.Crypto.CreateOpenSslCryptographicException();
+                Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
+                key.Dispose();
+                throw e;
             }
 
             return key;

src/libraries/Common/src/System/Security/Cryptography/ECAndroid.ImportExport.cs

@@ -34,6 +34,7 @@ public int ImportParameters(ECParameters parameters)
 
             if (key == null || key.IsInvalid)
             {
+                key?.Dispose();
                 throw new CryptographicException();
             }
 
@@ -175,7 +176,10 @@ public static SafeEcKeyHandle GenerateKeyByKeySize(int keySize)
             SafeEcKeyHandle? key = Interop.AndroidCrypto.EcKeyCreateByOid(oid);
 
             if (key == null || key.IsInvalid)
+            {
+                key?.Dispose();
                 throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, oid));
+            }
 
             return key;
         }

src/libraries/Common/src/System/Security/Cryptography/ECAndroid.cs

@@ -69,6 +69,7 @@ internal int GenerateKey(ECCurve curve)
 
                 if (key == null || key.IsInvalid)
                 {
+                    key?.Dispose();
                     throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, oid));
                 }
 
@@ -80,6 +81,7 @@ internal int GenerateKey(ECCurve curve)
 
                 if (key == null || key.IsInvalid)
                 {
+                    key?.Dispose();
                     throw new CryptographicException();
                 }
 

src/libraries/Common/src/System/Security/Cryptography/ECCng.ImportExport.cs

@@ -503,6 +503,7 @@ ref MemoryMarshal.GetReference(keyBlob),
             if (errorCode != ErrorCode.ERROR_SUCCESS)
             {
                 Exception e = errorCode.ToCryptographicException();
+                keyHandle.Dispose();
                 if (errorCode == ErrorCode.NTE_INVALID_PARAMETER)
                 {
                     throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, curveName), e);

src/libraries/Common/src/System/Security/Cryptography/ECDiffieHellmanCng.cs

@@ -20,8 +20,16 @@ public ECDiffieHellmanCng(int keySize)
         [SupportedOSPlatform("windows")]
         public ECDiffieHellmanCng(ECCurve curve)
         {
-            // GenerateKey will already do all of the validation we need.
-            GenerateKey(curve);
+            try
+            {
+                // GenerateKey will already do all of the validation we need.
+                GenerateKey(curve);
+            }
+            catch
+            {
+                Dispose();
+                throw;
+            }
         }
 
         public override int KeySize

src/libraries/Common/src/System/Security/Cryptography/ECDiffieHellmanOpenSslPublicKey.cs

@@ -22,8 +22,9 @@ internal ECDiffieHellmanOpenSslPublicKey(SafeEvpPKeyHandle pkeyHandle)
 
             if (key.IsInvalid)
             {
+                Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
                 key.Dispose();
-                throw Interop.Crypto.CreateOpenSslCryptographicException();
+                throw e;
             }
 
             _key = new ECOpenSsl(key);

src/libraries/Common/src/System/Security/Cryptography/ECDsaCng.cs

@@ -16,8 +16,16 @@ public sealed partial class ECDsaCng : ECDsa, IRuntimeAlgorithm
         [SupportedOSPlatform("windows")]
         public ECDsaCng(ECCurve curve)
         {
-            // Specified curves generate the key immediately
-            GenerateKey(curve);
+            try
+            {
+                // Specified curves generate the key immediately
+                GenerateKey(curve);
+            }
+            catch
+            {
+                Dispose();
+                throw;
+            }
         }
 
         /// <summary>