Fix init_get_thread_local()

Previously, init_get_thread_local() pushed the thread event handler
list onto the global register before calling CRYPTO_THREAD_set_local(),
and when the latter failed, forgot to pop the list from the stack again.

Instead of cleaning the stack on error, this commit avoids the situation
entirely by postponing the push operation until all other operations
succeeded. This reordering also significantly reduces the scope of the
critical section.

Another simplification of the code is achieved by moving the push operation
onto the register (which is disabled in FIPS mode) into a separate function.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/9401)

diff --git a/crypto/initthread.c b/crypto/initthread.c
index b3f45b9dc49c6f447b42a0ad20b7fe1339f3bf7b..7de8a3694519a03338a11ff537d08b76adb0db70 100644 (file)
--- a/crypto/initthread.c
+++ b/crypto/initthread.c
@@ -77,6 +77,12 @@ static GLOBAL_TEVENT_REGISTER *get_global_tevent_register(void)
 }
 #endif
 
+#ifndef FIPS_MODE
+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);
+static int  init_thread_deregister(void *arg, int all);
+#endif
 static void init_thread_stop(void *arg, THREAD_EVENT_HANDLER **hands);
 
 static THREAD_EVENT_HANDLER **
@@ -86,40 +92,22 @@ init_get_thread_local(CRYPTO_THREAD_LOCAL *local, int alloc, int keep)
 
     if (alloc) {
         if (hands == NULL) {
-#ifndef FIPS_MODE
-            GLOBAL_TEVENT_REGISTER *gtr;
-#endif
 
-            if ((hands = OPENSSL_zalloc(sizeof(*hands))) == NULL) {
-                OPENSSL_free(hands);
+            if ((hands = OPENSSL_zalloc(sizeof(*hands))) == NULL)
                 return NULL;
-            }
 
-#ifndef FIPS_MODE
-            /*
-             * The thread event handler is thread specific and is a linked
-             * list of all handler functions that should be called for the
-             * current thread. We also keep a global reference to that linked
-             * list, so that we can deregister handlers if necessary before all
-             * the threads are stopped.
-             */
-            gtr = get_global_tevent_register();
-            if (gtr == NULL) {
+            if (!CRYPTO_THREAD_set_local(local, hands)) {
                 OPENSSL_free(hands);
                 return NULL;
             }
-            CRYPTO_THREAD_write_lock(gtr->lock);
-            if (!sk_THREAD_EVENT_HANDLER_PTR_push(gtr->skhands, hands)) {
+
+#ifndef FIPS_MODE
+            if (!init_thread_push_handlers(hands)) {
+                CRYPTO_THREAD_set_local(local, NULL);
                 OPENSSL_free(hands);
-                CRYPTO_THREAD_unlock(gtr->lock);
                 return NULL;
             }
-            CRYPTO_THREAD_unlock(gtr->lock);
 #endif
-            if (!CRYPTO_THREAD_set_local(local, hands)) {
-                OPENSSL_free(hands);
-                return NULL;
-            }
         }
     } else if (!keep) {
         CRYPTO_THREAD_set_local(local, NULL);
@@ -148,6 +136,32 @@ static union {
     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
+ * current thread in case of certain events. (Currently, there is
+ * only one type of event, the 'thread stop' event.)
+ *
+ * We also keep a global reference to that linked list, so that we
+ * can deregister handlers if necessary before all the threads are
+ * stopped.
+ */
+static int init_thread_push_handlers(THREAD_EVENT_HANDLER **hands)
+{
+    int ret;
+    GLOBAL_TEVENT_REGISTER *gtr;
+
+    gtr = get_global_tevent_register();
+    if (gtr == NULL)
+        return 0;
+
+    CRYPTO_THREAD_write_lock(gtr->lock);
+    ret = (sk_THREAD_EVENT_HANDLER_PTR_push(gtr->skhands, hands) != 0);
+    CRYPTO_THREAD_unlock(gtr->lock);
+
+    return ret;
+}
+
 static void init_thread_remove_handlers(THREAD_EVENT_HANDLER **handsin)
 {
     GLOBAL_TEVENT_REGISTER *gtr;
@@ -187,8 +201,6 @@ int ossl_init_thread(void)
     return 1;
 }
 
-static int init_thread_deregister(void *arg, int all);
-
 void ossl_cleanup_thread(void)
 {
     init_thread_deregister(NULL, 1);