Refactor init_get_thread_local to be more understandable

We currently have a single function that does thread_local key
allocation/cleanup/fetching for our OSSL_init_thread_start/stop apis,
and its pretty confusing.  Wrap it up in some helper functions to make
it more clear at the call sites what we're trying to do.

Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27794)

diff --git a/crypto/initthread.c b/crypto/initthread.c
index ea591dcb89750a6edd0612e711b160e5330c3e42..50da74fec36fc9cbfdae737f0c679620673555c0 100644 (file)
--- a/crypto/initthread.c
+++ b/crypto/initthread.c
@@ -84,6 +84,25 @@ static GLOBAL_TEVENT_REGISTER *get_global_tevent_register(void)
 #endif
 
 #ifndef FIPS_MODULE
+/*
+ * Since per-thread-specific-data destructors are not universally
+ * available, i.e. not on Windows, only below CRYPTO_THREAD_LOCAL key
+ * is assumed to have destructor associated. And then an effort is made
+ * to call this single destructor on non-pthread platform[s].
+ *
+ * Initial value is "impossible". It is used as guard value to shortcut
+ * destructor for threads terminating before libcrypto is initialized or
+ * after it's de-initialized. Access to the key doesn't have to be
+ * serialized for the said threads, because they didn't use libcrypto
+ * and it doesn't matter if they pick "impossible" or dereference real
+ * key value and pull NULL past initialization in the first thread that
+ * intends to use libcrypto.
+ */
+static union {
+    long sane;
+    CRYPTO_THREAD_LOCAL value;
+} destructor_key = { -1 };
+
 static int  init_thread_push_handlers(THREAD_EVENT_HANDLER **hands);
 static void init_thread_remove_handlers(THREAD_EVENT_HANDLER **handsin);
 static void init_thread_destructor(void *hands);
@@ -91,11 +110,32 @@ static int  init_thread_deregister(void *arg, int all);
 #endif
 static void init_thread_stop(void *arg, THREAD_EVENT_HANDLER **hands);
 
-#ifndef FIPS_MODULE
+static THREAD_EVENT_HANDLER ** get_thread_event_handler(OSSL_LIB_CTX *ctx)
+{
+#ifdef FIPS_MODULE
+    return CRYPTO_THREAD_get_local_ex(CRYPTO_THREAD_LOCAL_TEVENT_KEY, ctx);
+#else
+    if (destructor_key.sane != -1)
+        return CRYPTO_THREAD_get_local(&destructor_key.value);
+    return NULL;
+#endif
+}
+
+static int set_thread_event_handler(OSSL_LIB_CTX *ctx, THREAD_EVENT_HANDLER **hands)
+{
+#ifdef FIPS_MODULE
+    return CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_TEVENT_KEY, ctx, hands);
+#else
+    if (destructor_key.sane != -1)
+        return CRYPTO_THREAD_set_local(&destructor_key.value, hands);
+    return 0;
+#endif
+}
+
 static THREAD_EVENT_HANDLER **
-init_get_thread_local(CRYPTO_THREAD_LOCAL *local, int alloc, int keep)
+init_manage_thread_local(OSSL_LIB_CTX *ctx, int alloc, int keep)
 {
-    THREAD_EVENT_HANDLER **hands = CRYPTO_THREAD_get_local(local);
+    THREAD_EVENT_HANDLER **hands = get_thread_event_handler(ctx);
 
     if (alloc) {
         if (hands == NULL) {
@@ -103,70 +143,41 @@ init_get_thread_local(CRYPTO_THREAD_LOCAL *local, int alloc, int keep)
             if ((hands = OPENSSL_zalloc(sizeof(*hands))) == NULL)
                 return NULL;
 
-            if (!CRYPTO_THREAD_set_local(local, hands)) {
+            if (!set_thread_event_handler(ctx, hands)) {
                 OPENSSL_free(hands);
                 return NULL;
             }
-
+#ifndef FIPS_MODULE
             if (!init_thread_push_handlers(hands)) {
-                CRYPTO_THREAD_set_local(local, NULL);
+                set_thread_event_handler(ctx, NULL);
                 OPENSSL_free(hands);
                 return NULL;
             }
+#endif
         }
     } else if (!keep) {
-        CRYPTO_THREAD_set_local(local, NULL);
+        set_thread_event_handler(ctx, NULL);
     }
 
     return hands;
 }
 
-#else
-static THREAD_EVENT_HANDLER **
-init_get_thread_local_ex(OSSL_LIB_CTX *ctx, int alloc, int keep)
+static ossl_inline THREAD_EVENT_HANDLER **init_fetch_clear_thread_local(OSSL_LIB_CTX *ctx)
 {
-    THREAD_EVENT_HANDLER **hands = CRYPTO_THREAD_get_local_ex(CRYPTO_THREAD_LOCAL_TEVENT_KEY, ctx);
-
-    if (alloc) {
-        if (hands == NULL) {
-
-            if ((hands = OPENSSL_zalloc(sizeof(*hands))) == NULL)
-                return NULL;
-
-            if (!CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_TEVENT_KEY, ctx, hands)) {
-                OPENSSL_free(hands);
-                return NULL;
-            }
-        }
-    } else if (!keep) {
-        CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_TEVENT_KEY, ctx, NULL);
-    }
+    return init_manage_thread_local(ctx, 0, 0);
+}
 
