X509Store.Certificates.Find() results in surprisingly large finalized object counts

[brporter]

Description

While debugging a memory allocation issue, I noted a large number of finalized objects were being reported in my perfview trace, specifically over 100,000 instances of finalization for Internal.Cryptography.Pal.Native.SafeCertContextHandle.

After some experimentation, I've found that the following program reproduces this behavior:

for (int i = 0; i < 100; i++)
{
    using (var store = new X509Store(StoreName.My, StoreLocation.LocalMachine, OpenFlags.ReadOnly | OpenFlags.OpenExistingOnly))
    {
        var certs = store.Certificates;

        var retVal = certs.Find(X509FindType.FindBySubjectName, "doestnotexist.com", true);
    }
}

Configuration

I've reproduced this behavior on both .NET 5.0 RC2 and .NET Core 3.1. Windows 10 20H2, x86 and x64.

Regression?

The behavior seem to be present in .NET Core 3.1, so if this is a regression it doesn't appear to be a particularly new one.

Other information

I'm happy to learn that I'm doing something terribly stupid with the above API. :)

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

[stephentoub]

Disposing of the X509Store doesn't dispose of all of the certificate objects returned in the Certificates collection, as they can be used after the store is disposed. Try adding:

foreach (var cert in certs) cert.Dispose();

to your test. That should reduce the amount of finalization you see.

However, there does appear to be a bug beyond that. @bartonjs, the StorePal.CopyTo's method use of SafeHandles is really strange: it's bending over backwards to try to keep pointers in safe handles only to then extract the underlying handle from each in a "dangerous" way. As a result, it's paying additional cost but isn't actually being any safer, and it's not disposing of a bunch of SafeHandles it seems it should, incurring even more cost. Even adding the aforementioned Dispose to get rid of a bunch of finalization, you can still see lots of finalization with this stack creating the handles:

   at Internal.Cryptography.Pal.Native.SafeCertContextHandle..ctor()
   at Interop.crypt32.CertEnumCertificatesInStore(SafeCertStoreHandle hCertStore, CERT_CONTEXT* pPrevCertContext)
   at Interop.crypt32.CertEnumCertificatesInStore(SafeCertStoreHandle hCertStore, SafeCertContextHandle& pCertContext)
   at Internal.Cryptography.Pal.StorePal.CopyTo(X509Certificate2Collection collection)
   at Internal.Cryptography.Pal.StorePal.CloneTo(X509Certificate2Collection collection)
   at System.Security.Cryptography.X509Certificates.X509Store.get_Certificates()

and all for handles that have IsInvalid==true. The handles are all transferred to something that will be disposed of, so it's doesn't appear to actually be a leak, but lots of invalid SafeHandle objects are getting left for finalization unnecessarily. It seems like this line:

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

Line 157 in 628c14b

CERT_CONTEXT* pPrevCertContext = pCertContext == null ? null : pCertContext.Disconnect();

should actually be:

CERT_CONTEXT* pPrevCertContext = null;
if (pCertContext != null)
{
    pPrevCertContext = pCertContext.Disconnect();
    pCertContext.Dispose();
}

and then also here:

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

Line 45 in 628c14b

}

there should be an additional call like:

pCertContext.Dispose();

after the loop to dispose of the final SafeHandle. But, again, I question how SafeHandles are being used here at all: it seems like this would all be much simpler if CertEnumCertificatesInStore just worked in terms of pointers rather than SafeHandles.

[brporter]

@stephentoub Enumerating the returned certificates collection and disposing of the certificates individually does reduce, but doesn't eliminate, the spike in finalized objects.

[stephentoub]

individually does reduce, but doesn't eliminate, the spike in finalized objects.

Right, see the remainder of my comment (which I added subsequent to initially posting).