peer review on "WOLFSSL_CLEANUP_THREADSAFE":

* add WOLFSSL_ATOMIC_INITIALIZER() to wc_port.h;
* rename feature macro to WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS for clarity;
* remove spin lock logic in wolfSSL_Init() and instead return DEADLOCK_AVERTED_E on contended initialization;
* unless WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS is user-defined to 0, automatically enable it when appropriate.
pull/8169/head
Daniel Pouzzner 2024-11-12 23:57:35 -06:00
parent b8aeaf4fa8
commit 524f0f5799
2 changed files with 43 additions and 38 deletions

View File

@ -5641,15 +5641,31 @@ int AddCA(WOLFSSL_CERT_MANAGER* cm, DerBuffer** pDer, int type, int verify)
static int wolfSSL_RAND_InitMutex(void); static int wolfSSL_RAND_InitMutex(void);
#endif #endif
#if defined(WOLFSSL_CLEANUP_THREADSAFE) && !defined(WOLFSSL_MUTEX_INITIALIZER) /* If we don't have static mutex initializers, but we do have static atomic
#ifndef WOLFSSL_ATOMIC_OPS * initializers, activate WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS to leverage
#error WOLFSSL_CLEANUP_THREADSAFE with !WOLFSSL_MUTEX_INITIALIZER requires WOLFSSL_ATOMIC_OPS * the latter.
*/
#ifndef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
#if !defined(WOLFSSL_MUTEX_INITIALIZER) && !defined(SINGLE_THREADED) && \
defined(WOLFSSL_ATOMIC_OPS) && defined(WOLFSSL_ATOMIC_INITIALIZER)
#define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 1
#else
#define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0
#endif #endif
/* WOLFSSL_CLEANUP_THREADSAFE depends on a wolfSSL_Atomic_Int that can #elif defined(WOLFSSL_MUTEX_INITIALIZER) || defined(SINGLE_THREADED)
* be statically initialized. #undef WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
*/ #define WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS 0
static wolfSSL_Atomic_Int inits_count_mutex_valid2 = 0; #endif
#endif /* WOLFSSL_CLEANUP_THREADSAFE && !WOLFSSL_MUTEX_INITIALIZER */
#if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
#ifndef WOLFSSL_ATOMIC_OPS
#error WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS with !WOLFSSL_MUTEX_INITIALIZER requires WOLFSSL_ATOMIC_OPS
#endif
#ifndef WOLFSSL_ATOMIC_INITIALIZER
#error WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS with !WOLFSSL_MUTEX_INITIALIZER requires WOLFSSL_ATOMIC_INITIALIZER
#endif
static wolfSSL_Atomic_Int inits_count_mutex_valid2 = WOLFSSL_ATOMIC_INITIALIZER(0);
#endif /* WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS && !WOLFSSL_MUTEX_INITIALIZER */
#if defined(OPENSSL_EXTRA) && defined(HAVE_ATEXIT) #if defined(OPENSSL_EXTRA) && defined(HAVE_ATEXIT)
static void AtExitCleanup(void) static void AtExitCleanup(void)
@ -5657,7 +5673,7 @@ static void AtExitCleanup(void)
if (initRefCount > 0) { if (initRefCount > 0) {
initRefCount = 1; initRefCount = 1;
(void)wolfSSL_Cleanup(); (void)wolfSSL_Cleanup();
#if defined(WOLFSSL_CLEANUP_THREADSAFE) && !defined(WOLFSSL_MUTEX_INITIALIZER) #if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
if (inits_count_mutex_valid == 1) { if (inits_count_mutex_valid == 1) {
(void)wc_FreeMutex(&inits_count_mutex); (void)wc_FreeMutex(&inits_count_mutex);
inits_count_mutex_valid = 0; inits_count_mutex_valid = 0;
@ -5679,37 +5695,22 @@ int wolfSSL_Init(void)
WOLFSSL_ENTER("wolfSSL_Init"); WOLFSSL_ENTER("wolfSSL_Init");
#ifndef WOLFSSL_MUTEX_INITIALIZER #ifndef WOLFSSL_MUTEX_INITIALIZER
if (inits_count_mutex_valid <= 0) { if (inits_count_mutex_valid == 0) {
#ifdef WOLFSSL_CLEANUP_THREADSAFE #if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
int current_inits_count_mutex_valid;
if (wolfSSL_Atomic_Int_FetchAdd(&inits_count_mutex_valid2, 1) != 0) { if (wolfSSL_Atomic_Int_FetchAdd(&inits_count_mutex_valid2, 1) != 0) {
/* We treat inits_count_mutex_valid as a spin lock -- this may cause (void)wolfSSL_Atomic_Int_FetchSub(&inits_count_mutex_valid2, 1);
* deadlocks in some runtimes. Applications can fully mitigate this return DEADLOCK_AVERTED_E;
* race by calling wolfSSL_Init() and wolfSSL_Cleanup()
* consecutively at startup, uncontended. This leaves
* inits_count_mutex safely initialized, but releases all other
* resources.
*/
while ((current_inits_count_mutex_valid = inits_count_mutex_valid) == 0);
if (current_inits_count_mutex_valid < 0) {
(void)wolfSSL_Atomic_Int_FetchSub(&inits_count_mutex_valid2, 1);
return BAD_MUTEX_E;
}
} }
else #endif /* WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS */
#endif /* WOLFSSL_CLEANUP_THREADSAFE */ if (wc_InitMutex(&inits_count_mutex) != 0) {
{ WOLFSSL_MSG("Bad Init Mutex count");
if (wc_InitMutex(&inits_count_mutex) != 0) { #if WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
WOLFSSL_MSG("Bad Init Mutex count"); (void)wolfSSL_Atomic_Int_FetchSub(&inits_count_mutex_valid2, 1);
inits_count_mutex_valid = -1;
#ifdef WOLFSSL_CLEANUP_THREADSAFE
(void)wolfSSL_Atomic_Int_FetchSub(&inits_count_mutex_valid2, 1);
#endif #endif
return BAD_MUTEX_E; return BAD_MUTEX_E;
} }
else { else {
inits_count_mutex_valid = 1; inits_count_mutex_valid = 1;
}
} }
} }
#endif /* !WOLFSSL_MUTEX_INITIALIZER */ #endif /* !WOLFSSL_MUTEX_INITIALIZER */
@ -10463,7 +10464,8 @@ int wolfSSL_Cleanup(void)
#endif #endif
#endif /* !NO_SESSION_CACHE */ #endif /* !NO_SESSION_CACHE */
#if !defined(WOLFSSL_CLEANUP_THREADSAFE) && !defined(WOLFSSL_MUTEX_INITIALIZER) #if !defined(WOLFSSL_MUTEX_INITIALIZER) && \
!WOLFSSL_CLEANUP_THREADSAFE_BY_ATOMIC_OPS
if ((inits_count_mutex_valid == 1) && if ((inits_count_mutex_valid == 1) &&
(wc_FreeMutex(&inits_count_mutex) != 0)) { (wc_FreeMutex(&inits_count_mutex) != 0)) {
if (ret == WOLFSSL_SUCCESS) if (ret == WOLFSSL_SUCCESS)

View File

@ -340,6 +340,7 @@
#if defined(__GNUC__) && defined(__ATOMIC_RELAXED) #if defined(__GNUC__) && defined(__ATOMIC_RELAXED)
/* C++ using direct calls to compiler built-in functions */ /* C++ using direct calls to compiler built-in functions */
typedef volatile int wolfSSL_Atomic_Int; typedef volatile int wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS #define WOLFSSL_ATOMIC_OPS
#endif #endif
#else #else
@ -347,6 +348,7 @@
/* Default C Implementation */ /* Default C Implementation */
#include <stdatomic.h> #include <stdatomic.h>
typedef atomic_int wolfSSL_Atomic_Int; typedef atomic_int wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS #define WOLFSSL_ATOMIC_OPS
#endif /* WOLFSSL_HAVE_ATOMIC_H */ #endif /* WOLFSSL_HAVE_ATOMIC_H */
#endif #endif
@ -358,6 +360,7 @@
#include <intrin.h> #include <intrin.h>
#endif #endif
typedef volatile long wolfSSL_Atomic_Int; typedef volatile long wolfSSL_Atomic_Int;
#define WOLFSSL_ATOMIC_INITIALIZER(x) (x)
#define WOLFSSL_ATOMIC_OPS #define WOLFSSL_ATOMIC_OPS
#endif #endif
#endif /* WOLFSSL_NO_ATOMICS */ #endif /* WOLFSSL_NO_ATOMICS */