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 <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19762)

(cherry picked from commit 92a25e24e6ec9735dea9ec645502cb075a5f8d24)

4 files changed, 51 insertions, 27 deletions

diff --git a/crypto/property/defn_cache.c b/crypto/property/defn_cache.c
index bb555fc..eb68a55 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 7b0e3c7..eb1d202 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 6b85ce1..797fb3b 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 60737b8..20ebb05 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;
 }