From a3359946ca0857074a49b65b589c3f6924258ba9 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 11 Jan 2021 12:40:41 -0500 Subject: [PATCH 1/2] Avoid OidLookup.ToOid call per extension in X509Certificate2.Extensions `X509Certificate2.Extensions` invokes `CertificatePal.Extensions`, which in turn creates the extensions collection, invoking `new Oid(string)` for each. This in turn calls `OidLookup.ToOid` in order to gather the friendly name, even though in many situations, no one actually cares about the friendly name. We can instead call `new Oid(string, null)`, which makes the friendly name lazily initialized on first use, saving the `OidLookup.ToOid` call when it's not needed, and significantly reducing the time to call Extensions (in particular when the predefined OID lookup tables don't contain the OID for an extension and when it can't be found on lookup). --- .../src/Internal/Cryptography/Pal.Windows/CertificatePal.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.cs index 10ac6e7480a1f..3b01e58aa2178 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/CertificatePal.cs @@ -377,7 +377,7 @@ public IEnumerable Extensions { CERT_EXTENSION* pCertExtension = pCertInfo->rgExtension + i; string oidValue = Marshal.PtrToStringAnsi(pCertExtension->pszObjId)!; - Oid oid = new Oid(oidValue); + Oid oid = new Oid(oidValue, friendlyName: null); bool critical = pCertExtension->fCritical != 0; byte[] rawData = pCertExtension->Value.ToByteArray(); From 5596cf6b14f6b99093965528100a68c2e77bbe8a Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 11 Jan 2021 12:45:34 -0500 Subject: [PATCH 2/2] Allow OidLookup.ToOid to cache failure ToOid has a cache, but it only caches successful results. If ToOid fails to find the relevant OID, nothing is cached, which makes the failure path very expensive, as every ToOid call for that OID takes the slow path. This lets it be cached. --- .../src/Internal/Cryptography/OidLookup.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs b/src/libraries/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs index a3fb3cf9acda4..7a72349a042c7 100644 --- a/src/libraries/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs +++ b/src/libraries/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs @@ -14,8 +14,8 @@ internal static partial class OidLookup private static readonly ConcurrentDictionary s_lateBoundOidToFriendlyName = new ConcurrentDictionary(); - private static readonly ConcurrentDictionary s_lateBoundFriendlyNameToOid = - new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); + private static readonly ConcurrentDictionary s_lateBoundFriendlyNameToOid = + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); // // Attempts to map a friendly name to an OID. Returns null if not a known name. @@ -80,13 +80,19 @@ internal static partial class OidLookup mappedOid = NativeFriendlyNameToOid(friendlyName, oidGroup, fallBackToAllGroups); - if (shouldUseCache && mappedOid != null) + if (shouldUseCache) { s_lateBoundFriendlyNameToOid.TryAdd(friendlyName, mappedOid); // Don't add the reverse here. Friendly Name => OID is a case insensitive search, // so the casing provided as input here may not be the 'correct' one. Just let // ToFriendlyName capture the response and cache it itself. + + // Also, mappedOid could be null here if the lookup failed. Allowing storing null + // means we're able to cache that a lookup failed so we don't repeat it. It's + // theoretically possible, however, the failure could have been intermittent, e.g. + // if the call was forced to follow an AD fallback path and the relevant servers + // were offline. } return mappedOid;