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 <matt@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3525)
(cherry picked from commit be606c013d31847718ceb5d97c567988a771c2e5)

diff --git a/crypto/lhash/lh_stats.c b/crypto/lhash/lh_stats.c
index 7337832422d6a2997b0d1ece3b84bbefdc92d256..5586afa0d8b346a79937042b59849883c34835b1 100644 (file)
--- 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 adde832cc45c637016064aaabb00c392e2b6abe8..f24d0c4c8a508ab68eb76166d238d718d06596fd 100644 (file)
--- 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 eb4a1a3f65fe10c7a1212595f04578c95fa48626..01d463fb3637ef427615b7c177ed2b667d07d65b 100644 (file)
--- 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 <openssl/crypto.h>
 
 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 ed98df8c2f2ba116da58cb3a3092f6ff66b355d8..15fe653d09313fbc3e64a0870384f6d4eb8d63eb 100644 (file)
--- a/crypto/objects/o_names.c
+++ b/crypto/objects/o_names.c
@@ -16,6 +16,7 @@
 #include <openssl/objects.h>
 #include <openssl/safestack.h>
 #include <openssl/e_os2.h>
+#include <internal/thread_once.h>
 #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);
 }