Clean the OpenSSL error queue more consistently

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

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Buffers;
 using System.Diagnostics;
 using System.Runtime.InteropServices;
 using System.Security.Cryptography;
@@ -13,8 +14,8 @@ internal static partial class Crypto
         [GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_ErrClearError")]
         internal static partial ulong ErrClearError();
 
-        [GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_ErrGetErrorAlloc")]
-        private static partial ulong ErrGetErrorAlloc([MarshalAs(UnmanagedType.Bool)] out bool isAllocFailure);
+        [GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_ErrGetExceptionError")]
+        private static partial ulong ErrGetExceptionError([MarshalAs(UnmanagedType.Bool)] out bool isAllocFailure);
 
         [GeneratedDllImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_ErrPeekError")]
         internal static partial ulong ErrPeekError();
@@ -30,43 +31,38 @@ internal static partial class Crypto
 
         private static unsafe string ErrErrorStringN(ulong error)
         {
-            var buffer = new byte[1024];
+            byte[] buffer = ArrayPool<byte>.Shared.Rent(1024);
+            string ret;
+
             fixed (byte* buf = &buffer[0])
             {
                 ErrErrorStringN(error, buf, buffer.Length);
-                return Marshal.PtrToStringAnsi((IntPtr)buf)!;
+                ret = Marshal.PtrToStringAnsi((IntPtr)buf)!;
             }
+
+            ArrayPool<byte>.Shared.Return(buffer);
+            return ret;
         }
 
         internal static Exception CreateOpenSslCryptographicException()
         {
-            // The Windows cryptography library reports error codes through
-            // Marshal.GetLastWin32Error, which has a single value when the
-            // function exits, last writer wins.
+            // The Windows cryptography librares reports error codes through
+            // return values, or Marshal.GetLastWin32Error, either of which
+            // has a single value when the function exits.
             //
             // OpenSSL maintains an error queue. Calls to ERR_get_error read
             // values out of the queue in the order that ERR_set_error wrote
             // them. Nothing enforces that a single call into an OpenSSL
-            // function will guarantee at-most one error being set.
+            // function will guarantee at-most one error being set, and there
+            // are well-known cases where multiple errors are emitted.
             //
-            // In order to maintain parity in how error flows look between the
-            // Windows code and the OpenSSL-calling code, drain the queue
-            // whenever an Exception is desired, and report the exception
-            // related to the last value in the queue.
-            bool isAllocFailure;
-            ulong error = ErrGetErrorAlloc(out isAllocFailure);
-            ulong lastRead = error;
-            bool lastIsAllocFailure = isAllocFailure;
-
-            // 0 (there's no named constant) is only returned when the calls
-            // to ERR_get_error exceed the calls to ERR_set_error.
-            while (lastRead != 0)
-            {
-                error = lastRead;
-                isAllocFailure = lastIsAllocFailure;
-
-                lastRead = ErrGetErrorAlloc(out lastIsAllocFailure);
-            }
+            // In older versions of .NET, we collected the last error in the
+            // queue, by repeatedly calling into ERR_get_error from managed code
+            // and using the last error as the basis of the exception.
+            // Now, we call into the shim once, which is responsible for
+            // maintaining the error state and informing us of the one value to report.
+            // (and when fetching that error we go ahead and clear out the rest).
+            ulong error = ErrGetExceptionError(out bool isAllocFailure);
 
             // If we're in an error flow which results in an Exception, but
             // no calls to ERR_set_error were made, throw the unadorned
@@ -82,7 +78,8 @@ internal static Exception CreateOpenSslCryptographicException()
             }
 
             // Even though ErrGetError returns ulong (C++ unsigned long), we
-            // really only expect error codes in the UInt32 range
+            // really only expect error codes in the UInt32 range, since that
+            // type is only 32 bits on x86 Linux.
             Debug.Assert(error <= uint.MaxValue, "ErrGetError should only return error codes in the UInt32 range.");
 
             // If there was an error code, and it wasn't something handled specially,

src/native/libs/System.Security.Cryptography.Native/entrypoints.c

@@ -89,7 +89,7 @@ static const Entry s_cryptoNative[] =
     DllImportEntry(CryptoNative_EncodeX509SubjectPublicKeyInfo)
     DllImportEntry(CryptoNative_ErrClearError)
     DllImportEntry(CryptoNative_ErrErrorStringN)
-    DllImportEntry(CryptoNative_ErrGetErrorAlloc)
+    DllImportEntry(CryptoNative_ErrGetExceptionError)
     DllImportEntry(CryptoNative_ErrPeekError)
     DllImportEntry(CryptoNative_ErrPeekLastError)
     DllImportEntry(CryptoNative_ErrReasonErrorString)

src/native/libs/System.Security.Cryptography.Native/openssl.c

@@ -81,6 +81,8 @@ int32_t CryptoNative_GetX509Thumbprint(X509* x509, uint8_t* pBuf, int32_t cBuf)
         return -SHA_DIGEST_LENGTH;
     }
 
+    ERR_clear_error();
+
     if (!X509_digest(x509, EVP_sha1(), pBuf, NULL))
     {
         return 0;
@@ -102,6 +104,8 @@ otherwise.
 */
 const ASN1_TIME* CryptoNative_GetX509NotBefore(X509* x509)
 {
+    // No error queue impact.
+
     if (x509)
     {
         return X509_get0_notBefore(x509);
@@ -123,6 +127,8 @@ otherwise.
 */
 const ASN1_TIME* CryptoNative_GetX509NotAfter(X509* x509)
 {
+    // No error queue impact.
+
     if (x509)
     {
         return X509_get0_notAfter(x509);
@@ -144,6 +150,8 @@ otherwise.
 */
 const ASN1_TIME* CryptoNative_GetX509CrlNextUpdate(X509_CRL* crl)
 {
+    // No error queue impact.
+
     if (crl)
     {
         return X509_CRL_get0_nextUpdate(crl);
@@ -168,6 +176,9 @@ The encoded value of the version, otherwise:
 */
 int32_t CryptoNative_GetX509Version(X509* x509)
 {
+    // No errors are expected to be written to the queue on this call,
+    // and the managed caller doesn't check for one.
+
     if (x509)
     {
         return (int32_t)X509_get_version(x509);
@@ -189,6 +200,8 @@ describing the object type.
 */
 ASN1_OBJECT* CryptoNative_GetX509PublicKeyAlgorithm(X509* x509)
 {
+    // No error queue impact, all of the called routines are just field accessors.
+
     if (x509)
     {
         X509_PUBKEY* pubkey = X509_get_X509_PUBKEY(x509);
@@ -216,6 +229,8 @@ describing the object type.
 */
 ASN1_OBJECT* CryptoNative_GetX509SignatureAlgorithm(X509* x509)
 {
+    // No error queue impact.
+
     if (x509)
     {
         const X509_ALGOR* sigAlg = X509_get0_tbs_sigalg(x509);
@@ -243,6 +258,8 @@ Any negative value: The input buffer size was reported as insufficient. A buffer
 */
 int32_t CryptoNative_GetX509PublicKeyParameterBytes(X509* x509, uint8_t* pBuf, int32_t cBuf)
 {
+    ERR_clear_error();
+
     if (!x509)
     {
         return 0;
@@ -302,6 +319,8 @@ the public key.
 */
 ASN1_BIT_STRING* CryptoNative_GetX509PublicKeyBytes(X509* x509)
 {
+    // No error queue impact.
+
     if (x509)
     {
         return X509_get0_pubkey_bitstr(x509);
@@ -345,6 +364,8 @@ Any negative value: The input buffer size was reported as insufficient. A buffer
 */
 int32_t CryptoNative_GetAsn1StringBytes(ASN1_STRING* asn1, uint8_t* pBuf, int32_t cBuf)
 {
+    // No error queue impact.
+
     if (!asn1 || cBuf < 0)
     {
         return 0;
@@ -380,6 +401,8 @@ Any negative value: The input buffer size was reported as insufficient. A buffer
 */
 int32_t CryptoNative_GetX509NameRawBytes(X509_NAME* x509Name, uint8_t* pBuf, int32_t cBuf)
 {
+    ERR_clear_error();
+
     const uint8_t* nameBuf;
     size_t nameBufLen;
 
@@ -433,6 +456,7 @@ Note that 0 does not always indicate an error, merely that GetX509EkuField shoul
 */
 int32_t CryptoNative_GetX509EkuFieldCount(EXTENDED_KEY_USAGE* eku)
 {
+    // No error queue impact.
     return sk_ASN1_OBJECT_num(eku);
 }
 
@@ -449,6 +473,7 @@ that particular OID.
 */
 ASN1_OBJECT* CryptoNative_GetX509EkuField(EXTENDED_KEY_USAGE* eku, int32_t loc)
 {
+    // No error queue impact.
     return sk_ASN1_OBJECT_value(eku, loc);
 }
 
@@ -467,6 +492,8 @@ BIO* CryptoNative_GetX509NameInfo(X509* x509, int32_t nameType, int32_t forIssue
 {
     static const char szOidUpn[] = "1.3.6.1.4.1.311.20.2.3";
 
+    ERR_clear_error();
+
     if (!x509 || nameType < NAME_TYPE_SIMPLE || nameType > NAME_TYPE_URL)
     {
         return NULL;
@@ -729,6 +756,8 @@ int32_t CryptoNative_CheckX509Hostname(X509* x509, const char* hostname, int32_t
     if (cchHostname < 0)
         return -5;
 
+    ERR_clear_error();
+
     // OpenSSL will treat a target hostname starting with '.' as special.
     // We don't expect target hostnames to start with '.', but if one gets in here, the fallback
     // and the mainline won't be the same... so just make it report false.
@@ -771,6 +800,8 @@ int32_t CryptoNative_CheckX509IpAddress(
     if (!addressBytes)
         return -6;
 
+    ERR_clear_error();
+
     int subjectNid = NID_commonName;
     int sanGenType = GEN_IPADD;
     GENERAL_NAMES* san = (GENERAL_NAMES*)(X509_get_ext_d2i(x509, NID_subject_alt_name, NULL, NULL));
@@ -848,6 +879,7 @@ Note that 0 does not always indicate an error, merely that GetX509StackField sho
 */
 int32_t CryptoNative_GetX509StackFieldCount(STACK_OF(X509) * stack)
 {
+    // No error queue impact.
     return sk_X509_num(stack);
 }
 
@@ -864,6 +896,7 @@ that particular element.
 */
 X509* CryptoNative_GetX509StackField(STACK_OF(X509) * stack, int loc)
 {
+    // No error queue impact.
     return sk_X509_value(stack, loc);
 }
 
@@ -876,6 +909,7 @@ when done with it.
 */
 void CryptoNative_RecursiveFreeX509Stack(STACK_OF(X509) * stack)
 {
+    // No error queue impact.
     sk_X509_pop_free(stack, X509_free);
 }
 
@@ -899,6 +933,8 @@ int32_t CryptoNative_X509StoreSetVerifyTime(X509_STORE* ctx,
                                             int32_t second,
                                             int32_t isDst)
 {
+    ERR_clear_error();
+
     if (!ctx)
     {
         return 0;
@@ -935,6 +971,7 @@ otherwise NULL.
 */
 X509* CryptoNative_ReadX509AsDerFromBio(BIO* bio)
 {
+    ERR_clear_error();
     return d2i_X509_bio(bio, NULL);
 }
 
@@ -955,6 +992,8 @@ OpenSSL's BIO_tell
 */
 int32_t CryptoNative_BioTell(BIO* bio)
 {
+    // No error queue impact.
+
     if (!bio)
     {
         return -1;
@@ -982,6 +1021,8 @@ OpenSSL's BIO_seek
 */
 int32_t CryptoNative_BioSeek(BIO* bio, int32_t ofs)
 {
+    // No error queue impact.
+
     if (!bio)
     {
         return -1;
@@ -1002,6 +1043,7 @@ A STACK_OF(X509*) with no comparator.
 */
 STACK_OF(X509) * CryptoNative_NewX509Stack()
 {
+    ERR_clear_error();
     return sk_X509_new_null();
 }
 
@@ -1018,6 +1060,8 @@ Return values:
 */
 int32_t CryptoNative_PushX509StackField(STACK_OF(X509) * stack, X509* x509)
 {
+    ERR_clear_error();
+
     if (!stack)
     {
         return 0;
@@ -1039,6 +1083,7 @@ Returns a bool to managed code.
 */
 int32_t CryptoNative_GetRandomBytes(uint8_t* buf, int32_t num)
 {
+    ERR_clear_error();
     int ret = RAND_bytes(buf, num);
 
     return ret == 1;
@@ -1063,6 +1108,8 @@ int32_t CryptoNative_LookupFriendlyNameByOid(const char* oidValue, const char**
     int nid;
     const char* ln;
 
+    ERR_clear_error();
+
     if (!oidValue || !friendlyName)
     {
         return -2;
@@ -1121,6 +1168,7 @@ Version number as MNNFFRBB (major minor fix final beta/patch)
 */
 int64_t CryptoNative_OpenSslVersionNumber()
 {
+    // No error queue impact.
     return (int64_t)OpenSSL_version_num();
 }
 
@@ -1130,6 +1178,9 @@ void CryptoNative_RegisterLegacyAlgorithms()
     if (API_EXISTS(OSSL_PROVIDER_try_load))
     {
         OSSL_PROVIDER_try_load(NULL, "legacy", 1);
+
+        // Doesn't matter if it succeeded or failed.
+        ERR_clear_error();
     }
 #endif
 }