Convert mem_dbg and mem_sec to the new Thread API

Use new Thread API style locks, and thread local storage for mem_dbg
Reviewed-by: NRich Salz <rsalz@openssl.org>

crypto/mem_dbg.c

@@ -112,6 +112,7 @@
 #include <stdlib.h>
 #include <time.h>
 #include "internal/cryptlib.h"
+#include "internal/threads.h"
 #include <openssl/crypto.h>
 #include <openssl/buffer.h>
 #include <openssl/bio.h>
@@ -147,7 +148,7 @@ static unsigned long order = 0; /* number of memory requests */
  *   OPENSSL_mem_debug_pop()     to pop an entry,
  */
 struct app_mem_info_st {
-    CRYPTO_THREADID threadid;
+    CRYPTO_THREAD_ID threadid;
     const char *file;
     int line;
     const char *info;
@@ -155,12 +156,10 @@ struct app_mem_info_st {
     int references;
 };
 
-/*
- * hash-table with those app_mem_info_st's that are at the
- * top of their thread's stack (with `thread' as key); access requires
- * MALLOC2 lock
- */
-static LHASH_OF(APP_INFO) *amih = NULL;
+static CRYPTO_ONCE memdbg_init = CRYPTO_ONCE_STATIC_INIT;
+static CRYPTO_RWLOCK *malloc_lock = NULL;
+static CRYPTO_RWLOCK *long_malloc_lock = NULL;
+static CRYPTO_THREAD_LOCAL appinfokey;
 
 /* memory-block description */
 struct mem_st {
@@ -168,7 +167,7 @@ struct mem_st {
     int num;
     const char *file;
     int line;
-    CRYPTO_THREADID threadid;
+    CRYPTO_THREAD_ID threadid;
     unsigned long order;
     time_t time;
     APP_INFO *app_info;
@@ -185,10 +184,17 @@ static LHASH_OF(MEM) *mh = NULL; /* hash-table of memory requests (address as
 static unsigned int num_disable = 0;
 
 /*
- * Valid iff num_disable > 0.  CRYPTO_LOCK_MALLOC2 is locked exactly in this
+ * Valid iff num_disable > 0.  long_malloc_lock is locked exactly in this
  * case (by the thread named in disabling_thread).
  */
-static CRYPTO_THREADID disabling_threadid;
+static CRYPTO_THREAD_ID disabling_threadid;
+
+static void do_memdbg_init(void)
+{
+    malloc_lock = CRYPTO_THREAD_lock_new();
+    long_malloc_lock = CRYPTO_THREAD_lock_new();
+    CRYPTO_THREAD_init_local(&appinfokey, NULL);
+}
 
 static void app_info_free(APP_INFO *inf)
 {
@@ -208,7 +214,9 @@ int CRYPTO_mem_ctrl(int mode)
 #else
     int ret = mh_mode;
 
-    CRYPTO_w_lock(CRYPTO_LOCK_MALLOC);
+    CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+
+    CRYPTO_THREAD_write_lock(malloc_lock);
     switch (mode) {
     default:
         break;
@@ -226,30 +234,29 @@ int CRYPTO_mem_ctrl(int mode)
     /* switch off temporarily (for library-internal use): */
     case CRYPTO_MEM_CHECK_DISABLE:
         if (mh_mode & CRYPTO_MEM_CHECK_ON) {
-            CRYPTO_THREADID cur;
-            CRYPTO_THREADID_current(&cur);
-            /* see if we don't have the MALLOC2 lock already */
+            CRYPTO_THREAD_ID cur = CRYPTO_THREAD_get_current_id();
+            /* see if we don't have long_malloc_lock already */
             if (!num_disable
-                || CRYPTO_THREADID_cmp(&disabling_threadid, &cur)) {
+                || !CRYPTO_THREAD_compare_id(disabling_threadid, cur)) {
                 /*
-                 * Long-time lock CRYPTO_LOCK_MALLOC2 must not be claimed
-                 * while we're holding CRYPTO_LOCK_MALLOC, or we'll deadlock
-                 * if somebody else holds CRYPTO_LOCK_MALLOC2 (and cannot
+                 * Long-time lock long_malloc_lock must not be claimed
+                 * while we're holding malloc_lock, or we'll deadlock
+                 * if somebody else holds long_malloc_lock (and cannot
                  * release it because we block entry to this function). Give
                  * them a chance, first, and then claim the locks in
                  * appropriate order (long-time lock first).
                  */
-                CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC);
+                CRYPTO_THREAD_unlock(malloc_lock);
                 /*
-                 * Note that after we have waited for CRYPTO_LOCK_MALLOC2 and
-                 * CRYPTO_LOCK_MALLOC, we'll still be in the right "case" and
+                 * Note that after we have waited for long_malloc_lock and
+                 * malloc_lock, we'll still be in the right "case" and
                  * "if" branch because MemCheck_start and MemCheck_stop may
                  * never be used while there are multiple OpenSSL threads.
                  */
-                CRYPTO_w_lock(CRYPTO_LOCK_MALLOC2);
-                CRYPTO_w_lock(CRYPTO_LOCK_MALLOC);
+                CRYPTO_THREAD_write_lock(long_malloc_lock);
+                CRYPTO_THREAD_write_lock(malloc_lock);
                 mh_mode &= ~CRYPTO_MEM_CHECK_ENABLE;
-                CRYPTO_THREADID_cpy(&disabling_threadid, &cur);
+                disabling_threadid = cur;
             }
             num_disable++;
         }
@@ -261,13 +268,13 @@ int CRYPTO_mem_ctrl(int mode)
                 num_disable--;
                 if (num_disable == 0) {
                     mh_mode |= CRYPTO_MEM_CHECK_ENABLE;
-                    CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC2);
+                    CRYPTO_THREAD_unlock(long_malloc_lock);
                 }
             }
         }
         break;
     }
