Remove unnecessary SafeHandle allocations in X509Certificates library

Several related issues:
- The custom SafeHandle types expose an InvalidHandle property.  As a property, one typically expects that each call to the property returns the same instance, but each call to these properties is manufacturing a new SafeHandle instance.
- Lots of the accesses to InvalidHandle are to get an initially "null" handle to pass to a native function that expects a null handle on the first invocation.  This means that we're allocating a finalizable object only to then throw it away.
- In some cases, we were completely ignoring the allocated handle, e.g. initializing a handle to InvalidHandle and then immediately using that variable as an out argument.
- etc.

This commit addresses these issues, such that the number of allocated handles noticeably decreases.  As an example, during the execution of the System.Security.Cryptography.X509Certificates.Tests as they currently exist, prior to this change 1661 SafePointerHandles were being allocated; after this change, that number drops to 1410, for a 15% reduction.

Author: stephentoub

src/Common/src/Microsoft/Win32/SafeHandles/SafeHandleCache.cs

@@ -0,0 +1,54 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+using System.Diagnostics;
+using System.Runtime.InteropServices;
+using System.Threading;
+
+namespace Microsoft.Win32.SafeHandles
+{
+    /// <summary>Provides a cache for special instances of SafeHandles.</summary>
+    /// <typeparam name="T">Specifies the type of SafeHandle.</typeparam>
+    internal static class SafeHandleCache<T> where T : SafeHandle
+    {
+        private static T s_invalidHandle;
+
+        /// <summary>
+        /// Gets a cached, invalid handle.  As the instance is cached, it should either never be Disposed
+        /// or it should override <see cref="SafeHandle.Dispose(bool)"/> to prevent disposal when the
+        /// instance is the <see cref="InvalidHandle"/>.
+        /// </summary>
+        internal static T GetInvalidHandle(Func<T> invalidHandleFactory)
+        {
+            T currentHandle = Volatile.Read(ref s_invalidHandle);
+            if (currentHandle == null)
+            {
+                T newHandle = invalidHandleFactory();
+                currentHandle = Interlocked.CompareExchange(ref s_invalidHandle, newHandle, null);
+                if (currentHandle == null)
+                {
+                    GC.SuppressFinalize(newHandle);
+                    currentHandle = newHandle;
+                }
+                else
+                {
+                    newHandle.Dispose();
+                }
+            }
+            Debug.Assert(currentHandle.IsInvalid);
+            return currentHandle;
+        }
+
+        /// <summary>Gets whether the specified handle is <see cref="InvalidHandle"/>.</summary>
+        /// <param name="handle">The handle to compare.</param>
+        /// <returns>true if <paramref name="handle"/> is <see cref="InvalidHandle"/>; otherwise, false.</returns>
+        internal static bool IsCachedInvalidHandle(SafeHandle handle)
+        {
+            Debug.Assert(handle != null);
+            bool isCachedInvalidHandle = ReferenceEquals(handle, Volatile.Read(ref s_invalidHandle));
+            Debug.Assert(!isCachedInvalidHandle || handle.IsInvalid, "The cached invalid handle must still be invalid.");
+            return isCachedInvalidHandle;
+        }
+    }
+}

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.Import.cs

@@ -137,7 +137,7 @@ private static SafeCertContextHandle GetSignerInPKCS7Store(SafeCertStoreHandle h
                     certInfo.SerialNumber.pbData = pCmsgSignerInfo->SerialNumber.pbData;
                 }
 
-                SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+                SafeCertContextHandle pCertContext = null;
                 if (!Interop.crypt32.CertFindCertificateInStore(hCertStore, CertFindType.CERT_FIND_SUBJECT_CERT, &certInfo, ref pCertContext))
                     throw new CryptographicException(Marshal.GetHRForLastWin32Error());
 
