From 92a25e24e6ec9735dea9ec645502cb075a5f8d24 Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Thu, 24 Nov 2022 18:48:10 +0100 Subject: [PATCH] Fix occasional assertion failure when storing properties Fixes #18631 The store lock does not prevent concurrent access to the property cache, because there are multiple stores. We drop the newly created entry and use the exisiting one if there is one already. Reviewed-by: Matt Caswell Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/19762) --- crypto/property/defn_cache.c | 35 ++++++++++++++-------------- crypto/property/property.c | 2 +- crypto/property/property_local.h | 2 +- test/property_test.c | 39 +++++++++++++++++++++++++------- 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/crypto/property/defn_cache.c b/crypto/property/defn_cache.c index bb555fccc1b..eb68a55aa79 100644 --- a/crypto/property/defn_cache.c +++ b/crypto/property/defn_cache.c @@ -70,22 +70,24 @@ OSSL_PROPERTY_LIST *ossl_prop_defn_get(OSSL_LIB_CTX *ctx, const char *prop) property_defns = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_PROPERTY_DEFN_INDEX); - if (property_defns == NULL || !ossl_lib_ctx_read_lock(ctx)) + if (!ossl_assert(property_defns != NULL) || !ossl_lib_ctx_read_lock(ctx)) return NULL; elem.prop = prop; r = lh_PROPERTY_DEFN_ELEM_retrieve(property_defns, &elem); ossl_lib_ctx_unlock(ctx); - return r != NULL ? r->defn : NULL; + if (r == NULL || !ossl_assert(r->defn != NULL)) + return NULL; + return r->defn; } /* - * Cache the property list for a given property string. Callers of this function - * should call ossl_prop_defn_get first to ensure that there is no existing - * cache entry for this property string. + * Cache the property list for a given property string *pl. + * If an entry already exists in the cache *pl is freed and + * overwritten with the existing entry from the cache. */ int ossl_prop_defn_set(OSSL_LIB_CTX *ctx, const char *prop, - OSSL_PROPERTY_LIST *pl) + OSSL_PROPERTY_LIST **pl) { PROPERTY_DEFN_ELEM elem, *old, *p = NULL; size_t len; @@ -102,28 +104,27 @@ int ossl_prop_defn_set(OSSL_LIB_CTX *ctx, const char *prop, if (!ossl_lib_ctx_write_lock(ctx)) return 0; + elem.prop = prop; if (pl == NULL) { - elem.prop = prop; lh_PROPERTY_DEFN_ELEM_delete(property_defns, &elem); goto end; } + /* check if property definition is in the cache already */ + if ((p = lh_PROPERTY_DEFN_ELEM_retrieve(property_defns, &elem)) != NULL) { + ossl_property_free(*pl); + *pl = p->defn; + goto end; + } len = strlen(prop); p = OPENSSL_malloc(sizeof(*p) + len); if (p != NULL) { p->prop = p->body; - p->defn = pl; + p->defn = *pl; memcpy(p->body, prop, len + 1); old = lh_PROPERTY_DEFN_ELEM_insert(property_defns, p); - if (!ossl_assert(old == NULL)) { - /* - * This should not happen. Any caller of ossl_prop_defn_set should - * have called ossl_prop_defn_get first - so we should know that - * there is no existing entry. If we get here we have a bug. We - * deliberately leak the |old| reference in order to avoid a crash - * if there are any existing users of it. - */ + if (!ossl_assert(old == NULL)) + /* This should not happen. An existing entry is handled above. */ goto end; - } if (!lh_PROPERTY_DEFN_ELEM_error(property_defns)) goto end; } diff --git a/crypto/property/property.c b/crypto/property/property.c index d5e35889cc2..6f56d2366bd 100644 --- a/crypto/property/property.c +++ b/crypto/property/property.c @@ -327,7 +327,7 @@ int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov, impl->properties = ossl_parse_property(store->ctx, properties); if (impl->properties == NULL) goto err; - if (!ossl_prop_defn_set(store->ctx, properties, impl->properties)) { + if (!ossl_prop_defn_set(store->ctx, properties, &impl->properties)) { ossl_property_free(impl->properties); impl->properties = NULL; goto err; diff --git a/crypto/property/property_local.h b/crypto/property/property_local.h index 6b85ce1586e..797fb3bf5f2 100644 --- a/crypto/property/property_local.h +++ b/crypto/property/property_local.h @@ -52,4 +52,4 @@ int ossl_property_has_optional(const OSSL_PROPERTY_LIST *query); /* Property definition cache functions */ OSSL_PROPERTY_LIST *ossl_prop_defn_get(OSSL_LIB_CTX *ctx, const char *prop); int ossl_prop_defn_set(OSSL_LIB_CTX *ctx, const char *prop, - OSSL_PROPERTY_LIST *pl); + OSSL_PROPERTY_LIST **pl); diff --git a/test/property_test.c b/test/property_test.c index 14b891c3a0f..3b978638036 100644 --- a/test/property_test.c +++ b/test/property_test.c @@ -284,19 +284,42 @@ static int test_property_merge(int n) static int test_property_defn_cache(void) { OSSL_METHOD_STORE *store; - OSSL_PROPERTY_LIST *red, *blue; - int r = 0; + OSSL_PROPERTY_LIST *red = NULL, *blue = NULL, *blue2 = NULL; + int r; - if (TEST_ptr(store = ossl_method_store_new(NULL)) + r = TEST_ptr(store = ossl_method_store_new(NULL)) && add_property_names("red", "blue", NULL) && TEST_ptr(red = ossl_parse_property(NULL, "red")) && TEST_ptr(blue = ossl_parse_property(NULL, "blue")) && TEST_ptr_ne(red, blue) - && TEST_true(ossl_prop_defn_set(NULL, "red", red)) - && TEST_true(ossl_prop_defn_set(NULL, "blue", blue)) - && TEST_ptr_eq(ossl_prop_defn_get(NULL, "red"), red) - && TEST_ptr_eq(ossl_prop_defn_get(NULL, "blue"), blue)) - r = 1; + && TEST_true(ossl_prop_defn_set(NULL, "red", &red)); + + if (!r) { + ossl_property_free(red); + red = NULL; + ossl_property_free(blue); + blue = NULL; + } + + r = r && TEST_true(ossl_prop_defn_set(NULL, "blue", &blue)); + if (!r) { + ossl_property_free(blue); + blue = NULL; + } + + r = r && TEST_ptr_eq(ossl_prop_defn_get(NULL, "red"), red) + && TEST_ptr_eq(ossl_prop_defn_get(NULL, "blue"), blue) + && TEST_ptr(blue2 = ossl_parse_property(NULL, "blue")) + && TEST_ptr_ne(blue2, blue) + && TEST_true(ossl_prop_defn_set(NULL, "blue", &blue2)); + if (!r) { + ossl_property_free(blue2); + blue2 = NULL; + } + + r = r && TEST_ptr_eq(blue2, blue) + && TEST_ptr_eq(ossl_prop_defn_get(NULL, "blue"), blue); + ossl_method_store_free(store); return r; } -- 2.47.3