-    CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC);
+    CRYPTO_THREAD_unlock(malloc_lock);
     return (ret);
 #endif
 }
@@ -279,14 +286,15 @@ static int mem_check_on(void)
     int ret = 0;
 
     if (mh_mode & CRYPTO_MEM_CHECK_ON) {
-        CRYPTO_THREADID cur;
-        CRYPTO_THREADID_current(&cur);
-        CRYPTO_r_lock(CRYPTO_LOCK_MALLOC);
+        CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+
+        CRYPTO_THREAD_ID cur = CRYPTO_THREAD_get_current_id();
+        CRYPTO_THREAD_read_lock(malloc_lock);
 
         ret = (mh_mode & CRYPTO_MEM_CHECK_ENABLE)
-            || CRYPTO_THREADID_cmp(&disabling_threadid, &cur);
+            || !CRYPTO_THREAD_compare_id(disabling_threadid, cur);
 
-        CRYPTO_r_unlock(CRYPTO_LOCK_MALLOC);
+        CRYPTO_THREAD_unlock(malloc_lock);
     }
     return (ret);
 }
@@ -316,44 +324,29 @@ static unsigned long mem_hash(const MEM *a)
     return (ret);
 }
 
-static int app_info_cmp(const APP_INFO *a, const APP_INFO *b)
-{
-    return CRYPTO_THREADID_cmp(&a->threadid, &b->threadid);
-}
-
-static unsigned long app_info_hash(const APP_INFO *a)
-{
-    unsigned long ret;
-
-    ret = CRYPTO_THREADID_hash(&a->threadid);
-    /* This is left in as a "who am I to question legacy?" measure */
-    ret = ret * 17851 + (ret >> 14) * 7 + (ret >> 4) * 251;
-    return (ret);
-}
-
 /* returns 1 if there was an info to pop, 0 if the stack was empty. */
 static int pop_info(void)
 {
-    APP_INFO tmp;
     APP_INFO *current = NULL;
 
-    if (amih != NULL) {
-        CRYPTO_THREADID_current(&tmp.threadid);
-        if ((current = lh_APP_INFO_delete(amih, &tmp)) != NULL) {
-            APP_INFO *next = current->next;
+    CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+    current = (APP_INFO *)CRYPTO_THREAD_get_local(&appinfokey);
+    if (current != NULL) {
+        APP_INFO *next = current->next;
 
-            if (next != NULL) {
-                next->references++;
-                (void)lh_APP_INFO_insert(amih, next);
-            }
-            if (--(current->references) <= 0) {
-                current->next = NULL;
-                if (next != NULL)
-                    next->references--;
-                OPENSSL_free(current);
-            }
-            return 1;
+        if (next != NULL) {
+            next->references++;
+            CRYPTO_THREAD_set_local(&appinfokey, next);
+        } else {
+            CRYPTO_THREAD_set_local(&appinfokey, NULL);
+        }
+        if (--(current->references) <= 0) {
+            current->next = NULL;
+            if (next != NULL)
+                next->references--;
+            OPENSSL_free(current);
         }
+        return 1;
     }
     return 0;
 }
@@ -366,23 +359,22 @@ int CRYPTO_mem_debug_push(const char *info, const char *file, int line)
     if (mem_check_on()) {
         CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE);
 
+        CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+
         if ((ami = OPENSSL_malloc(sizeof(*ami))) == NULL)
             goto err;
-        if (amih == NULL) {
-            if ((amih = lh_APP_INFO_new(app_info_hash, app_info_cmp)) == NULL) {
-                OPENSSL_free(ami);
-                goto err;
-            }
-        }
 
-        CRYPTO_THREADID_current(&ami->threadid);
+        ami->threadid = CRYPTO_THREAD_get_current_id();
         ami->file = file;
         ami->line = line;
         ami->info = info;
         ami->references = 1;
         ami->next = NULL;
 
-        if ((amim = lh_APP_INFO_insert(amih, ami)) != NULL)
+        amim = (APP_INFO *)CRYPTO_THREAD_get_local(&appinfokey);
+        CRYPTO_THREAD_set_local(&appinfokey, ami);
+
+        if (amim != NULL)
             ami->next = amim;
         ret = 1;
  err:
@@ -410,7 +402,7 @@ void CRYPTO_mem_debug_malloc(void *addr, size_t num, int before_p,
                              const char *file, int line)
 {
     MEM *m, *mm;
-    APP_INFO tmp, *amim;
+    APP_INFO *amim;
 
     switch (before_p & 127) {
     case 0:
@@ -421,6 +413,9 @@ void CRYPTO_mem_debug_malloc(void *addr, size_t num, int before_p,
 
         if (mem_check_on()) {
             CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE);
+
+            CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+
             if ((m = OPENSSL_malloc(sizeof(*m))) == NULL) {
                 OPENSSL_free(addr);
                 CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ENABLE);
@@ -439,7 +434,7 @@ void CRYPTO_mem_debug_malloc(void *addr, size_t num, int before_p,
             m->file = file;
             m->line = line;
             m->num = num;
-            CRYPTO_THREADID_current(&m->threadid);
+            m->threadid = CRYPTO_THREAD_get_current_id();
 
             if (order == break_order_num) {
                 /* BREAK HERE */
@@ -451,13 +446,10 @@ void CRYPTO_mem_debug_malloc(void *addr, size_t num, int before_p,
 # endif
             m->time = time(NULL);
 
-            CRYPTO_THREADID_current(&tmp.threadid);
-            m->app_info = NULL;
-            if (amih != NULL
-                && (amim = lh_APP_INFO_retrieve(amih, &tmp)) != NULL) {
-                m->app_info = amim;
+            amim = (APP_INFO *)CRYPTO_THREAD_get_local(&appinfokey);
+            m->app_info = amim;
+            if (amim != NULL)
                 amim->references++;
-            }
 
             if ((mm = lh_MEM_insert(mh, m)) != NULL) {
                 /* Not good, but don't sweat it */
@@ -554,7 +546,16 @@ static void print_leak(const MEM *m, MEM_LEAK *l)
     APP_INFO *amip;
     int ami_cnt;
     struct tm *lcl = NULL;
-    CRYPTO_THREADID ti;
+    /*
+     * Convert between CRYPTO_THREAD_ID (which could be anything at all) and
+     * a long. This may not be meaningful depending on what CRYPTO_THREAD_ID is
+     * but hopefully should give something sensible on most platforms
+     */
+    union {
+        CRYPTO_THREAD_ID tid;
+        unsigned long ltid;
+    } tid;
+    CRYPTO_THREAD_ID ti;
 
 #define BUF_REMAIN (sizeof buf - (size_t)(bufp - buf))
 
@@ -573,8 +574,9 @@ static void print_leak(const MEM *m, MEM_LEAK *l)
                  m->order, m->file, m->line);
     bufp += strlen(bufp);
 
-    BIO_snprintf(bufp, BUF_REMAIN, "thread=%lu, ",
-                 CRYPTO_THREADID_hash(&m->threadid));
+    tid.ltid = 0;
+    tid.tid = m->threadid;
+    BIO_snprintf(bufp, BUF_REMAIN, "thread=%lu, ", tid.ltid);
     bufp += strlen(bufp);
 
     BIO_snprintf(bufp, BUF_REMAIN, "number=%d, address=%p\n",
@@ -590,7 +592,7 @@ static void print_leak(const MEM *m, MEM_LEAK *l)
     ami_cnt = 0;
 
     if (amip) {
-        CRYPTO_THREADID_cpy(&ti, &amip->threadid);
+        ti = amip->threadid;
 
         do {
             int buf_len;
@@ -598,9 +600,11 @@ static void print_leak(const MEM *m, MEM_LEAK *l)
 
             ami_cnt++;
             memset(buf, '>', ami_cnt);
+            tid.ltid = 0;
+            tid.tid = amip->threadid;
             BIO_snprintf(buf + ami_cnt, sizeof buf - ami_cnt,
                          " thread=%lu, file=%s, line=%d, info=\"",
-                         CRYPTO_THREADID_hash(&amip->threadid), amip->file,
+                         tid.ltid, amip->file,
                          amip->line);
             buf_len = strlen(buf);
             info_len = strlen(amip->info);
@@ -617,7 +621,7 @@ static void print_leak(const MEM *m, MEM_LEAK *l)
 
             amip = amip->next;
         }
-        while (amip && !CRYPTO_THREADID_cmp(&amip->threadid, &ti));
+        while (amip && CRYPTO_THREAD_compare_id(amip->threadid, ti));
     }
 
 #ifndef OPENSSL_NO_CRYPTO_MDEBUG_BACKTRACE
@@ -638,12 +642,11 @@ int CRYPTO_mem_leaks(BIO *b)
 {
     MEM_LEAK ml;
 
-    if (mh == NULL && amih == NULL)
-        return 1;
-
     /* Ensure all resources are released */
     OPENSSL_cleanup();
 
+    CRYPTO_THREAD_run_once(&memdbg_init, do_memdbg_init);
+
     CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE);
 
     ml.bio = b;
@@ -668,7 +671,7 @@ int CRYPTO_mem_leaks(BIO *b)
          */
         int old_mh_mode;
 
-        CRYPTO_w_lock(CRYPTO_LOCK_MALLOC);
+        CRYPTO_THREAD_write_lock(malloc_lock);
 
         /*
          * avoid deadlock when lh_free() uses CRYPTO_mem_debug_free(), which uses
@@ -679,17 +682,19 @@ int CRYPTO_mem_leaks(BIO *b)
 
         lh_MEM_free(mh);
         mh = NULL;
-        if (amih != NULL) {
-            if (lh_APP_INFO_num_items(amih) == 0) {
-                lh_APP_INFO_free(amih);
-                amih = NULL;
-            }
-        }
 
         mh_mode = old_mh_mode;
-        CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC);
+        CRYPTO_THREAD_unlock(malloc_lock);
     }
-    CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ENABLE);
+    CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_OFF);
+
+    /* Clean up locks etc */
+    CRYPTO_THREAD_cleanup_local(&appinfokey);
+    CRYPTO_THREAD_lock_free(malloc_lock);
+    CRYPTO_THREAD_lock_free(long_malloc_lock);
+    malloc_lock = NULL;
+    long_malloc_lock = NULL;
+
     return ml.chunks == 0 ? 1 : 0;
 }
 
@@ -699,8 +704,6 @@ int CRYPTO_mem_leaks_fp(FILE *fp)
     BIO *b;
     int ret;
 
-    if (mh == NULL && amih == NULL)
-        return 1;
     /*
      * Need to turn off memory checking when allocated BIOs ... especially as
      * we're creating them at a time when we're trying to check we've not

crypto/mem_sec.c

@@ -25,10 +25,9 @@
 # include <sys/param.h>
 # include <sys/stat.h>
 # include <fcntl.h>
+# include "internal/threads.h"
 #endif
 
-#define LOCK()      CRYPTO_w_lock(CRYPTO_LOCK_MALLOC)
-#define UNLOCK()    CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC)
 #define CLEAR(p, s) OPENSSL_cleanse(p, s)
 #ifndef PAGE_SIZE
 # define PAGE_SIZE    4096
@@ -40,6 +39,8 @@ static size_t secure_mem_used;
 static int secure_mem_initialized;
 static int too_late;
 
+static CRYPTO_RWLOCK *sec_malloc_lock = NULL;
+
 /*
  * These are the functions that must be implemented by a secure heap (sh).
  */
@@ -58,13 +59,16 @@ int CRYPTO_secure_malloc_init(size_t size, int minsize)
 
     if (too_late)
         return ret;
-    LOCK();
+
     OPENSSL_assert(!secure_mem_initialized);
     if (!secure_mem_initialized) {
+        sec_malloc_lock = CRYPTO_THREAD_lock_new();
+        if (sec_malloc_lock == NULL)
+            return 0;
         ret = sh_init(size, minsize);
         secure_mem_initialized = 1;
     }
-    UNLOCK();
+
     return ret;
 #else
     return 0;
@@ -74,10 +78,9 @@ int CRYPTO_secure_malloc_init(size_t size, int minsize)
 void CRYPTO_secure_malloc_done()
 {
 #ifdef IMPLEMENTED
-    LOCK();
     sh_done();
     secure_mem_initialized = 0;
-    UNLOCK();
+    CRYPTO_THREAD_lock_free(sec_malloc_lock);
 #endif /* IMPLEMENTED */
 }
 
@@ -100,11 +103,11 @@ void *CRYPTO_secure_malloc(size_t num, const char *file, int line)
         too_late = 1;
         return CRYPTO_malloc(num, file, line);
     }
-    LOCK();
+    CRYPTO_THREAD_write_lock(sec_malloc_lock);
     ret = sh_malloc(num);
     actual_size = ret ? sh_actual_size(ret) : 0;
     secure_mem_used += actual_size;
-    UNLOCK();
+    CRYPTO_THREAD_unlock(sec_malloc_lock);
     return ret;
 #else
     return CRYPTO_malloc(num, file, line);
@@ -131,12 +134,12 @@ void CRYPTO_secure_free(void *ptr, const char *file, int line)
         CRYPTO_free(ptr, file, line);
         return;
     }
-    LOCK();
+    CRYPTO_THREAD_write_lock(sec_malloc_lock);
     actual_size = sh_actual_size(ptr);
     CLEAR(ptr, actual_size);
     secure_mem_used -= actual_size;
     sh_free(ptr);
-    UNLOCK();
+    CRYPTO_THREAD_unlock(sec_malloc_lock);
 #else
     CRYPTO_free(ptr, file, line);
 #endif /* IMPLEMENTED */
@@ -149,9 +152,9 @@ int CRYPTO_secure_allocated(const void *ptr)
 
     if (!secure_mem_initialized)
         return 0;
-    LOCK();
+    CRYPTO_THREAD_write_lock(sec_malloc_lock);
     ret = sh_allocated(ptr);
-    UNLOCK();
+    CRYPTO_THREAD_unlock(sec_malloc_lock);
     return ret;
 #else
     return 0;
@@ -172,9 +175,9 @@ size_t CRYPTO_secure_actual_size(void *ptr)
 #ifdef IMPLEMENTED
     size_t actual_size;
 
-    LOCK();
+    CRYPTO_THREAD_write_lock(sec_malloc_lock);
     actual_size = sh_actual_size(ptr);
-    UNLOCK();
+    CRYPTO_THREAD_unlock(sec_malloc_lock);
     return actual_size;
 #else
     return 0;

include/openssl/crypto.h

@@ -174,10 +174,8 @@ extern "C" {
 # define CRYPTO_LOCK_SSL_METHOD          17
 # define CRYPTO_LOCK_RAND                18
 # define CRYPTO_LOCK_RAND2               19
-# define CRYPTO_LOCK_MALLOC              20
 # define CRYPTO_LOCK_READDIR             24
 # define CRYPTO_LOCK_RSA_BLINDING        25
-# define CRYPTO_LOCK_MALLOC2             27
 # define CRYPTO_LOCK_DYNLOCK             29
 # define CRYPTO_LOCK_ENGINE              30
 # define CRYPTO_LOCK_ECDSA               32