Commit be606c01 authored by Rich Salz's avatar Rich Salz
Browse files

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: default avatarMatt Caswell <matt@openssl.org>
Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3525)
parent db0f35dd

crypto/lhash/lh_stats.c

+17 −5
Original line number Diff line number Diff line
@@ -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)

crypto/lhash/lhash.c

+17 −12
Original line number Diff line number Diff line
@@ -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);

crypto/lhash/lhash_lcl.h

+13 −6
Original line number Diff line number Diff line
@@ -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;

};

crypto/objects/o_names.c

+65 −28
Original line number Diff line number Diff line
@@ -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);

}