The branch openssl-3.0 has been updated via e1436d54b9de5012d1716212c7329e46cf21a24a (commit) via e4a32f209ce6dcb380a7dc8c10a42946345ff38f (commit) from 824b0d56e757f4a5c0f8af48add768db33d8ce51 (commit)
- Log ----------------------------------------------------------------- commit e1436d54b9de5012d1716212c7329e46cf21a24a Author: Pauli <pa...@openssl.org> Date: Tue Dec 21 11:44:49 2021 +1100 test: add some unit tests for the property to string functions That is: ossl_property_name_str and ossl_property_value_str. These only have high level tests during the creation of child library contexts. Reviewed-by: Tomas Mraz <to...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/17325) (cherry picked from commit 9f6841e9d8964943cf5f616543750cee85c4911c) commit e4a32f209ce6dcb380a7dc8c10a42946345ff38f Author: Pauli <pa...@openssl.org> Date: Tue Dec 21 11:44:31 2021 +1100 property: use a stack to efficiently convert index to string The existing code does this conversion by searching the hash table for the appropriate index which is slow and expensive. Fixes #15867 Reviewed-by: Tomas Mraz <to...@openssl.org> (Merged from https://github.com/openssl/openssl/pull/17325) (cherry picked from commit 2e3c59356f847a76a90f9f837d4983428df6eb19) ----------------------------------------------------------------------- Summary of changes: crypto/property/property_string.c | 114 +++++++++++++++++--------------------- test/property_test.c | 61 ++++++++++++++------ 2 files changed, 97 insertions(+), 78 deletions(-) diff --git a/crypto/property/property_string.c b/crypto/property/property_string.c index 38deab5af0..6c61bfbbb2 100644 --- a/crypto/property/property_string.c +++ b/crypto/property/property_string.c @@ -40,6 +40,8 @@ typedef struct { PROP_TABLE *prop_values; OSSL_PROPERTY_IDX prop_name_idx; OSSL_PROPERTY_IDX prop_value_idx; + STACK_OF(OPENSSL_CSTRING) *prop_namelist; + STACK_OF(OPENSSL_CSTRING) *prop_valuelist; } PROPERTY_STRING_DATA; static unsigned long property_hash(const PROPERTY_STRING *a) @@ -78,6 +80,9 @@ static void property_string_data_free(void *vpropdata) CRYPTO_THREAD_lock_free(propdata->lock); property_table_free(&propdata->prop_names); property_table_free(&propdata->prop_values); + sk_OPENSSL_CSTRING_free(propdata->prop_namelist); + sk_OPENSSL_CSTRING_free(propdata->prop_valuelist); + propdata->prop_namelist = propdata->prop_valuelist = NULL; propdata->prop_name_idx = propdata->prop_value_idx = 0; OPENSSL_free(propdata); @@ -90,24 +95,21 @@ static void *property_string_data_new(OSSL_LIB_CTX *ctx) { return NULL; propdata->lock = CRYPTO_THREAD_lock_new(); - if (propdata->lock == NULL) - goto err; - propdata->prop_names = lh_PROPERTY_STRING_new(&property_hash, &property_cmp); - if (propdata->prop_names == NULL) - goto err; - propdata->prop_values = lh_PROPERTY_STRING_new(&property_hash, &property_cmp); - if (propdata->prop_values == NULL) - goto err; - + propdata->prop_namelist = sk_OPENSSL_CSTRING_new_null(); + propdata->prop_valuelist = sk_OPENSSL_CSTRING_new_null(); + if (propdata->lock == NULL + || propdata->prop_names == NULL + || propdata->prop_values == NULL + || propdata->prop_namelist == NULL + || propdata->prop_valuelist == NULL) { + property_string_data_free(propdata); + return NULL; + } return propdata; - -err: - property_string_data_free(propdata); - return NULL; } static const OSSL_LIB_CTX_METHOD property_string_data_method = { @@ -134,57 +136,65 @@ static PROPERTY_STRING *new_property_string(const char *s, return ps; } -static OSSL_PROPERTY_IDX ossl_property_string(CRYPTO_RWLOCK *lock, - PROP_TABLE *t, - OSSL_PROPERTY_IDX *pidx, - const char *s) +static OSSL_PROPERTY_IDX ossl_property_string(OSSL_LIB_CTX *ctx, int name, + int create, const char *s) { PROPERTY_STRING p, *ps, *ps_new; + PROP_TABLE *t; + STACK_OF(OPENSSL_CSTRING) *slist; + OSSL_PROPERTY_IDX *pidx; + PROPERTY_STRING_DATA *propdata + = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_PROPERTY_STRING_INDEX, + &property_string_data_method); + if (propdata == NULL) + return 0; + + t = name ? propdata->prop_names : propdata->prop_values; p.s = s; - if (!CRYPTO_THREAD_read_lock(lock)) { + if (!CRYPTO_THREAD_read_lock(propdata->lock)) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_UNABLE_TO_GET_READ_LOCK); return 0; } ps = lh_PROPERTY_STRING_retrieve(t, &p); - if (ps == NULL && pidx != NULL) { - CRYPTO_THREAD_unlock(lock); - if (!CRYPTO_THREAD_write_lock(lock)) { + if (ps == NULL && create) { + CRYPTO_THREAD_unlock(propdata->lock); + if (!CRYPTO_THREAD_write_lock(propdata->lock)) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_UNABLE_TO_GET_WRITE_LOCK); return 0; } + pidx = name ? &propdata->prop_name_idx : &propdata->prop_value_idx; ps = lh_PROPERTY_STRING_retrieve(t, &p); if (ps == NULL && (ps_new = new_property_string(s, pidx)) != NULL) { + slist = name ? propdata->prop_namelist : propdata->prop_valuelist; + if (sk_OPENSSL_CSTRING_push(slist, ps_new->s) <= 0) { + property_free(ps_new); + CRYPTO_THREAD_unlock(propdata->lock); + return 0; + } lh_PROPERTY_STRING_insert(t, ps_new); if (lh_PROPERTY_STRING_error(t)) { + /*- + * Undo the previous push which means also decrementing the + * index and freeing the allocated storage. + */ + sk_OPENSSL_CSTRING_pop(slist); property_free(ps_new); - CRYPTO_THREAD_unlock(lock); + --*pidx; + CRYPTO_THREAD_unlock(propdata->lock); return 0; } ps = ps_new; } } - CRYPTO_THREAD_unlock(lock); + CRYPTO_THREAD_unlock(propdata->lock); return ps != NULL ? ps->idx : 0; } -struct find_str_st { - const char *str; - OSSL_PROPERTY_IDX idx; -}; - -static void find_str_fn(PROPERTY_STRING *prop, void *vfindstr) -{ - struct find_str_st *findstr = vfindstr; - - if (prop->idx == findstr->idx) - findstr->str = prop->s; -} - static const char *ossl_property_str(int name, OSSL_LIB_CTX *ctx, OSSL_PROPERTY_IDX idx) { - struct find_str_st findstr; + const char *r; PROPERTY_STRING_DATA *propdata = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_PROPERTY_STRING_INDEX, &property_string_data_method); @@ -192,33 +202,21 @@ static const char *ossl_property_str(int name, OSSL_LIB_CTX *ctx, if (propdata == NULL) return NULL; - findstr.str = NULL; - findstr.idx = idx; - if (!CRYPTO_THREAD_read_lock(propdata->lock)) { ERR_raise(ERR_LIB_CRYPTO, ERR_R_UNABLE_TO_GET_READ_LOCK); return NULL; } - lh_PROPERTY_STRING_doall_arg(name ? propdata->prop_names - : propdata->prop_values, - find_str_fn, &findstr); + r = sk_OPENSSL_CSTRING_value(name ? propdata->prop_namelist + : propdata->prop_valuelist, idx - 1); CRYPTO_THREAD_unlock(propdata->lock); - return findstr.str; + return r; } OSSL_PROPERTY_IDX ossl_property_name(OSSL_LIB_CTX *ctx, const char *s, int create) { - PROPERTY_STRING_DATA *propdata - = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_PROPERTY_STRING_INDEX, - &property_string_data_method); - - if (propdata == NULL) - return 0; - return ossl_property_string(propdata->lock, propdata->prop_names, - create ? &propdata->prop_name_idx : NULL, - s); + return ossl_property_string(ctx, 1, create, s); } const char *ossl_property_name_str(OSSL_LIB_CTX *ctx, OSSL_PROPERTY_IDX idx) @@ -229,15 +227,7 @@ const char *ossl_property_name_str(OSSL_LIB_CTX *ctx, OSSL_PROPERTY_IDX idx) OSSL_PROPERTY_IDX ossl_property_value(OSSL_LIB_CTX *ctx, const char *s, int create) { - PROPERTY_STRING_DATA *propdata - = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_PROPERTY_STRING_INDEX, - &property_string_data_method); - - if (propdata == NULL) - return 0; - return ossl_property_string(propdata->lock, propdata->prop_values, - create ? &propdata->prop_value_idx : NULL, - s); + return ossl_property_string(ctx, 0, create, s); } const char *ossl_property_value_str(OSSL_LIB_CTX *ctx, OSSL_PROPERTY_IDX idx) diff --git a/test/property_test.c b/test/property_test.c index ad44cf1513..14b891c3a0 100644 --- a/test/property_test.c +++ b/test/property_test.c @@ -50,30 +50,59 @@ static void down_ref(void *p) static int test_property_string(void) { - OSSL_METHOD_STORE *store; + OSSL_LIB_CTX *ctx; + OSSL_METHOD_STORE *store = NULL; int res = 0; OSSL_PROPERTY_IDX i, j; - if (TEST_ptr(store = ossl_method_store_new(NULL)) - && TEST_int_eq(ossl_property_name(NULL, "fnord", 0), 0) - && TEST_int_ne(ossl_property_name(NULL, "fnord", 1), 0) - && TEST_int_ne(ossl_property_name(NULL, "name", 1), 0) + /*- + * Use our own library context because we depend on ordering from a + * pristine state. + */ + if (TEST_ptr(ctx = OSSL_LIB_CTX_new()) + && TEST_ptr(store = ossl_method_store_new(ctx)) + && TEST_int_eq(ossl_property_name(ctx, "fnord", 0), 0) + && TEST_int_ne(ossl_property_name(ctx, "fnord", 1), 0) + && TEST_int_ne(ossl_property_name(ctx, "name", 1), 0) + /* Pre loaded names */ + && TEST_str_eq(ossl_property_name_str(ctx, 1), "provider") + && TEST_str_eq(ossl_property_name_str(ctx, 2), "version") + && TEST_str_eq(ossl_property_name_str(ctx, 3), "fips") + && TEST_str_eq(ossl_property_name_str(ctx, 4), "output") + && TEST_str_eq(ossl_property_name_str(ctx, 5), "input") + && TEST_str_eq(ossl_property_name_str(ctx, 6), "structure") + /* The names we added */ + && TEST_str_eq(ossl_property_name_str(ctx, 7), "fnord") + && TEST_str_eq(ossl_property_name_str(ctx, 8), "name") + /* Out of range */ + && TEST_ptr_null(ossl_property_name_str(ctx, 0)) + && TEST_ptr_null(ossl_property_name_str(ctx, 9)) /* Property value checks */ - && TEST_int_eq(ossl_property_value(NULL, "fnord", 0), 0) - && TEST_int_ne(i = ossl_property_value(NULL, "no", 0), 0) - && TEST_int_ne(j = ossl_property_value(NULL, "yes", 0), 0) + && TEST_int_eq(ossl_property_value(ctx, "fnord", 0), 0) + && TEST_int_ne(i = ossl_property_value(ctx, "no", 0), 0) + && TEST_int_ne(j = ossl_property_value(ctx, "yes", 0), 0) && TEST_int_ne(i, j) - && TEST_int_eq(ossl_property_value(NULL, "yes", 1), j) - && TEST_int_eq(ossl_property_value(NULL, "no", 1), i) - && TEST_int_ne(i = ossl_property_value(NULL, "illuminati", 1), 0) - && TEST_int_eq(j = ossl_property_value(NULL, "fnord", 1), i + 1) - && TEST_int_eq(ossl_property_value(NULL, "fnord", 1), j) + && TEST_int_eq(ossl_property_value(ctx, "yes", 1), j) + && TEST_int_eq(ossl_property_value(ctx, "no", 1), i) + && TEST_int_ne(i = ossl_property_value(ctx, "illuminati", 1), 0) + && TEST_int_eq(j = ossl_property_value(ctx, "fnord", 1), i + 1) + && TEST_int_eq(ossl_property_value(ctx, "fnord", 1), j) + /* Pre loaded values */ + && TEST_str_eq(ossl_property_value_str(ctx, 1), "yes") + && TEST_str_eq(ossl_property_value_str(ctx, 2), "no") + /* The value we added */ + && TEST_str_eq(ossl_property_value_str(ctx, 3), "illuminati") + && TEST_str_eq(ossl_property_value_str(ctx, 4), "fnord") + /* Out of range */ + && TEST_ptr_null(ossl_property_value_str(ctx, 0)) + && TEST_ptr_null(ossl_property_value_str(ctx, 5)) /* Check name and values are distinct */ - && TEST_int_eq(ossl_property_value(NULL, "cold", 0), 0) - && TEST_int_ne(ossl_property_name(NULL, "fnord", 0), - ossl_property_value(NULL, "fnord", 0))) + && TEST_int_eq(ossl_property_value(ctx, "cold", 0), 0) + && TEST_int_ne(ossl_property_name(ctx, "fnord", 0), + ossl_property_value(ctx, "fnord", 0))) res = 1; ossl_method_store_free(store); + OSSL_LIB_CTX_free(ctx); return res; }