From e44abfafba0de547f01bce356cb0b32c33ff2472 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 6 May 2020 22:54:23 +0000 Subject: [PATCH 1/3] Add an atexit handler to bypass calls into ERR_ string routines. This works around Ubuntu's apparent lack of NO_ATEXIT support in their build of OpenSSL. --- .../openssl.c | 20 ++++++++++ .../pal_err.c | 38 ++++++++++++++++++- .../pal_err.h | 5 +++ 3 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c index 1a9ea04839756..3986ff4e76eae 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +#include "pal_err.h" #include "pal_types.h" #include "pal_utilities.h" #include "pal_safecrt.h" @@ -1263,6 +1264,21 @@ static int32_t EnsureOpenSsl10Initialized() #define OPENSSL_INIT_NO_ATEXIT 0x00080000L #endif +int volatile g_str_read_count = 0; +int volatile g_err_unloaded = 0; + +static void HandleShutdown() +{ + // Set this first, which stops new callers from using the error string tables. + g_err_unloaded = 1; + + // Now, spin until existing calls all finish, and we can move on with shutdown. + int x; + while ((x = __atomic_load_n(&g_str_read_count, __ATOMIC_SEQ_CST))) + { + } +} + static int32_t EnsureOpenSsl11Initialized() { // In OpenSSL 1.0 we call OPENSSL_add_all_algorithms_conf() and ERR_load_crypto_strings(), @@ -1279,6 +1295,10 @@ static int32_t EnsureOpenSsl11Initialized() OPENSSL_INIT_LOAD_SSL_STRINGS, NULL); + // As a fallback for when the NO_ATEXIT isn't respected, register a later + // atexit handler, so we will indicate that we're in the shutdown state + // and stop asking problematic questions from other threads. + atexit(HandleShutdown); return 0; } diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c index 252e7be59ce29..5dc8cc2a8c52a 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c @@ -34,10 +34,44 @@ uint64_t CryptoNative_ErrPeekLastError() const char* CryptoNative_ErrReasonErrorString(uint64_t error) { - return ERR_reason_error_string((unsigned long)error); + const char* errStr = NULL; + + if (!g_err_unloaded) + { +#ifdef NEED_OPENSSL_1_1 + __atomic_fetch_add(&g_str_read_count, 1, __ATOMIC_SEQ_CST); +#endif + + errStr = ERR_reason_error_string((unsigned long)error); + +#ifdef NEED_OPENSSL_1_1 + __atomic_fetch_add(&g_str_read_count, -1, __ATOMIC_SEQ_CST); +#endif + } + + return errStr; } void CryptoNative_ErrErrorStringN(uint64_t e, char* buf, int32_t len) { - ERR_error_string_n((unsigned long)e, buf, Int32ToSizeT(len)); + if (!g_err_unloaded) + { +#ifdef NEED_OPENSSL_1_1 + __atomic_fetch_add(&g_str_read_count, 1, __ATOMIC_SEQ_CST); +#endif + + ERR_error_string_n((unsigned long)e, buf, Int32ToSizeT(len)); + +#ifdef NEED_OPENSSL_1_1 + __atomic_fetch_add(&g_str_read_count, -1, __ATOMIC_SEQ_CST); +#endif + } + else + { + // If there's no string table, just make it be the empty string. + if (buf != NULL && len > 0) + { + buf[0] = 0; + } + } } diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h index 3610c263d0af1..f1c0107d1cbf4 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h @@ -6,6 +6,11 @@ #include #include "opensslshim.h" +#ifdef NEED_OPENSSL_1_1 +extern int volatile g_str_read_count; +extern int volatile g_err_unloaded; +#endif + /* Shims the ERR_clear_error method. */ From e3ce8cd4a09ea3867946452d5a7976b1e3ea8a5a Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Thu, 7 May 2020 20:17:55 +0000 Subject: [PATCH 2/3] Use a mutex instead of clever interlocked counting. --- .../openssl.c | 16 ++++++----- .../pal_err.c | 28 +++++++++++-------- .../pal_err.h | 2 +- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c index 3986ff4e76eae..21820f8f443a1 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/openssl.c @@ -1264,19 +1264,21 @@ static int32_t EnsureOpenSsl10Initialized() #define OPENSSL_INIT_NO_ATEXIT 0x00080000L #endif -int volatile g_str_read_count = 0; +pthread_mutex_t g_err_mutex = PTHREAD_MUTEX_INITIALIZER; int volatile g_err_unloaded = 0; static void HandleShutdown() { - // Set this first, which stops new callers from using the error string tables. + // Generally, a mutex to set a boolean is overkill, but this lock + // ensures that there are no callers already inside the string table + // when the unload (possibly) executes. + int result = pthread_mutex_lock(&g_err_mutex); + assert(!result && "Acquiring the error string table mutex failed."); + g_err_unloaded = 1; - // Now, spin until existing calls all finish, and we can move on with shutdown. - int x; - while ((x = __atomic_load_n(&g_str_read_count, __ATOMIC_SEQ_CST))) - { - } + result = pthread_mutex_unlock(&g_err_mutex); + assert(!result && "Releasing the error string table mutex failed."); } static int32_t EnsureOpenSsl11Initialized() diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c index 5dc8cc2a8c52a..e38aaf71f7aec 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.c @@ -36,35 +36,35 @@ const char* CryptoNative_ErrReasonErrorString(uint64_t error) { const char* errStr = NULL; +#ifdef NEED_OPENSSL_1_1 + int result = pthread_mutex_lock(&g_err_mutex); + assert(!result && "Acquiring the error string table mutex failed."); + if (!g_err_unloaded) { -#ifdef NEED_OPENSSL_1_1 - __atomic_fetch_add(&g_str_read_count, 1, __ATOMIC_SEQ_CST); #endif - errStr = ERR_reason_error_string((unsigned long)error); - #ifdef NEED_OPENSSL_1_1 - __atomic_fetch_add(&g_str_read_count, -1, __ATOMIC_SEQ_CST); -#endif } + result = pthread_mutex_unlock(&g_err_mutex); + assert(!result && "Releasing the error string table mutex failed."); +#endif + return errStr; } void CryptoNative_ErrErrorStringN(uint64_t e, char* buf, int32_t len) { +#ifdef NEED_OPENSSL_1_1 + int result = pthread_mutex_lock(&g_err_mutex); + assert(!result && "Acquiring the error string table mutex failed."); + if (!g_err_unloaded) { -#ifdef NEED_OPENSSL_1_1 - __atomic_fetch_add(&g_str_read_count, 1, __ATOMIC_SEQ_CST); #endif - ERR_error_string_n((unsigned long)e, buf, Int32ToSizeT(len)); - #ifdef NEED_OPENSSL_1_1 - __atomic_fetch_add(&g_str_read_count, -1, __ATOMIC_SEQ_CST); -#endif } else { @@ -74,4 +74,8 @@ void CryptoNative_ErrErrorStringN(uint64_t e, char* buf, int32_t len) buf[0] = 0; } } + + result = pthread_mutex_unlock(&g_err_mutex); + assert(!result && "Releasing the error string table mutex failed."); +#endif } diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h index f1c0107d1cbf4..734cd8d5ca49f 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h @@ -7,7 +7,7 @@ #include "opensslshim.h" #ifdef NEED_OPENSSL_1_1 -extern int volatile g_str_read_count; +extern pthread_mutex_t g_err_mutex; extern int volatile g_err_unloaded; #endif From bfb5c814d93e5f8740661640874f5f0a07bf6fdb Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Fri, 8 May 2020 15:02:20 +0000 Subject: [PATCH 3/3] Add extra #include to make build machines happy. --- .../Native/Unix/System.Security.Cryptography.Native/pal_err.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h index 734cd8d5ca49f..1c7470251e411 100644 --- a/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h +++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_err.h @@ -6,6 +6,8 @@ #include #include "opensslshim.h" +#include + #ifdef NEED_OPENSSL_1_1 extern pthread_mutex_t g_err_mutex; extern int volatile g_err_unloaded;