More reliably clean up more SafeHandle instances (#71991)

* More reliably clean up more SafeHandle instances

This calls Dispose on SafeHandles (or things wrapping SafeHandles) that were otherwise being left for finalization:
1. Some of these are fixing finalization happening even on success paths (typically where the implementation isn't disposing of something that directly or indirectly wraps a SafeHandle).
2. Some of these are fixing finalization happening for invalid SafeHandles.
3. Some of these are fixing tests to finalize less.  My goal with fixing the tests was to eliminate the noise in order to find instances of the other two cases.

These were found primarily via two means:
- Debug-instrumentation in SafeHandle to log when one is finalized.  This instrumentation is built into debug/checked builds of SafeHandle and requires setting the DEBUG_SAFEHANDLE_FINALIZATION environment variable to "1".
- Auditing use of SafeHandle.IsInvalid

There's a lot more that can be cleaned up using the SafeHandle instrumentation, but I'm pausing here for now.  The System.IO.Pipes, System.IO.FileSystem, and System.Security.Cryptography tests are clean on Windows.

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>