From be606c013d31847718ceb5d97c567988a771c2e5 Mon Sep 17 00:00:00 2001 From: Rich Salz Date: Wed, 7 Jun 2017 11:23:37 -0400 Subject: [PATCH 1/1] Add a lock around the OBJ_NAME table Various initialization functions modify this table, which can cause heap corruption in the absence of external synchronization. Some stats are modified from OPENSSL_LH_retrieve, where callers aren't expecting to have to take out an exclusive lock. Switch to using atomic operations for those stats. Reviewed-by: Matt Caswell Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3525) --- crypto/lhash/lh_stats.c | 22 +++++++--- crypto/lhash/lhash.c | 29 +++++++------ crypto/lhash/lhash_lcl.h | 19 +++++--- crypto/objects/o_names.c | 93 ++++++++++++++++++++++++++++------------ 4 files changed, 112 insertions(+), 51 deletions(-) diff --git a/crypto/lhash/lh_stats.c b/crypto/lhash/lh_stats.c index 7337832422..5586afa0d8 100644 --- a/crypto/lhash/lh_stats.c +++ b/crypto/lhash/lh_stats.c @@ -61,6 +61,9 @@ void OPENSSL_LH_node_usage_stats(const OPENSSL_LHASH *lh, FILE *fp) void OPENSSL_LH_stats_bio(const OPENSSL_LHASH *lh, BIO *out) { + OPENSSL_LHASH *lh_mut = (OPENSSL_LHASH *) lh; + int ret; + BIO_printf(out, "num_items = %lu\n", lh->num_items); BIO_printf(out, "num_nodes = %u\n", lh->num_nodes); BIO_printf(out, "num_alloc_nodes = %u\n", lh->num_alloc_nodes); @@ -69,15 +72,24 @@ void OPENSSL_LH_stats_bio(const OPENSSL_LHASH *lh, BIO *out) BIO_printf(out, "num_contracts = %lu\n", lh->num_contracts); BIO_printf(out, "num_contract_reallocs = %lu\n", lh->num_contract_reallocs); - BIO_printf(out, "num_hash_calls = %lu\n", lh->num_hash_calls); - BIO_printf(out, "num_comp_calls = %lu\n", lh->num_comp_calls); + CRYPTO_atomic_add(&lh_mut->num_hash_calls, 0, &ret, + lh->retrieve_stats_lock); + BIO_printf(out, "num_hash_calls = %d\n", ret); + CRYPTO_atomic_add(&lh_mut->num_comp_calls, 0, &ret, + lh->retrieve_stats_lock); + BIO_printf(out, "num_comp_calls = %d\n", ret); BIO_printf(out, "num_insert = %lu\n", lh->num_insert); BIO_printf(out, "num_replace = %lu\n", lh->num_replace); BIO_printf(out, "num_delete = %lu\n", lh->num_delete); BIO_printf(out, "num_no_delete = %lu\n", lh->num_no_delete); - BIO_printf(out, "num_retrieve = %lu\n", lh->num_retrieve); - BIO_printf(out, "num_retrieve_miss = %lu\n", lh->num_retrieve_miss); - BIO_printf(out, "num_hash_comps = %lu\n", lh->num_hash_comps); + CRYPTO_atomic_add(&lh_mut->num_retrieve, 0, &ret, lh->retrieve_stats_lock); + BIO_printf(out, "num_retrieve = %d\n", ret); + CRYPTO_atomic_add(&lh_mut->num_retrieve_miss, 0, &ret, + lh->retrieve_stats_lock); + BIO_printf(out, "num_retrieve_miss = %d\n", ret); + CRYPTO_atomic_add(&lh_mut->num_hash_comps, 0, &ret, + lh->retrieve_stats_lock); + BIO_printf(out, "num_hash_comps = %d\n", ret); } void OPENSSL_LH_node_stats_bio(const OPENSSL_LHASH *lh, BIO *out) diff --git a/crypto/lhash/lhash.c b/crypto/lhash/lhash.c index 82de223599..0fbd3854f1 100644 --- a/crypto/lhash/lhash.c +++ b/crypto/lhash/lhash.c @@ -29,9 +29,11 @@ OPENSSL_LHASH *OPENSSL_LH_new(OPENSSL_LH_HASHFUNC h, OPENSSL_LH_COMPFUNC c) OPENSSL_LHASH *ret; if ((ret = OPENSSL_zalloc(sizeof(*ret))) == NULL) - goto err0; + return NULL; if ((ret->b = OPENSSL_zalloc(sizeof(*ret->b) * MIN_NODES)) == NULL) - goto err1; + goto err; + if ((ret->retrieve_stats_lock = CRYPTO_THREAD_lock_new()) == NULL) + goto err; ret->comp = ((c == NULL) ? (OPENSSL_LH_COMPFUNC)strcmp : c); ret->hash = ((h == NULL) ? (OPENSSL_LH_HASHFUNC)OPENSSL_LH_strhash : h); ret->num_nodes = MIN_NODES / 2; @@ -41,10 +43,10 @@ OPENSSL_LHASH *OPENSSL_LH_new(OPENSSL_LH_HASHFUNC h, OPENSSL_LH_COMPFUNC c) ret->down_load = DOWN_LOAD; return (ret); - err1: +err: + OPENSSL_free(ret->b); OPENSSL_free(ret); - err0: - return (NULL); + return NULL; } void OPENSSL_LH_free(OPENSSL_LHASH *lh) @@ -63,6 +65,7 @@ void OPENSSL_LH_free(OPENSSL_LHASH *lh) n = nn; } } + CRYPTO_THREAD_lock_free(lh->retrieve_stats_lock); OPENSSL_free(lh->b); OPENSSL_free(lh); } @@ -133,18 +136,19 @@ void *OPENSSL_LH_retrieve(OPENSSL_LHASH *lh, const void *data) unsigned long hash; OPENSSL_LH_NODE **rn; void *ret; + int scratch; lh->error = 0; rn = getrn(lh, data, &hash); if (*rn == NULL) { - lh->num_retrieve_miss++; - return (NULL); + CRYPTO_atomic_add(&lh->num_retrieve_miss, 1, &scratch, lh->retrieve_stats_lock); + return NULL; } else { ret = (*rn)->data; - lh->num_retrieve++; + CRYPTO_atomic_add(&lh->num_retrieve, 1, &scratch, lh->retrieve_stats_lock); } - return (ret); + return ret; } static void doall_util_fn(OPENSSL_LHASH *lh, int use_arg, @@ -270,9 +274,10 @@ static OPENSSL_LH_NODE **getrn(OPENSSL_LHASH *lh, OPENSSL_LH_NODE **ret, *n1; unsigned long hash, nn; OPENSSL_LH_COMPFUNC cf; + int scratch; hash = (*(lh->hash)) (data); - lh->num_hash_calls++; + CRYPTO_atomic_add(&lh->num_hash_calls, 1, &scratch, lh->retrieve_stats_lock); *rhash = hash; nn = hash % lh->pmax; @@ -282,12 +287,12 @@ static OPENSSL_LH_NODE **getrn(OPENSSL_LHASH *lh, cf = lh->comp; ret = &(lh->b[(int)nn]); for (n1 = *ret; n1 != NULL; n1 = n1->next) { - lh->num_hash_comps++; + CRYPTO_atomic_add(&lh->num_hash_comps, 1, &scratch, lh->retrieve_stats_lock); if (n1->hash != hash) { ret = &(n1->next); continue; } - lh->num_comp_calls++; + CRYPTO_atomic_add(&lh->num_comp_calls, 1, &scratch, lh->retrieve_stats_lock); if (cf(n1->data, data) == 0) break; ret = &(n1->next); diff --git a/crypto/lhash/lhash_lcl.h b/crypto/lhash/lhash_lcl.h index eb4a1a3f65..01d463fb36 100644 --- a/crypto/lhash/lhash_lcl.h +++ b/crypto/lhash/lhash_lcl.h @@ -6,7 +6,7 @@ * in the file LICENSE in the source distribution or at * https://www.openssl.org/source/license.html */ - +#include struct lhash_node_st { void *data; @@ -18,6 +18,13 @@ struct lhash_st { OPENSSL_LH_NODE **b; OPENSSL_LH_COMPFUNC comp; OPENSSL_LH_HASHFUNC hash; + /* + * some stats are updated on lookup, which callers aren't expecting to have + * to take an exclusive lock around. This lock protects them on platforms + * without atomics, and their types are int rather than unsigned long below + * so they can be adjusted with CRYPTO_atomic_add. + */ + CRYPTO_RWLOCK *retrieve_stats_lock; unsigned int num_nodes; unsigned int num_alloc_nodes; unsigned int p; @@ -29,14 +36,14 @@ struct lhash_st { unsigned long num_expand_reallocs; unsigned long num_contracts; unsigned long num_contract_reallocs; - unsigned long num_hash_calls; - unsigned long num_comp_calls; + int num_hash_calls; + int num_comp_calls; unsigned long num_insert; unsigned long num_replace; unsigned long num_delete; unsigned long num_no_delete; - unsigned long num_retrieve; - unsigned long num_retrieve_miss; - unsigned long num_hash_comps; + int num_retrieve; + int num_retrieve_miss; + int num_hash_comps; int error; }; diff --git a/crypto/objects/o_names.c b/crypto/objects/o_names.c index ed98df8c2f..15fe653d09 100644 --- a/crypto/objects/o_names.c +++ b/crypto/objects/o_names.c @@ -16,6 +16,7 @@ #include #include #include +#include #include "obj_lcl.h" /* @@ -44,6 +45,7 @@ static int obj_strcmp(const char *a, const char *b) */ static LHASH_OF(OBJ_NAME) *names_lh = NULL; static int names_type_num = OBJ_NAME_TYPE_NUM; +static CRYPTO_RWLOCK *lock = NULL; struct name_funcs_st { unsigned long (*hash_func) (const char *name); @@ -62,23 +64,33 @@ static STACK_OF(NAME_FUNCS) *name_funcs_stack; static unsigned long obj_name_hash(const OBJ_NAME *a); static int obj_name_cmp(const OBJ_NAME *a, const OBJ_NAME *b); -int OBJ_NAME_init(void) +static CRYPTO_ONCE init = CRYPTO_ONCE_STATIC_INIT; +DEFINE_RUN_ONCE_STATIC(o_names_init) { - if (names_lh != NULL) - return (1); CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE); names_lh = lh_OBJ_NAME_new(obj_name_hash, obj_name_cmp); + lock = CRYPTO_THREAD_lock_new(); CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ENABLE); - return (names_lh != NULL); + return names_lh != NULL && lock != NULL; +} + +int OBJ_NAME_init(void) +{ + return RUN_ONCE(&init, o_names_init); } int OBJ_NAME_new_index(unsigned long (*hash_func) (const char *), int (*cmp_func) (const char *, const char *), void (*free_func) (const char *, int, const char *)) { - int ret, i, push; + int ret = 0, i, push; NAME_FUNCS *name_funcs; + if (!OBJ_NAME_init()) + return 0; + + CRYPTO_THREAD_write_lock(lock); + if (name_funcs_stack == NULL) { CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE); name_funcs_stack = sk_NAME_FUNCS_new_null(); @@ -86,7 +98,7 @@ int OBJ_NAME_new_index(unsigned long (*hash_func) (const char *), } if (name_funcs_stack == NULL) { /* ERROR */ - return (0); + goto out; } ret = names_type_num; names_type_num++; @@ -96,7 +108,8 @@ int OBJ_NAME_new_index(unsigned long (*hash_func) (const char *), CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ENABLE); if (name_funcs == NULL) { OBJerr(OBJ_F_OBJ_NAME_NEW_INDEX, ERR_R_MALLOC_FAILURE); - return (0); + ret = 0; + goto out; } name_funcs->hash_func = OPENSSL_LH_strhash; name_funcs->cmp_func = obj_strcmp; @@ -108,7 +121,8 @@ int OBJ_NAME_new_index(unsigned long (*hash_func) (const char *), if (!push) { OBJerr(OBJ_F_OBJ_NAME_NEW_INDEX, ERR_R_MALLOC_FAILURE); OPENSSL_free(name_funcs); - return 0; + ret = 0; + goto out; } } name_funcs = sk_NAME_FUNCS_value(name_funcs_stack, ret); @@ -118,7 +132,10 @@ int OBJ_NAME_new_index(unsigned long (*hash_func) (const char *), name_funcs->cmp_func = cmp_func; if (free_func != NULL) name_funcs->free_func = free_func; - return (ret); + +out: + CRYPTO_THREAD_unlock(lock); + return ret; } static int obj_name_cmp(const OBJ_NAME *a, const OBJ_NAME *b) @@ -134,7 +151,7 @@ static int obj_name_cmp(const OBJ_NAME *a, const OBJ_NAME *b) } else ret = strcmp(a->name, b->name); } - return (ret); + return ret; } static unsigned long obj_name_hash(const OBJ_NAME *a) @@ -150,18 +167,20 @@ static unsigned long obj_name_hash(const OBJ_NAME *a) ret = OPENSSL_LH_strhash(a->name); } ret ^= a->type; - return (ret); + return ret; } const char *OBJ_NAME_get(const char *name, int type) { OBJ_NAME on, *ret; int num = 0, alias; + const char *value = NULL; if (name == NULL) - return (NULL); - if ((names_lh == NULL) && !OBJ_NAME_init()) - return (NULL); + return NULL; + if (!OBJ_NAME_init()) + return NULL; + CRYPTO_THREAD_read_lock(lock); alias = type & OBJ_NAME_ALIAS; type &= ~OBJ_NAME_ALIAS; @@ -172,24 +191,30 @@ const char *OBJ_NAME_get(const char *name, int type) for (;;) { ret = lh_OBJ_NAME_retrieve(names_lh, &on); if (ret == NULL) - return (NULL); + break; if ((ret->alias) && !alias) { if (++num > 10) - return (NULL); + break; on.name = ret->data; } else { - return (ret->data); + value = ret->data; + break; } } + + CRYPTO_THREAD_unlock(lock); + return value; } int OBJ_NAME_add(const char *name, int type, const char *data) { OBJ_NAME *onp, *ret; - int alias; + int alias, ok = 0; - if ((names_lh == NULL) && !OBJ_NAME_init()) - return (0); + if (!OBJ_NAME_init()) + return 0; + + CRYPTO_THREAD_write_lock(lock); alias = type & OBJ_NAME_ALIAS; type &= ~OBJ_NAME_ALIAS; @@ -197,7 +222,7 @@ int OBJ_NAME_add(const char *name, int type, const char *data) onp = OPENSSL_malloc(sizeof(*onp)); if (onp == NULL) { /* ERROR */ - return 0; + goto unlock; } onp->name = name; @@ -223,18 +248,26 @@ int OBJ_NAME_add(const char *name, int type, const char *data) if (lh_OBJ_NAME_error(names_lh)) { /* ERROR */ OPENSSL_free(onp); - return 0; + goto unlock; } } - return 1; + + ok = 1; + +unlock: + CRYPTO_THREAD_unlock(lock); + return ok; } int OBJ_NAME_remove(const char *name, int type) { OBJ_NAME on, *ret; + int ok = 0; - if (names_lh == NULL) - return (0); + if (!OBJ_NAME_init()) + return 0; + + CRYPTO_THREAD_write_lock(lock); type &= ~OBJ_NAME_ALIAS; on.name = name; @@ -253,9 +286,11 @@ int OBJ_NAME_remove(const char *name, int type) ret->data); } OPENSSL_free(ret); - return (1); - } else - return (0); + ok = 1; + } + + CRYPTO_THREAD_unlock(lock); + return ok; } typedef struct { @@ -363,8 +398,10 @@ void OBJ_NAME_cleanup(int type) if (type < 0) { lh_OBJ_NAME_free(names_lh); sk_NAME_FUNCS_pop_free(name_funcs_stack, name_funcs_free); + CRYPTO_THREAD_lock_free(lock); names_lh = NULL; name_funcs_stack = NULL; + lock = NULL; } else lh_OBJ_NAME_set_down_load(names_lh, down_load); } -- 2.34.1