From: Tomas Mraz Date: Mon, 27 May 2024 14:50:05 +0000 (+0200) Subject: Use the new hashtable for core_namemap X-Git-Tag: openssl-3.4.0-alpha1~85 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4cad608509855a7c181a856d64084c97aee80589;p=thirdparty%2Fopenssl.git Use the new hashtable for core_namemap This replaces LHASH in core_namemap with the new hashtable and adds a reverse mapping in form of stack of stacks instead of iterating the existing hash table members. The new hashtable is used in lockless-read mode. Reviewed-by: Neil Horman Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/24504) --- diff --git a/crypto/core_namemap.c b/crypto/core_namemap.c index 1dcf390fc2e..0cb96b562e0 100644 --- a/crypto/core_namemap.c +++ b/crypto/core_namemap.c @@ -8,62 +8,55 @@ */ #include "internal/namemap.h" -#include -#include "crypto/lhash.h" /* ossl_lh_strcasehash */ #include "internal/tsan_assist.h" +#include "internal/hashtable.h" #include "internal/sizes.h" #include "crypto/context.h" -/*- - * The namenum entry - * ================= - */ -typedef struct { - char *name; - int number; -} NAMENUM_ENTRY; +#define NAMEMAP_HT_BUCKETS 4096 -DEFINE_LHASH_OF_EX(NAMENUM_ENTRY); +HT_START_KEY_DEFN(namenum_key) +HT_DEF_KEY_FIELD_CHAR_ARRAY(name, 64) +HT_END_KEY_DEFN(NAMENUM_KEY) /*- * The namemap itself * ================== */ +typedef char STRING; +typedef STACK_OF(STRING) NAMES; + +DEFINE_STACK_OF(STRING) +DEFINE_STACK_OF(NAMES) + struct ossl_namemap_st { /* Flags */ unsigned int stored:1; /* If 1, it's stored in a library context */ + HT *namenum_ht; /* Name->number mapping */ + CRYPTO_RWLOCK *lock; - LHASH_OF(NAMENUM_ENTRY) *namenum; /* Name->number mapping */ + STACK_OF(NAMES) *numnames; TSAN_QUALIFIER int max_number; /* Current max number */ }; -/* LHASH callbacks */ - -static unsigned long namenum_hash(const NAMENUM_ENTRY *n) +static void name_string_free(char *name) { - return ossl_lh_strcasehash(n->name); + OPENSSL_free(name); } -static int namenum_cmp(const NAMENUM_ENTRY *a, const NAMENUM_ENTRY *b) +static void names_free(NAMES *n) { - return OPENSSL_strcasecmp(a->name, b->name); -} - -static void namenum_free(NAMENUM_ENTRY *n) -{ - if (n != NULL) - OPENSSL_free(n->name); - OPENSSL_free(n); + sk_STRING_pop_free(n, name_string_free); } /* OSSL_LIB_CTX_METHOD functions for a namemap stored in a library context */ void *ossl_stored_namemap_new(OSSL_LIB_CTX *libctx) { - OSSL_NAMEMAP *namemap = ossl_namemap_new(); + OSSL_NAMEMAP *namemap = ossl_namemap_new(libctx); if (namemap != NULL) namemap->stored = 1; @@ -107,20 +100,6 @@ int ossl_namemap_empty(OSSL_NAMEMAP *namemap) #endif } -typedef struct doall_names_data_st { - int number; - const char **names; - int found; -} DOALL_NAMES_DATA; - -static void do_name(const NAMENUM_ENTRY *namenum, DOALL_NAMES_DATA *data) -{ - if (namenum->number == data->number) - data->names[data->found++] = namenum->name; -} - -IMPLEMENT_LHASH_DOALL_ARG_CONST(NAMENUM_ENTRY, DOALL_NAMES_DATA); - /* * Call the callback for all names in the namemap with the given number. * A return value 1 means that the callback was called for all names. A @@ -130,61 +109,41 @@ int ossl_namemap_doall_names(const OSSL_NAMEMAP *namemap, int number, void (*fn)(const char *name, void *data), void *data) { - DOALL_NAMES_DATA cbdata; - size_t num_names; int i; + NAMES *names; - cbdata.number = number; - cbdata.found = 0; - - if (namemap == NULL) + if (namemap == NULL || number <= 0) return 0; /* - * We collect all the names first under a read lock. Subsequently we call + * We duplicate the NAMES stack under a read lock. Subsequently we call * the user function, so that we're not holding the read lock when in user * code. This could lead to deadlocks. */ if (!CRYPTO_THREAD_read_lock(namemap->lock)) return 0; - num_names = lh_NAMENUM_ENTRY_num_items(namemap->namenum); - if (num_names == 0) { - CRYPTO_THREAD_unlock(namemap->lock); - return 0; - } - cbdata.names = OPENSSL_malloc(sizeof(*cbdata.names) * num_names); - if (cbdata.names == NULL) { - CRYPTO_THREAD_unlock(namemap->lock); - return 0; - } - lh_NAMENUM_ENTRY_doall_DOALL_NAMES_DATA(namemap->namenum, do_name, - &cbdata); - CRYPTO_THREAD_unlock(namemap->lock); + names = sk_NAMES_value(namemap->numnames, number - 1); + if (names != NULL) + names = sk_STRING_dup(names); - for (i = 0; i < cbdata.found; i++) - fn(cbdata.names[i], data); + CRYPTO_THREAD_unlock(namemap->lock); - OPENSSL_free(cbdata.names); - return 1; -} + if (names == NULL) + return 0; -/* This function is not thread safe, the namemap must be locked */ -static int namemap_name2num(const OSSL_NAMEMAP *namemap, - const char *name) -{ - NAMENUM_ENTRY *namenum_entry, namenum_tmpl; + for (i = 0; i < sk_STRING_num(names); i++) + fn(sk_STRING_value(names, i), data); - namenum_tmpl.name = (char *)name; - namenum_tmpl.number = 0; - namenum_entry = - lh_NAMENUM_ENTRY_retrieve(namemap->namenum, &namenum_tmpl); - return namenum_entry != NULL ? namenum_entry->number : 0; + sk_STRING_free(names); + return i > 0; } int ossl_namemap_name2num(const OSSL_NAMEMAP *namemap, const char *name) { - int number; + int number = 0; + HT_VALUE *val; + NAMENUM_KEY key; #ifndef FIPS_MODULE if (namemap == NULL) @@ -194,14 +153,19 @@ int ossl_namemap_name2num(const OSSL_NAMEMAP *namemap, const char *name) if (namemap == NULL) return 0; - if (!CRYPTO_THREAD_read_lock(namemap->lock)) - return 0; - number = namemap_name2num(namemap, name); - CRYPTO_THREAD_unlock(namemap->lock); + HT_INIT_KEY(&key); + HT_SET_KEY_STRING_CASE(&key, name, name); + + val = ossl_ht_get(namemap->namenum_ht, TO_HT_KEY(&key)); + + if (val != NULL) + /* We store a (small) int directly instead of a pointer to it. */ + number = (int)(intptr_t)val->value; return number; } +/* TODO: Optimize to avoid strndup() */ int ossl_namemap_name2num_n(const OSSL_NAMEMAP *namemap, const char *name, size_t name_len) { @@ -216,62 +180,97 @@ int ossl_namemap_name2num_n(const OSSL_NAMEMAP *namemap, return ret; } -struct num2name_data_st { - size_t idx; /* Countdown */ - const char *name; /* Result */ -}; - -static void do_num2name(const char *name, void *vdata) +const char *ossl_namemap_num2name(const OSSL_NAMEMAP *namemap, int number, + size_t idx) { - struct num2name_data_st *data = vdata; + NAMES *names; + const char *ret = NULL; + + if (namemap == NULL || number <= 0) + return NULL; - if (data->idx > 0) - data->idx--; - else if (data->name == NULL) - data->name = name; + if (!CRYPTO_THREAD_read_lock(namemap->lock)) + return NULL; + + names = sk_NAMES_value(namemap->numnames, number - 1); + if (names != NULL) + ret = sk_STRING_value(names, idx); + + CRYPTO_THREAD_unlock(namemap->lock); + + return ret; } -const char *ossl_namemap_num2name(const OSSL_NAMEMAP *namemap, int number, - size_t idx) +/* This function is not thread safe, the namemap must be locked */ +static int numname_insert(OSSL_NAMEMAP *namemap, int number, + const char *name) { - struct num2name_data_st data; + NAMES *names; + char *tmpname; + + if (number > 0) { + names = sk_NAMES_value(namemap->numnames, number - 1); + if (!ossl_assert(names != NULL)) { + /* cannot happen */ + return 0; + } + } else { + /* a completely new entry */ + names = sk_STRING_new_null(); + if (names == NULL) + return 0; + } - data.idx = idx; - data.name = NULL; - if (!ossl_namemap_doall_names(namemap, number, do_num2name, &data)) - return NULL; - return data.name; + if ((tmpname = OPENSSL_strdup(name)) == NULL) + goto err; + + if (!sk_STRING_push(names, tmpname)) + goto err; + + if (number <= 0) { + if (!sk_NAMES_push(namemap->numnames, names)) + goto err; + number = sk_NAMES_num(namemap->numnames); + } + return number; + + err: + if (number <= 0) + sk_STRING_free(names); + OPENSSL_free(tmpname); + return 0; } /* This function is not thread safe, the namemap must be locked */ static int namemap_add_name(OSSL_NAMEMAP *namemap, int number, const char *name) { - NAMENUM_ENTRY *namenum = NULL; - int tmp_number; + int ret; + HT_VALUE val = { 0 }; + NAMENUM_KEY key; /* If it already exists, we don't add it */ - if ((tmp_number = namemap_name2num(namemap, name)) != 0) - return tmp_number; + if ((ret = ossl_namemap_name2num(namemap, name)) != 0) + return ret; - if ((namenum = OPENSSL_zalloc(sizeof(*namenum))) == NULL) + if ((number = numname_insert(namemap, number, name)) == 0) return 0; - if ((namenum->name = OPENSSL_strdup(name)) == NULL) - goto err; - - /* The tsan_counter use here is safe since we're under lock */ - namenum->number = - number != 0 ? number : 1 + tsan_counter(&namemap->max_number); - (void)lh_NAMENUM_ENTRY_insert(namemap->namenum, namenum); - - if (lh_NAMENUM_ENTRY_error(namemap->namenum)) - goto err; - return namenum->number; + /* Using tsan_store alone here is safe since we're under lock */ + tsan_store(&namemap->max_number, number); - err: - namenum_free(namenum); - return 0; + HT_INIT_KEY(&key); + HT_SET_KEY_STRING_CASE(&key, name, name); + val.value = (void *)(intptr_t)number; + ret = ossl_ht_insert(namemap->namenum_ht, TO_HT_KEY(&key), &val, NULL); + if (!ossl_assert(ret != 0)) /* cannot happen as we are under write lock */ + return 0; + if (ret < 1) { + /* unable to insert due to too many collisions */ + ERR_raise(ERR_LIB_CRYPTO, CRYPTO_R_TOO_MANY_NAMES); + return 0; + } + return number; } int ossl_namemap_add_name(OSSL_NAMEMAP *namemap, int number, @@ -334,7 +333,7 @@ int ossl_namemap_add_names(OSSL_NAMEMAP *namemap, int number, goto end; } - this_number = namemap_name2num(namemap, p); + this_number = ossl_namemap_name2num(namemap, p); if (number == 0) { number = this_number; @@ -508,16 +507,28 @@ OSSL_NAMEMAP *ossl_namemap_stored(OSSL_LIB_CTX *libctx) return namemap; } -OSSL_NAMEMAP *ossl_namemap_new(void) +OSSL_NAMEMAP *ossl_namemap_new(OSSL_LIB_CTX *libctx) { OSSL_NAMEMAP *namemap; + HT_CONFIG htconf = { NULL, NULL, NULL, NAMEMAP_HT_BUCKETS, 1, 1 }; - if ((namemap = OPENSSL_zalloc(sizeof(*namemap))) != NULL - && (namemap->lock = CRYPTO_THREAD_lock_new()) != NULL - && (namemap->namenum = - lh_NAMENUM_ENTRY_new(namenum_hash, namenum_cmp)) != NULL) - return namemap; + htconf.ctx = libctx; + + if ((namemap = OPENSSL_zalloc(sizeof(*namemap))) == NULL) + goto err; + if ((namemap->lock = CRYPTO_THREAD_lock_new()) == NULL) + goto err; + + if ((namemap->namenum_ht = ossl_ht_new(&htconf)) == NULL) + goto err; + + if ((namemap->numnames = sk_NAMES_new_null()) == NULL) + goto err; + + return namemap; + + err: ossl_namemap_free(namemap); return NULL; } @@ -527,8 +538,9 @@ void ossl_namemap_free(OSSL_NAMEMAP *namemap) if (namemap == NULL || namemap->stored) return; - lh_NAMENUM_ENTRY_doall(namemap->namenum, namenum_free); - lh_NAMENUM_ENTRY_free(namemap->namenum); + sk_NAMES_pop_free(namemap->numnames, names_free); + + ossl_ht_free(namemap->namenum_ht); CRYPTO_THREAD_lock_free(namemap->lock); OPENSSL_free(namemap); diff --git a/crypto/cpt_err.c b/crypto/cpt_err.c index 02d631466cf..bbcad8e51d2 100644 --- a/crypto/cpt_err.c +++ b/crypto/cpt_err.c @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 1995-2022 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2024 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -65,6 +65,7 @@ static const ERR_STRING_DATA CRYPTO_str_reasons[] = { "secure malloc failure"}, {ERR_PACK(ERR_LIB_CRYPTO, 0, CRYPTO_R_STRING_TOO_LONG), "string too long"}, {ERR_PACK(ERR_LIB_CRYPTO, 0, CRYPTO_R_TOO_MANY_BYTES), "too many bytes"}, + {ERR_PACK(ERR_LIB_CRYPTO, 0, CRYPTO_R_TOO_MANY_NAMES), "too many names"}, {ERR_PACK(ERR_LIB_CRYPTO, 0, CRYPTO_R_TOO_MANY_RECORDS), "too many records"}, {ERR_PACK(ERR_LIB_CRYPTO, 0, CRYPTO_R_TOO_SMALL_BUFFER), diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index f65167fd75c..88830a3cd2b 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -500,6 +500,7 @@ CRYPTO_R_RANDOM_SECTION_ERROR:119:random section error CRYPTO_R_SECURE_MALLOC_FAILURE:111:secure malloc failure CRYPTO_R_STRING_TOO_LONG:112:string too long CRYPTO_R_TOO_MANY_BYTES:113:too many bytes +CRYPTO_R_TOO_MANY_NAMES:132:too many names CRYPTO_R_TOO_MANY_RECORDS:114:too many records CRYPTO_R_TOO_SMALL_BUFFER:116:too small buffer CRYPTO_R_UNKNOWN_NAME_IN_RANDOM_SECTION:120:unknown name in random section diff --git a/include/crypto/cryptoerr.h b/include/crypto/cryptoerr.h index 1b6192e3f0a..d7df9a2c02e 100644 --- a/include/crypto/cryptoerr.h +++ b/include/crypto/cryptoerr.h @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 2020-2022 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 2020-2024 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy diff --git a/include/internal/namemap.h b/include/internal/namemap.h index 6c42a9cd7c4..adfc76c3c3b 100644 --- a/include/internal/namemap.h +++ b/include/internal/namemap.h @@ -13,7 +13,7 @@ typedef struct ossl_namemap_st OSSL_NAMEMAP; OSSL_NAMEMAP *ossl_namemap_stored(OSSL_LIB_CTX *libctx); -OSSL_NAMEMAP *ossl_namemap_new(void); +OSSL_NAMEMAP *ossl_namemap_new(OSSL_LIB_CTX *libctx); void ossl_namemap_free(OSSL_NAMEMAP *namemap); int ossl_namemap_empty(OSSL_NAMEMAP *namemap); diff --git a/include/openssl/cryptoerr.h b/include/openssl/cryptoerr.h index e84b12df6d5..7fa79cf3854 100644 --- a/include/openssl/cryptoerr.h +++ b/include/openssl/cryptoerr.h @@ -1,6 +1,6 @@ /* * Generated by util/mkerr.pl DO NOT EDIT - * Copyright 1995-2022 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 1995-2024 The OpenSSL Project Authors. All Rights Reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use * this file except in compliance with the License. You can obtain a copy @@ -47,6 +47,7 @@ # define CRYPTO_R_SECURE_MALLOC_FAILURE 111 # define CRYPTO_R_STRING_TOO_LONG 112 # define CRYPTO_R_TOO_MANY_BYTES 113 +# define CRYPTO_R_TOO_MANY_NAMES 132 # define CRYPTO_R_TOO_MANY_RECORDS 114 # define CRYPTO_R_TOO_SMALL_BUFFER 116 # define CRYPTO_R_UNKNOWN_NAME_IN_RANDOM_SECTION 120 diff --git a/test/namemap_internal_test.c b/test/namemap_internal_test.c index b3f498004fb..8e4266fc719 100644 --- a/test/namemap_internal_test.c +++ b/test/namemap_internal_test.c @@ -22,7 +22,7 @@ static int test_namemap_empty(void) int ok; ok = TEST_int_eq(ossl_namemap_empty(NULL), 1) - && TEST_ptr(nm = ossl_namemap_new()) + && TEST_ptr(nm = ossl_namemap_new(NULL)) && TEST_int_eq(ossl_namemap_empty(nm), 1) && TEST_int_ne(ossl_namemap_add_name(nm, 0, NAME1), 0) && TEST_int_eq(ossl_namemap_empty(nm), 0); @@ -55,7 +55,7 @@ static int test_namemap(OSSL_NAMEMAP *nm) static int test_namemap_independent(void) { - OSSL_NAMEMAP *nm = ossl_namemap_new(); + OSSL_NAMEMAP *nm = ossl_namemap_new(NULL); int ok = TEST_ptr(nm) && test_namemap(nm); ossl_namemap_free(nm);