-    return hands;
+static ossl_inline ossl_unused THREAD_EVENT_HANDLER **init_fetch_thread_local(OSSL_LIB_CTX *ctx)
+{
+    return init_manage_thread_local(ctx, 0, 1);
 }
 
-#endif
+static ossl_inline THREAD_EVENT_HANDLER **init_fetch_alloc_thread_local(OSSL_LIB_CTX *ctx)
+{
+    return init_manage_thread_local(ctx, 1, 0);
+}
 
 #ifndef FIPS_MODULE
-/*
- * Since per-thread-specific-data destructors are not universally
- * available, i.e. not on Windows, only below CRYPTO_THREAD_LOCAL key
- * is assumed to have destructor associated. And then an effort is made
- * to call this single destructor on non-pthread platform[s].
- *
- * Initial value is "impossible". It is used as guard value to shortcut
- * destructor for threads terminating before libcrypto is initialized or
- * after it's de-initialized. Access to the key doesn't have to be
- * serialized for the said threads, because they didn't use libcrypto
- * and it doesn't matter if they pick "impossible" or dereference real
- * key value and pull NULL past initialization in the first thread that
- * intends to use libcrypto.
- */
-static union {
-    long sane;
-    CRYPTO_THREAD_LOCAL value;
-} destructor_key = { -1 };
-
 /*
  * The thread event handler list is a thread specific linked list
  * of callback functions which are invoked in list order by the
@@ -255,8 +266,8 @@ void OPENSSL_thread_stop_ex(OSSL_LIB_CTX *ctx)
 void OPENSSL_thread_stop(void)
 {
     if (destructor_key.sane != -1) {
-        THREAD_EVENT_HANDLER **hands
-            = init_get_thread_local(&destructor_key.value, 0, 0);
+        THREAD_EVENT_HANDLER **hands = init_fetch_clear_thread_local(NULL);
+
         init_thread_stop(NULL, hands);
 
         init_thread_remove_handlers(hands);
@@ -267,8 +278,8 @@ void OPENSSL_thread_stop(void)
 void ossl_ctx_thread_stop(OSSL_LIB_CTX *ctx)
 {
     if (destructor_key.sane != -1) {
-        THREAD_EVENT_HANDLER **hands
-            = init_get_thread_local(&destructor_key.value, 0, 1);
+        THREAD_EVENT_HANDLER **hands = init_fetch_thread_local(ctx);
+
         init_thread_stop(ctx, hands);
     }
 }
@@ -325,7 +336,7 @@ void ossl_ctx_thread_stop(OSSL_LIB_CTX *ctx)
 {
     THREAD_EVENT_HANDLER **hands;
 
-    hands = init_get_thread_local_ex(ctx, 0, 0);
+    hands = init_fetch_clear_thread_local(ctx);
     init_thread_stop(ctx, hands);
     OPENSSL_free(hands);
 }
@@ -380,26 +391,19 @@ int ossl_init_thread_start(const void *index, void *arg,
 {
     THREAD_EVENT_HANDLER **hands;
     THREAD_EVENT_HANDLER *hand;
+    OSSL_LIB_CTX *ctx = NULL;
 #ifdef FIPS_MODULE
-    OSSL_LIB_CTX *ctx = arg;
-
     /*
      * In FIPS mode the list of THREAD_EVENT_HANDLERs is unique per combination
      * of OSSL_LIB_CTX and thread. This is because in FIPS mode each
      * OSSL_LIB_CTX gets informed about thread stop events individually.
      */
-    hands = init_get_thread_local_ex(ctx, 1, 0);
-#else
-    /*
-     * Outside of FIPS mode the list of THREAD_EVENT_HANDLERs is unique per
-     * thread, but may hold multiple OSSL_LIB_CTXs. We only get told about
-     * thread stop events globally, so we have to ensure all affected
-     * OSSL_LIB_CTXs are informed.
-     */
-    CRYPTO_THREAD_LOCAL *local = &destructor_key.value;
-    hands = init_get_thread_local(local, 1, 0);
+
+    ctx = arg;
 #endif
 
+    hands = init_fetch_alloc_thread_local(ctx);
+
     if (hands == NULL)
         return 0;