Commit 1a564e530b for openssl.org
commit 1a564e530b7e92acf8dbc58c5b5ca7a1cc98d5ed
Author: Neil Horman <nhorman@openssl.org>
Date: Fri Jun 12 12:59:42 2026 -0400
make ossl_method_store use cmp_exch_ptr when cleaning archive
from the conversation here:
https://github.com/openssl/openssl/pull/31018#discussion_r3386832056
@mattcaswell noted that while cleaning QUERY items and moving them to
the archive list, we do an atomic load of a QUERY's next pointer to
another shared query's next pointer. While its not been observed, it
may be possible for the clean operation to move an element to the
archive while a concurrent thread is prepending to the list, the result
being that the active (cache_list) list has a head pointer whos next
pointer points into the archive list.
The result of this would be subsequent lookups fail to find anything not
archived in the cache, and need to go through the slow
ossl_method_construct path again to slowly rebuild the cache. Thats not
catastrophic, but its definately a bug that will result in additional
memory allocations, along with entries that never get used again, and
possible memory leaks.
Switch the load_ptr call to be an atomic cmp_exch_ptr call to ensure
that the node being visited isn't mutated concurrently by both a thread
doing a clean and a list insert. This ensures that only one thread wins
the update, while the other restarts their operation.
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:41 2026
(Merged from https://github.com/openssl/openssl/pull/31487)
diff --git a/crypto/property/property.c b/crypto/property/property.c
index 8b5ed044c8..ee85031e6c 100644
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -976,7 +976,19 @@ static int ossl_method_store_atomic_archive(STORED_ALGORITHMS *sa, QUERY *old)
static ossl_inline int ossl_method_store_put_in_archive(STORED_ALGORITHMS *sa, QUERY *old)
{
/*
- * point the item we're remoing's next pointer to the top of the archive list
+ * point the item we're removing's next pointer to the top of the archive list
+ * Note: We're writing to the old->next here which is shared, so that's suspicious, but
+ * because we've already removed old from the cache_list in ossl_method_store_clean_archive
+ * this is safe for the following reasons:
+ * 1) the clean path is done under a write lock, so sa->archive is guaranteed stable
+ * 2) any concurrent reader (ie ossl_method_store_cache_set|get, if visiting the old node
+ * while we're moving it, will either read the true next value (pointing to the next element
+ * in the cache_list), or the one we write here (the next list in the archive)
+ *
+ * Reading the true next value is fine, as that's the normal traversal anyway.
+ * Reading the next pointer as pointing into the archive list is not great, but in the worst
+ * case this results in a transient failed cache lookup, which just means a temporary slow path
+ * retrieval of an algorithm.
*/
if (!CRYPTO_atomic_load_ptr((void **)&sa->archive, (void **)&old->next, sa->alock))
return 0;
@@ -994,7 +1006,7 @@ static ossl_inline int ossl_method_store_put_in_archive(STORED_ALGORITHMS *sa, Q
*/
static void ossl_method_store_atomic_clean_archive(STORED_ALGORITHMS *sa)
{
- QUERY *idx, *idxn;
+ QUERY *idx, *idxn, *tmp;
int archived;
int i;
int lock_failed;
@@ -1066,10 +1078,34 @@ static void ossl_method_store_atomic_clean_archive(STORED_ALGORITHMS *sa)
if (archived == 1) {
/*
* Start by making idx skip idxn in the list
+ * First load the expected next value of idx->next
*/
- if (!CRYPTO_atomic_load_ptr((void **)&idxn->next, (void **)&idx->next, sa->alock))
+ if (!CRYPTO_atomic_load_ptr((void **)&idx->next, (void **)&tmp, sa->alock))
break;
+ /*
+ * Now compare the value of idx->next to what we just loaded to tmp above
+ * if they match, we can safely update idx->next to skip the idxn entry
+ * by pointing idx->next to idxn->next.
+ * If the comparison fails, then we need to start the list traversal over again.
+ * Note: This should never happen, as once an item is in the list, this is the
+ * only path in which an in-list item has its next pointer mutated, and this
+ * occurs under a write lock, but we should be safe here
+ */
+ if (!CRYPTO_atomic_cmp_exch_ptr((void **)&idx->next,
+ (void **)&tmp, (void *)idxn->next,
+ sa->alock, &lock_failed)) {
+ if (lock_failed)
+ break;
+ /*
+ * The list was mutated while we were trying to mutate it
+ * Normally we would just use the reloaded value of tmp here to re-attempt
+ * the removal, but since idx was changed underneath us, we don't know where
+ * we are in the list anymore. Its safer to just restart the whole traversal
+ */
+ goto restart_list;
+ }
+
if (!ossl_method_store_put_in_archive(sa, idxn))
break;
@@ -1113,7 +1149,7 @@ static QUERY *ossl_method_store_atomic_find_in_list(STORED_ALGORITHMS *sa, int n
if (!CRYPTO_atomic_load_int(&idx->archived, &archived, sa->alock))
goto out;
if (archived == 0 && idx->nid == nid && idx->prov == prov
- && !strcmp(idx->prop_query, prop_query)) {
+ && (strcmp(idx->prop_query, prop_query) == 0)) {
ret = idx;
break;
}