From: Neil Horman <nhorman@openssl.org>
Date: Mon, 15 Jun 2026 14:04:16 +0000 (-0400)
Subject: restore "oldest wins" behavior in method store
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=43e1e98a26510e676ea27c0039c679a544c48d89;p=thirdparty%2Fopenssl.git

restore "oldest wins" behavior in method store

We expressly define EVP_*fetch apis as not guaranteeing which provider
and algorithm is sourced from.  However, its likely that some users have
some inadvertent reliance on getting the same provider for a non
provider specific (and non property specific) fetch.  While thats
generally bad practice (since we don't guarantee it), its particularly
hard to provide that behavior in the new cache infrastructure, so lets
save everyone some trouble by not changing that behavior needlessly.

Reviewed-by: Matt Caswell <matt@openssl.foundation>
Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
MergeDate: Wed Jun 17 14:38:43 2026
(Merged from https://github.com/openssl/openssl/pull/31487)
---

diff --git a/crypto/property/property.c b/crypto/property/property.c
index ee85031e6c6..59238bfcd2f 100644
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -1190,6 +1190,7 @@ static ossl_inline int ossl_method_store_cache_set_atomic(OSSL_METHOD_STORE *sto
 {
     QUERY *p = NULL;
     int res = 1;
+    int skip_providerless = 0;
 
     if (method == NULL) {
         p = ossl_method_store_atomic_find_in_list(sa, nid, prov, prop_query);
@@ -1202,8 +1203,15 @@ static ossl_inline int ossl_method_store_cache_set_atomic(OSSL_METHOD_STORE *sto
     if (p != NULL) {
         ossl_method_store_atomic_archive(sa, p);
         p = ossl_method_store_atomic_find_in_list(sa, nid, NULL, prop_query);
+        /*
+         * Note: We want to preserve previous behavior here.  Namely, if we load multiple
+         * providers we don't want to change the algorithm implementation we return if we've
+         * already potentially returned one from another provider.  So if an alg exists for
+         * a given nid/prop query with a NULL provider, don't replace the one we have.
+         * Instead, let the oldest one win
+         */
         if (p != NULL)
-            ossl_method_store_atomic_archive(sa, p);
+            skip_providerless = 1;
     }
     p = QUERY_new(strlen(prop_query));
     if (p != NULL) {
@@ -1224,30 +1232,31 @@ static ossl_inline int ossl_method_store_cache_set_atomic(OSSL_METHOD_STORE *sto
             goto err;
         }
 
-        /*
-         * We also want to add this method into the cache against a key computed _only_
-         * from nid and property query.  This lets us match in the event someone does a lookup
-         * against a NULL provider (i.e. the "any provided alg will do" match
-         */
-        p = QUERY_new(strlen(prop_query));
-        if (p == NULL)
-            goto err;
-        TSAN_BENIGN(p, "Unpublished value is safe on subsequent read");
-        p->saptr = sa;
-        p->nid = nid;
-        p->prov = NULL;
-        p->archived = 0;
-        strcpy(p->prop_query, prop_query);
-        p->method.method = method;
-        p->method.up_ref = method_up_ref;
-        p->method.free = method_destruct;
-        if (!ossl_method_up_ref(&p->method))
-            goto err;
-        if (!ossl_method_store_atomic_insert_to_list(sa, p)) {
-            ossl_method_free(&p->method);
-            goto err;
+        if (skip_providerless == 0) {
+            /*
+             * We also want to add this method into the cache against a key computed _only_
+             * from nid and property query.  This lets us match in the event someone does a lookup
+             * against a NULL provider (i.e. the "any provided alg will do" match
+             */
+            p = QUERY_new(strlen(prop_query));
+            if (p == NULL)
+                goto err;
+            TSAN_BENIGN(p, "Unpublished value is safe on subsequent read");
+            p->saptr = sa;
+            p->nid = nid;
+            p->prov = NULL;
+            p->archived = 0;
+            strcpy(p->prop_query, prop_query);
+            p->method.method = method;
+            p->method.up_ref = method_up_ref;
+            p->method.free = method_destruct;
+            if (!ossl_method_up_ref(&p->method))
+                goto err;
+            if (!ossl_method_store_atomic_insert_to_list(sa, p)) {
+                ossl_method_free(&p->method);
+                goto err;
+            }
         }
-
         goto end;
     }
 err:
diff --git a/test/property_test.c b/test/property_test.c
index 467647bf908..f827d3c4354 100644
--- a/test/property_test.c
+++ b/test/property_test.c
@@ -634,8 +634,8 @@ static int test_query_cache_stochastic(void)
             || result != v + i)
             errors++;
     }
-    /* There is a tiny probability that this will fail when it shouldn't */
-    res = !TEST_int_gt(errors, 0);
+
+    res = TEST_int_eq(errors, 0);
 
 err:
     ossl_method_store_free(store);
@@ -672,7 +672,7 @@ static int test_query_cache_set_duplicate(void)
      */
     ossl_method_store_cache_set(store, &prov, 1, "", &refs, counted_up_ref,
         counted_down_ref);
-    if (!TEST_int_eq(refs, 5)
+    if (!TEST_int_eq(refs, 4)
         || !TEST_true(ossl_method_store_cache_get(store, &prov, 1, "",
             &result))
         || !TEST_ptr_eq(result, &refs))