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 <paulyang.inf@gmail.com>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/28195)

diff --git a/crypto/conf/conf_mod.c b/crypto/conf/conf_mod.c
index ce8ff4da3062d6082db59f27b290f65a75b527a3..03a27da357a23642d76d8b9984b36a687be8d869 100644 (file)
--- 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 03d8a3973804281d9508da5bc22dd9533aeaa3e3..bc0193915721f45039d207e3f9a14e1f6f9a7826 100644 (file)
--- 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 ef1a757bce6e6e65b2b2cd6e430dc6b9d0374f78..3b68cd8ff1edb21222ac4e34f5589491404c934b 100644 (file)
--- 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 21d63ce2a7e53474191d9423d7f2c7dd2efb6769..5d5d64baf2af7f1a4e5cf3e9afe0ceb498705720 100644 (file)
--- 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 597e2d8e010935504ec8725522262b93377f79ab..11029ea0c0bc6527df931042e6eed3e33897b9fd 100644 (file)
--- 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 6fc033b3e6c8e56092c7b5864cb6dbb710ae5bf5..3f030ab218f9912690e118f01d8bfc77f6d04f04 100644 (file)
--- 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 9c0214eeb0c655b238533991fdcad3a901dc859c..fccb1c1915f4f392edc71f4344f49188e56af3e8 100644 (file)
--- 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 90160e8da71d948c82c89b50894213510b05e8ef..14f16736d9e04431341a36222f5874f0c3e21551 100644 (file)
--- 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 8828211332591ea85e6a912031e79e3067b9473d..ea7f4942991c9abbac732b50b87dfc60884c7f45 100644 (file)
--- 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 08081c60730ed8e51c4f87264c630c895ff1d95c..871698d2628b57a922904f793fc3d5382e7bf4c7 100644 (file)
--- 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;