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)

diff --git a/crypto/property/defn_cache.c b/crypto/property/defn_cache.c
index bb555fccc1b230fdb4e0a28cc24a5d9918dbdfc1..eb68a55aa79e17e2ef45a95cb446579464761cbb 100644 (file)
--- 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 d5e35889cc28f5f4e1e26924095363de8ba5f63a..6f56d2366bd617d0f234f48b7c8ce05b3c452648 100644 (file)
--- 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 6b85ce1586e80954934e59c1d5212cf6abb005c0..797fb3bf5f2bb8dcf305090fe118f0b18398183b 100644 (file)
--- 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 14b891c3a0fba6554cce79c8bcd5c1dc83fd4c2c..3b978638036ccfbe6c4eaf0d9dc09a62c81b3660 100644 (file)
--- 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;
 }