From 9471f7760dcc81fe6995f016fc7120db0c678818 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 8 Mar 2016 15:44:05 +0000 Subject: [PATCH] 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: Rich Salz --- crypto/mem_dbg.c | 199 ++++++++++++++++++++------------------- crypto/mem_sec.c | 31 +++--- include/openssl/crypto.h | 2 - 3 files changed, 118 insertions(+), 114 deletions(-) diff --git a/crypto/mem_dbg.c b/crypto/mem_dbg.c index f2a1fd837a..36efed8fba 100644 --- a/crypto/mem_dbg.c +++ b/crypto/mem_dbg.c @@ -112,6 +112,7 @@ #include #include #include "internal/cryptlib.h" +#include "internal/threads.h" #include #include #include @@ -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 diff --git a/crypto/mem_sec.c b/crypto/mem_sec.c index 7e41765b30..31c052512d 100644 --- a/crypto/mem_sec.c +++ b/crypto/mem_sec.c @@ -25,10 +25,9 @@ # include # include # include +# 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; diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h index 2969e2e61d..cfda8bb214 100644 --- a/include/openssl/crypto.h +++ b/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 -- 2.34.1