From: Neil Horman Date: Thu, 7 Aug 2025 13:50:58 +0000 (-0400) Subject: Fix failure checking on rcu_read_lock X-Git-Tag: openssl-3.6.0-alpha1~202 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=036a46d2a4b00a004d05dc6a6d19be7184f8ecf1;p=thirdparty%2Fopenssl.git Fix failure checking on rcu_read_lock during memfail testing: https://github.com/openssl/openssl/actions/runs/16794088536/job/47561223902 We get lots of test failures in ossl_rcu_read_lock. This occurs because we have a few cases in the read lock path that attempt mallocs, which, if they fail, trigger an assert or a silent failure, which isn't really appropriate. We should instead fail gracefully, by informing the caller that the lock failed, like we do for CRYPTO_THREAD_read_lock. Fortunately, these are all internal apis, so we can convert ossl_rcu_read_lock to return an int indicating success/failure, and fail gracefully during the test, rather than hitting an assert abort. Fixes openssl/project#1315 Reviewed-by: Paul Yang Reviewed-by: Dmitry Belyavskiy Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/28195) --- diff --git a/crypto/conf/conf_mod.c b/crypto/conf/conf_mod.c index ce8ff4da306..03a27da357a 100644 --- a/crypto/conf/conf_mod.c +++ b/crypto/conf/conf_mod.c @@ -411,7 +411,9 @@ static CONF_MODULE *module_find(const char *name) if (!RUN_ONCE(&init_module_list_lock, do_init_module_list_lock)) return NULL; - ossl_rcu_read_lock(module_list_lock); + if (!ossl_rcu_read_lock(module_list_lock)) + return NULL; + mods = ossl_rcu_deref(&supported_modules); for (i = 0; i < sk_CONF_MODULE_num(mods); i++) { diff --git a/crypto/hashtable/hashtable.c b/crypto/hashtable/hashtable.c index 03d8a397380..bc019391572 100644 --- a/crypto/hashtable/hashtable.c +++ b/crypto/hashtable/hashtable.c @@ -233,9 +233,9 @@ err: return NULL; } -void ossl_ht_read_lock(HT *htable) +int ossl_ht_read_lock(HT *htable) { - ossl_rcu_read_lock(htable->lock); + return ossl_rcu_read_lock(htable->lock); } void ossl_ht_read_unlock(HT *htable) diff --git a/crypto/threads_none.c b/crypto/threads_none.c index ef1a757bce6..3b68cd8ff1e 100644 --- a/crypto/threads_none.c +++ b/crypto/threads_none.c @@ -37,9 +37,9 @@ void ossl_rcu_lock_free(CRYPTO_RCU_LOCK *lock) OPENSSL_free(lock); } -void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) +int ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) { - return; + return 1; } void ossl_rcu_write_lock(CRYPTO_RCU_LOCK *lock) diff --git a/crypto/threads_pthread.c b/crypto/threads_pthread.c index 21d63ce2a7e..5d5d64baf2a 100644 --- a/crypto/threads_pthread.c +++ b/crypto/threads_pthread.c @@ -328,7 +328,7 @@ static void ossl_rcu_free_local_data(void *arg) OPENSSL_free(data); } -void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) +int ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) { struct rcu_thr_data *data; int i, available_qp = -1; @@ -341,9 +341,18 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) if (data == NULL) { data = OPENSSL_zalloc(sizeof(*data)); - OPENSSL_assert(data != NULL); - CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, data); - ossl_init_thread_start(NULL, lock->ctx, ossl_rcu_free_local_data); + if (data == NULL) + return 0; + + if (!CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, data)) { + OPENSSL_free(data); + return 0; + } + if (!ossl_init_thread_start(NULL, lock->ctx, ossl_rcu_free_local_data)) { + OPENSSL_free(data); + CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, NULL); + return 0; + } } for (i = 0; i < MAX_QPS; i++) { @@ -352,7 +361,7 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) /* If we have a hold on this lock already, we're good */ if (data->thread_qps[i].lock == lock) { data->thread_qps[i].depth++; - return; + return 1; } } @@ -364,6 +373,7 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) data->thread_qps[available_qp].qp = get_hold_current_qp(lock); data->thread_qps[available_qp].depth = 1; data->thread_qps[available_qp].lock = lock; + return 1; } void ossl_rcu_read_unlock(CRYPTO_RCU_LOCK *lock) diff --git a/crypto/threads_win.c b/crypto/threads_win.c index 597e2d8e010..11029ea0c0b 100644 --- a/crypto/threads_win.c +++ b/crypto/threads_win.c @@ -228,7 +228,7 @@ static void ossl_rcu_free_local_data(void *arg) OPENSSL_free(data); } -void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) +int ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) { struct rcu_thr_data *data; int i; @@ -242,9 +242,17 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) if (data == NULL) { data = OPENSSL_zalloc(sizeof(*data)); - OPENSSL_assert(data != NULL); - CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, data); - ossl_init_thread_start(NULL, lock->ctx, ossl_rcu_free_local_data); + if (data == NULL) + return 0; + if (!CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, data)) { + OPENSSL_free(data); + return 0; + } + if (!ossl_init_thread_start(NULL, lock->ctx, ossl_rcu_free_local_data)) { + OPENSSL_free(data); + CRYPTO_THREAD_set_local_ex(CRYPTO_THREAD_LOCAL_RCU_KEY, lock->ctx, NULL); + return 0; + } } for (i = 0; i < MAX_QPS; i++) { @@ -252,7 +260,7 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) available_qp = i; /* If we have a hold on this lock already, we're good */ if (data->thread_qps[i].lock == lock) - return; + return 1; } /* @@ -263,6 +271,7 @@ void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock) data->thread_qps[available_qp].qp = get_hold_current_qp(lock); data->thread_qps[available_qp].depth = 1; data->thread_qps[available_qp].lock = lock; + return 1; } void ossl_rcu_write_lock(CRYPTO_RCU_LOCK *lock) diff --git a/fuzz/hashtable.c b/fuzz/hashtable.c index 6fc033b3e6c..3f030ab218f 100644 --- a/fuzz/hashtable.c +++ b/fuzz/hashtable.c @@ -276,7 +276,8 @@ int FuzzerTestOneInput(const uint8_t *buf, size_t len) HT_SET_KEY_FIELD(&key, fuzzkey, keyval); /* lock the table for reading */ - ossl_ht_read_lock(fuzzer_table); + if (!ossl_ht_read_lock(fuzzer_table)) + return 0; /* * If the value to find is not already allocated diff --git a/include/internal/hashtable.h b/include/internal/hashtable.h index 9c0214eeb0c..fccb1c1915f 100644 --- a/include/internal/hashtable.h +++ b/include/internal/hashtable.h @@ -280,7 +280,7 @@ void ossl_ht_free(HT *htable); /* * Lock the table for reading */ -void ossl_ht_read_lock(HT *htable); +int ossl_ht_read_lock(HT *htable); /* * Lock the table for writing diff --git a/include/internal/rcu.h b/include/internal/rcu.h index 90160e8da71..14f16736d9e 100644 --- a/include/internal/rcu.h +++ b/include/internal/rcu.h @@ -19,7 +19,7 @@ typedef struct rcu_lock_st CRYPTO_RCU_LOCK; CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx); void ossl_rcu_lock_free(CRYPTO_RCU_LOCK *lock); -void ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock); +int ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock); void ossl_rcu_write_lock(CRYPTO_RCU_LOCK *lock); void ossl_rcu_write_unlock(CRYPTO_RCU_LOCK *lock); void ossl_rcu_read_unlock(CRYPTO_RCU_LOCK *lock); diff --git a/test/lhash_test.c b/test/lhash_test.c index 88282113325..ea7f4942991 100644 --- a/test/lhash_test.c +++ b/test/lhash_test.c @@ -573,7 +573,8 @@ static void do_mt_hash_work(void) } switch(behavior) { case DO_LOOKUP: - ossl_ht_read_lock(m_ht); + if (!ossl_ht_read_lock(m_ht)) + break; m = ossl_ht_mt_TEST_MT_ENTRY_get(m_ht, TO_HT_KEY(&key), &v); if (m != NULL && m != expected_m) { worker_exits[num] = "Read unexpected value from hashtable"; diff --git a/test/threadstest.c b/test/threadstest.c index 08081c60730..871698d2628 100644 --- a/test/threadstest.c +++ b/test/threadstest.c @@ -374,7 +374,13 @@ static void reader_fn(int *iterations) CRYPTO_atomic_add(&writer1_done, 0, &lw1, atomiclock); CRYPTO_atomic_add(&writer2_done, 0, &lw2, atomiclock); count++; - ossl_rcu_read_lock(rcu_lock); + if (!ossl_rcu_read_lock(rcu_lock)) { + TEST_info("rcu torture read lock failed"); + rcu_torture_result = 0; + *iterations = count; + return; + } + valp = ossl_rcu_deref(&writer_ptr); val = (valp == NULL) ? 0 : *valp;