@@ -147,7 +147,7 @@ private static SafeCertContextHandle GetSignerInPKCS7Store(SafeCertStoreHandle h
 
         private static SafeCertContextHandle FilterPFXStore(byte[] rawData, String password, PfxCertStoreFlags pfxCertStoreFlags)
         {
-            SafeCertStoreHandle hStore = SafeCertStoreHandle.InvalidHandle;
+            SafeCertStoreHandle hStore;
             unsafe
             {
                 fixed (byte* pbRawData = rawData)
@@ -164,7 +164,7 @@ private static SafeCertContextHandle FilterPFXStore(byte[] rawData, String passw
                 // Find the first cert with private key. If none, then simply take the very first cert. Along the way, delete the keycontainers
                 // of any cert we don't accept.
                 SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
-                SafeCertContextHandle pEnumContext = SafeCertContextHandle.InvalidHandle;
+                SafeCertContextHandle pEnumContext = null;
                 while (Interop.crypt32.CertEnumCertificatesInStore(hStore, ref pEnumContext))
                 {
                     if (pEnumContext.ContainsPrivateKey)

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Helpers.cs

@@ -23,11 +23,10 @@ internal static class Helpers
         /// </summary>
         public static SafeHandle ToLpstrArray(this OidCollection oids, out int numOids)
         {
-            SafeLocalAllocHandle safeLocalAllocHandle = SafeLocalAllocHandle.InvalidHandle;
             if (oids == null || oids.Count == 0)
             {
                 numOids = 0;
-                return safeLocalAllocHandle;
+                return SafeLocalAllocHandle.InvalidHandle;
             }
 
             // Copy the oid strings to a local list to prevent a security race condition where
@@ -52,7 +51,7 @@ public static SafeHandle ToLpstrArray(this OidCollection oids, out int numOids)
                     }
                 }
 
-                safeLocalAllocHandle = SafeLocalAllocHandle.Create(allocationSize);
+                SafeLocalAllocHandle safeLocalAllocHandle = SafeLocalAllocHandle.Create(allocationSize);
                 byte** pOidPointers = (byte**)(safeLocalAllocHandle.DangerousGetHandle());
                 byte* pOidContents = (byte*)(pOidPointers + numOids);
 

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/Interop.crypt32.cs

@@ -104,15 +104,15 @@ public static SafeCertStoreHandle CertOpenStore(CertStoreProvider lpszStoreProvi
             /// <summary>
             /// A less error-prone wrapper for CertEnumCertificatesInStore().
             /// 
-            /// To begin the enumeration, set pCertContext to SafeCertStoreHandle.InvalidHandle. Each iteration replaces pCertContext with
-            /// the next certificate in the iteration. The final call sets pCertContext back to SafeCertStoreHandle.InvalidHandle and returns "false"
-            /// to indicate the the end of the store has been reached.
+            /// To begin the enumeration, set pCertContext to null. Each iteration replaces pCertContext with
+            /// the next certificate in the iteration. The final call sets pCertContext to an invalid SafeCertStoreHandle 
+            /// and returns "false" to indicate the the end of the store has been reached.
             /// </summary>
             public static bool CertEnumCertificatesInStore(SafeCertStoreHandle hCertStore, ref SafeCertContextHandle pCertContext)
             {
                 unsafe
                 {
-                    CERT_CONTEXT* pPrevCertContext = pCertContext.Disconnect();
+                    CERT_CONTEXT* pPrevCertContext = pCertContext == null ? null : pCertContext.Disconnect();
                     pCertContext = CertEnumCertificatesInStore(hCertStore, pPrevCertContext);
                     return !pCertContext.IsInvalid;
                 }
@@ -226,13 +226,13 @@ public static unsafe bool CertGetCertificateChain(ChainEngine hChainEngine, Safe
             /// <summary>
             /// A less error-prone wrapper for CertEnumCertificatesInStore().
             /// 
-            /// To begin the enumeration, set pCertContext to SafeCertStoreHandle.InvalidHandle. Each iteration replaces pCertContext with
-            /// the next certificate in the iteration. The final call sets pCertContext back to SafeCertStoreHandle.InvalidHandle and returns "false"
-            /// to indicate the the end of the store has been reached.
+            /// To begin the enumeration, set pCertContext to null. Each iteration replaces pCertContext with
+            /// the next certificate in the iteration. The final call sets pCertContext to an invalid SafeCertStoreHandle 
+            /// and returns "false" to indicate the the end of the store has been reached.
             /// </summary>
             public static unsafe bool CertFindCertificateInStore(SafeCertStoreHandle hCertStore, CertFindType dwFindType, void* pvFindPara, ref SafeCertContextHandle pCertContext)
             {
-                CERT_CONTEXT* pPrevCertContext = pCertContext.Disconnect();
+                CERT_CONTEXT* pPrevCertContext = pCertContext == null ? null : pCertContext.Disconnect();
                 pCertContext = CertFindCertificateInStore(hCertStore, CertEncodingType.All, CertFindFlags.None, dwFindType, pvFindPara, pPrevCertContext);
                 return !pCertContext.IsInvalid;
             }

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/Native/SafeHandles.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.Win32.SafeHandles;
 using System;
 using System.Diagnostics;
 using System.Runtime.InteropServices;
@@ -12,22 +13,27 @@ namespace Internal.Cryptography.Pal.Native
     /// </summary>
     internal abstract class SafePointerHandle<T> : SafeHandle where T : SafeHandle, new()
     {
-        public SafePointerHandle()
+        protected SafePointerHandle()
             : base(IntPtr.Zero, true)
         {
         }
 
         public sealed override bool IsInvalid
         {
-            get
-            {
-                return handle == IntPtr.Zero;
-            }
+            get { return handle == IntPtr.Zero; }
         }
 
         public static T InvalidHandle
         {
-            get { return new T(); }
+            get { return SafeHandleCache<T>.GetInvalidHandle(() => new T()); }
+        }
+
+        protected override void Dispose(bool disposing)
+        {
+            if (!SafeHandleCache<T>.IsCachedInvalidHandle(this))
+            {
+                base.Dispose(disposing);
+            }
         }
     }
 
@@ -146,28 +152,17 @@ protected sealed override bool ReleaseHandle()
     /// </summary>
     internal sealed class SafeLocalAllocHandle : SafePointerHandle<SafeLocalAllocHandle>
     {
-        public SafeLocalAllocHandle()
-            : base()
-        {
-        }
-
         public static SafeLocalAllocHandle Create(int cb)
         {
-            return new SafeLocalAllocHandle(Marshal.AllocHGlobal(cb));
+            var h = new SafeLocalAllocHandle();
+            h.SetHandle(Marshal.AllocHGlobal(cb));
+            return h;
         }
 
         protected sealed override bool ReleaseHandle()
         {
             Marshal.FreeHGlobal(handle);
             return true;
         }
-
-        private SafeLocalAllocHandle(IntPtr p)
-            : base()
-        {
-            handle = p;
-        }
     }
 }
-
-

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/StorePal.Export.cs

@@ -27,7 +27,7 @@ public byte[] Export(X509ContentType contentType, String password)
             {
                 case X509ContentType.Cert:
                     {
-                        SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+                        SafeCertContextHandle pCertContext = null;
                         if (!Interop.crypt32.CertEnumCertificatesInStore(_certStore, ref pCertContext))
                             return null;
                         try
@@ -48,7 +48,7 @@ public byte[] Export(X509ContentType contentType, String password)
 
                 case X509ContentType.SerializedCert:
                     {
-                        SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+                        SafeCertContextHandle pCertContext = null;
                         if (!Interop.crypt32.CertEnumCertificatesInStore(_certStore, ref pCertContext))
                             return null;
 

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/StorePal.Find.cs

@@ -478,7 +478,7 @@ private unsafe StorePal FindCore(CertFindType dwFindType, void* pvFindPara, bool
             if (findResults.IsInvalid)
                 throw new CryptographicException(Marshal.GetHRForLastWin32Error());
 
-            SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+            SafeCertContextHandle pCertContext = null;
             while (Interop.crypt32.CertFindCertificateInStore(_certStore, dwFindType, pvFindPara, ref pCertContext))
             {
                 if (filter != null && !filter(pCertContext))

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/StorePal.Import.cs

@@ -48,7 +48,7 @@ private static StorePal FromBlobOrFile(byte[] rawData, String fileName, String p
                         void* pvObject = fromFile ? (void*)pFileName : (void*)&blob;
 
                         ContentType contentType;
-                        SafeCertStoreHandle certStore = SafeCertStoreHandle.InvalidHandle;
+                        SafeCertStoreHandle certStore;
                         if (!Interop.crypt32.CryptQueryObject(
                             fromFile ? CertQueryObjectType.CERT_QUERY_OBJECT_FILE : CertQueryObjectType.CERT_QUERY_OBJECT_BLOB,
                             pvObject,
@@ -86,7 +86,7 @@ private static StorePal FromBlobOrFile(byte[] rawData, String fileName, String p
 
                         if (!persistKeySet)
                         {
-                            SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+                            SafeCertContextHandle pCertContext = null;
                             while (Interop.crypt32.CertEnumCertificatesInStore(certStore, ref pCertContext))
                             {
                                 CRYPTOAPI_BLOB nullBlob = new CRYPTOAPI_BLOB(0, null);

src/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/StorePal.cs

@@ -27,7 +27,7 @@ public IEnumerable<X509Certificate2> Certificates
             {
                 LowLevelListWithIList<X509Certificate2> certificates = new LowLevelListWithIList<X509Certificate2>();
 
-                SafeCertContextHandle pCertContext = SafeCertContextHandle.InvalidHandle;
+                SafeCertContextHandle pCertContext = null;
                 while (Interop.crypt32.CertEnumCertificatesInStore(_certStore, ref pCertContext))
                 {
                     X509Certificate2 cert = new X509Certificate2(pCertContext.DangerousGetHandle());
@@ -49,7 +49,7 @@ public void Remove(ICertificatePal certificate)
             unsafe
             {
                 SafeCertContextHandle existingCertContext = ((CertificatePal)certificate).CertContext;
-                SafeCertContextHandle enumCertContext = SafeCertContextHandle.InvalidHandle;
+                SafeCertContextHandle enumCertContext = null;
                 CERT_CONTEXT* pCertContext = existingCertContext.CertContext;
                 if (!Interop.crypt32.CertFindCertificateInStore(_certStore, CertFindType.CERT_FIND_EXISTING, pCertContext, ref enumCertContext))
                     return; // The certificate is not present in the store, simply return.

src/System.Security.Cryptography.X509Certificates/src/Microsoft/Win32/SafeHandles/SafeX509ChainHandle.cs

@@ -1,16 +1,11 @@
 // Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Internal.Cryptography.Pal;
 using System;
-using System.IO;
-using System.Text;
 using System.Diagnostics;
-using System.Globalization;
 using System.Runtime.InteropServices;
 
-using Internal.Cryptography;
-using Internal.Cryptography.Pal;
-
 namespace Microsoft.Win32.SafeHandles
 {
     public sealed class SafeX509ChainHandle : SafeHandle
@@ -25,15 +20,22 @@ public override bool IsInvalid
             get { return handle == IntPtr.Zero; }
         }
 
+        public static SafeX509ChainHandle InvalidHandle
+        {
+            get { return SafeHandleCache<SafeX509ChainHandle>.GetInvalidHandle(() => new SafeX509ChainHandle()); }
+        }
+
         protected override bool ReleaseHandle()
         {
             return ChainPal.ReleaseSafeX509ChainHandle(handle);
         }
 
-        internal static SafeX509ChainHandle InvalidHandle
+        protected override void Dispose(bool disposing)
         {
-            get { return new SafeX509ChainHandle(); }
+            if (!SafeHandleCache<SafeX509ChainHandle>.IsCachedInvalidHandle(this))
+            {
+                base.Dispose(disposing);
+            }
         }
     }
 }
-