]> git.ipfire.org Git - thirdparty/openssl.git/commitdiff
Fix ignored call in ossl_rcu_call when cb item alloc fails
authorJakub Zelenka <jakub.zelenka@openssl.foundation>
Tue, 5 May 2026 17:50:11 +0000 (19:50 +0200)
committerTomas Mraz <tomas@openssl.foundation>
Mon, 11 May 2026 08:21:48 +0000 (10:21 +0200)
Currently when allocation of cb item fails, the actual cb function is
not called. The is used just in hashtable when the cb function frees
the old item which result in memory leak.

To fix this, the allocation needs to be separated and happen before the
assign operation is done.

Reviewed-by: Nikola Pajkovsky <nikolap@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
MergeDate: Mon May 11 08:21:55 2026
(Merged from https://github.com/openssl/openssl/pull/31092)

crypto/hashtable/hashtable.c
crypto/threads_none.c
crypto/threads_pthread.c
crypto/threads_win.c
doc/internal/man3/ossl_rcu_lock_new.pod
include/internal/rcu.h
test/threadstest.c

index febc8f69d172ff4edad20b0bc9ab3c5c3d713de1..83fbc1ac7363608c75336e9aea74459eaebc2f97 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright 2024-2025 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 2024-2026 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the Apache License 2.0 (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -311,6 +311,7 @@ static int ossl_ht_flush_internal(HT *h)
 {
     struct ht_mutable_data_st *newmd = NULL;
     struct ht_mutable_data_st *oldmd = NULL;
+    CRYPTO_RCU_CB_ITEM *cbi = NULL;
 
     newmd = OPENSSL_zalloc(sizeof(*newmd));
     if (newmd == NULL)
@@ -325,8 +326,15 @@ static int ossl_ht_flush_internal(HT *h)
 
     newmd->neighborhood_mask = DEFAULT_NEIGH_LEN - 1;
 
-    /* Swap the old and new mutable data sets */
     if (!h->config.no_rcu) {
+        cbi = ossl_rcu_cb_item_new();
+        if (cbi == NULL) {
+            OPENSSL_free(newmd->neighborhood_ptr_to_free);
+            OPENSSL_free(newmd);
+            return 0;
+        }
+
+        /* Swap the old and new mutable data sets */
         oldmd = ossl_rcu_deref(&h->md);
         ossl_rcu_assign_ptr(&h->md, &newmd);
     } else {
@@ -339,11 +347,11 @@ static int ossl_ht_flush_internal(HT *h)
     h->wpd.neighborhood_len = DEFAULT_NEIGH_LEN;
 
     if (!h->config.no_rcu) {
-        ossl_rcu_call(h->lock, free_oldmd, oldmd);
+        ossl_rcu_call(h->lock, cbi, free_oldmd, oldmd);
+        h->wpd.need_sync = 1;
     } else {
         free_oldmd(oldmd);
     }
-    h->wpd.need_sync = 1;
 
     return 1;
 }
@@ -461,6 +469,7 @@ static int grow_hashtable(HT *h, size_t oldsize)
 {
     struct ht_mutable_data_st *newmd;
     struct ht_mutable_data_st *oldmd = ossl_rcu_deref(&h->md);
+    CRYPTO_RCU_CB_ITEM *cbi = NULL;
     int rc = 0;
     uint64_t oldi, oldj, newi, newj;
     uint64_t oldhash;
@@ -513,6 +522,16 @@ static int grow_hashtable(HT *h, size_t oldsize)
             }
         }
     }
+
+    /*
+     * Pre allocate the rcu callback item before assigning the newmd.
+     */
+    if (!h->config.no_rcu) {
+        cbi = ossl_rcu_cb_item_new();
+        if (cbi == NULL)
+            goto out_free;
+    }
+
     /*
      * Now that our entries are all hashed into the new bucket list
      * update our bucket_len and target_max_load
@@ -524,7 +543,7 @@ static int grow_hashtable(HT *h, size_t oldsize)
      */
     if (!h->config.no_rcu) {
         ossl_rcu_assign_ptr(&h->md, &newmd);
-        ossl_rcu_call(h->lock, free_old_neigh_table, oldmd);
+        ossl_rcu_call(h->lock, cbi, free_old_neigh_table, oldmd);
         h->wpd.need_sync = 1;
     } else {
         h->md = newmd;
@@ -612,13 +631,18 @@ static int ossl_ht_insert_locked(HT *h, uint64_t hash,
                 }
                 /* Do a replacement */
                 if (!h->config.no_rcu) {
+                    CRYPTO_RCU_CB_ITEM *cbi = ossl_rcu_cb_item_new();
+                    if (cbi == NULL)
+                        return 0;
                     if (!CRYPTO_atomic_store(&md->neighborhoods[neigh_idx].entries[j].hash,
-                            hash, h->atomic_lock))
+                            hash, h->atomic_lock)) {
+                        ossl_rcu_cb_item_free(cbi);
                         return 0;
+                    }
                     *olddata = (HT_VALUE *)md->neighborhoods[neigh_idx].entries[j].value;
                     ossl_rcu_assign_ptr(&md->neighborhoods[neigh_idx].entries[j].value,
                         &newval);
-                    ossl_rcu_call(h->lock, free_old_ht_value, *olddata);
+                    ossl_rcu_call(h->lock, cbi, free_old_ht_value, *olddata);
                 } else {
                     md->neighborhoods[neigh_idx].entries[j].hash = hash;
                     *olddata = (HT_VALUE *)md->neighborhoods[neigh_idx].entries[j].value;
@@ -812,26 +836,27 @@ int ossl_ht_delete(HT *h, HT_KEY *key)
         if (compare_hash(hash, h->md->neighborhoods[neigh_idx].entries[j].hash)
             && match_key(key, &v->value.key)) {
             if (!h->config.no_rcu) {
+                CRYPTO_RCU_CB_ITEM *cbi = ossl_rcu_cb_item_new();
+                if (cbi == NULL)
+                    break;
                 if (!CRYPTO_atomic_store(&h->md->neighborhoods[neigh_idx].entries[j].hash,
-                        0, h->atomic_lock))
+                        0, h->atomic_lock)) {
+                    ossl_rcu_cb_item_free(cbi);
                     break;
+                }
                 ossl_rcu_assign_ptr(&h->md->neighborhoods[neigh_idx].entries[j].value, &nv);
+                ossl_rcu_call(h->lock, cbi, free_old_entry, v);
             } else {
                 h->md->neighborhoods[neigh_idx].entries[j].hash = 0;
                 h->md->neighborhoods[neigh_idx].entries[j].value = NULL;
+                free_old_entry(v);
             }
             h->wpd.value_count--;
+            h->wpd.need_sync = 1;
             rc = 1;
             break;
         }
     }
-    if (rc == 1) {
-        if (!h->config.no_rcu)
-            ossl_rcu_call(h->lock, free_old_entry, v);
-        else
-            free_old_entry(v);
-        h->wpd.need_sync = 1;
-    }
 
     return rc;
 }
index 2453983488a80b62ec759d40202779f35dfd9e23..44c4e227edaf65bd221b54ff252cf4ec9bc6199e 100644 (file)
@@ -72,18 +72,23 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
     }
 }
 
-int ossl_rcu_call(CRYPTO_RCU_LOCK *lock, rcu_cb_fn cb, void *data)
+CRYPTO_RCU_CB_ITEM *ossl_rcu_cb_item_new(void)
 {
-    struct rcu_cb_item *new = OPENSSL_zalloc(sizeof(*new));
+    return OPENSSL_zalloc(sizeof(CRYPTO_RCU_CB_ITEM));
+}
 
-    if (new == NULL)
-        return 0;
+void ossl_rcu_cb_item_free(CRYPTO_RCU_CB_ITEM *item)
+{
+    OPENSSL_free(item);
+}
 
-    new->fn = cb;
-    new->data = data;
-    new->next = lock->cb_items;
-    lock->cb_items = new;
-    return 1;
+void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, CRYPTO_RCU_CB_ITEM *item,
+    rcu_cb_fn cb, void *data)
+{
+    item->fn = cb;
+    item->data = data;
+    item->next = lock->cb_items;
+    lock->cb_items = item;
 }
 
 void *ossl_rcu_uptr_deref(void **p)
index 2598c26bbc7419a41b135002d6d73ad57a45a178..52c98a1c2ea88c5ad7aef7403c0e2c49ccceaba5 100644 (file)
@@ -517,24 +517,27 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
     }
 }
 
+CRYPTO_RCU_CB_ITEM *ossl_rcu_cb_item_new(void)
+{
+    return OPENSSL_zalloc(sizeof(CRYPTO_RCU_CB_ITEM));
+}
+
+void ossl_rcu_cb_item_free(CRYPTO_RCU_CB_ITEM *item)
+{
+    OPENSSL_free(item);
+}
+
 /*
  * Note: This call assumes its made under the protection of
  * ossl_rcu_write_lock
  */
-int ossl_rcu_call(CRYPTO_RCU_LOCK *lock, rcu_cb_fn cb, void *data)
+void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, CRYPTO_RCU_CB_ITEM *item,
+    rcu_cb_fn cb, void *data)
 {
-    struct rcu_cb_item *new = OPENSSL_zalloc(sizeof(*new));
-
-    if (new == NULL)
-        return 0;
-
-    new->data = data;
-    new->fn = cb;
-
-    new->next = lock->cb_items;
-    lock->cb_items = new;
-
-    return 1;
+    item->fn = cb;
+    item->data = data;
+    item->next = lock->cb_items;
+    lock->cb_items = item;
 }
 
 void *ossl_rcu_uptr_deref(void **p)
index 4a2b1b7dedf90f831bbddf8e9492331acc1af693..4fe59b3d08ed122b771bc6a38656269c85856841 100644 (file)
@@ -394,23 +394,26 @@ void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock)
     return;
 }
 
+CRYPTO_RCU_CB_ITEM *ossl_rcu_cb_item_new(void)
+{
+    return OPENSSL_zalloc(sizeof(CRYPTO_RCU_CB_ITEM));
+}
+
+void ossl_rcu_cb_item_free(CRYPTO_RCU_CB_ITEM *item)
+{
+    OPENSSL_free(item);
+}
+
 /*
  * Note, must be called under the protection of ossl_rcu_write_lock
  */
-int ossl_rcu_call(CRYPTO_RCU_LOCK *lock, rcu_cb_fn cb, void *data)
+void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, CRYPTO_RCU_CB_ITEM *item,
+    rcu_cb_fn cb, void *data)
 {
-    struct rcu_cb_item *new;
-
-    new = OPENSSL_zalloc(sizeof(struct rcu_cb_item));
-    if (new == NULL)
-        return 0;
-    new->data = data;
-    new->fn = cb;
-
-    new->next = lock->cb_items;
-    lock->cb_items = new;
-
-    return 1;
+    item->fn = cb;
+    item->data = data;
+    item->next = lock->cb_items;
+    lock->cb_items = item;
 }
 
 void *ossl_rcu_uptr_deref(void **p)
index 57b5e4d73d2ff8e3bfd56d449f0a109abe18360e..aca2693e2313f68a58ec9ce23ea46a385b0590ed 100644 (file)
@@ -6,6 +6,7 @@ ossl_rcu_lock_new,
 ossl_rcu_lock_free, ossl_rcu_read_lock,
 ossl_rcu_read_unlock, ossl_rcu_write_lock,
 ossl_rcu_write_unlock, ossl_synchronize_rcu,
+ossl_rcu_cb_item_new, ossl_rcu_cb_item_free,
 ossl_rcu_call, ossl_rcu_deref,
 ossl_rcu_assign_ptr, ossl_rcu_uptr_deref,
 ossl_rcu_assign_uptr
@@ -19,7 +20,10 @@ ossl_rcu_assign_uptr
  void ossl_rcu_write_unlock(CRYPTO_RCU_LOCK *lock);
  void ossl_rcu_read_unlock(CRYPTO_RCU_LOCK *lock);
  void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock);
- void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, rcu_cb_fn cb, void *data);
+ CRYPTO_RCU_CB_ITEM *ossl_rcu_cb_item_new(void);
+ void ossl_rcu_cb_item_free(CRYPTO_RCU_CB_ITEM *item);
+ void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, CRYPTO_RCU_CB_ITEM *item,
+                    rcu_cb_fn cb, void *data);
  void *ossl_rcu_deref(void **p);
  void ossl_rcu_uptr_deref(void **p);
  void ossl_rcu_assign_ptr(void **p, void **v);
@@ -96,10 +100,29 @@ the write side thread is safe to free.
 
 =item *
 
-ossl_rcu_call() enqueues a callback function to the lock, to be called
-when the next synchronization completes.  Note: It is not guaranteed that the
-thread which enqueued the callback will be the thread which executes the
-callback
+ossl_rcu_cb_item_new() allocates a callback item suitable for use with
+ossl_rcu_call().  Returns NULL on allocation failure.  The item is owned by
+the caller until it is passed to ossl_rcu_call(), at which point ownership
+transfers to the lock and the item must not be touched again by the caller.
+
+=item *
+
+ossl_rcu_cb_item_free() frees a callback item that was allocated by
+ossl_rcu_cb_item_new() but never passed to ossl_rcu_call().  Use this to
+release the item on the failure path of an operation that decided not to
+publish its update.
+
+=item *
+
+ossl_rcu_call() enqueues a callback function I<cb> to the lock, to be
+called with I<data> when the next synchronization completes.  The caller
+must provide a callback item I<item> previously obtained from
+ossl_rcu_cb_item_new().  After this call the lock owns the item and will
+free it after invoking the callback.  This function does not allocate and
+cannot fail, which lets callers allocate the item before performing any
+publish (assign_ptr) and bail cleanly if allocation fails.  Note: it is
+not guaranteed that the thread which enqueued the callback will be the
+thread which executes the callback.
 
 =item *
 
@@ -121,6 +144,9 @@ ossl_rcu_lock_free() frees an allocated RCU lock
 
 ossl_rcu_lock_new() returns a pointer to a newly created RCU lock structure.
 
+ossl_rcu_cb_item_new() returns a pointer to a newly created callback item,
+or NULL on allocation failure.
+
 ossl_rcu_deref() and ossl_rcu_uptr_deref() return the value pointed
 to by the passed in value v.
 
@@ -152,7 +178,7 @@ This example safely initializes and uses a lock.
 
  static void myinit(void)
  {
-     lock = ossl_rcu_lock_new(1);
+     lock = ossl_rcu_lock_new(1, NULL);
  }
 
  static int initlock(void)
@@ -162,10 +188,16 @@ This example safely initializes and uses a lock.
      return 1;
  }
 
- static void writer_thread()
+ static void free_old_foo(void *data)
+ {
+     OPENSSL_free(data);
+ }
+
+ static int writer_thread(void)
  {
     struct foo *newfoo;
     struct foo *oldfoo;
+    CRYPTO_RCU_CB_ITEM *cbi;
 
     initlock();
 
@@ -177,48 +209,60 @@ This example safely initializes and uses a lock.
      * 1) create a new shared object
      */
     newfoo = OPENSSL_zalloc(sizeof(struct foo));
+    if (newfoo == NULL)
+        return 0;
 
     /*
-     * acquire the write side lock
+     * 2) Pre allocate the rcu callback item before any publish.
+     */
+    cbi = ossl_rcu_cb_item_new();
+    if (cbi == NULL) {
+        OPENSSL_free(newfoo);
+        return 0;
+    }
+
+    /*
+     * 3) acquire the write side lock
      */
     ossl_rcu_write_lock(lock);
 
     /*
-     * 2) read the old pointer
+     * 4) read the old pointer
      */
     oldfoo = ossl_rcu_deref(&fooptr);
 
     /*
-     * 3) Copy the old pointer to the new object, and
+     * 5) Copy the old pointer to the new object, and
      *    make any needed adjustments
      */
     memcpy(newfoo, oldfoo, sizeof(struct foo));
     newfoo->aval++;
 
     /*
-     * 4) Update the shared pointer to the new value
+     * 6) Update the shared pointer to the new value
      */
     ossl_rcu_assign_ptr(&fooptr, &newfoo);
 
     /*
-     * 5) Release the write side lock
+     * 7) Schedule the old pointer to be freed when readers are done.
      */
-    ossl_rcu_write_unlock(lock);
+    ossl_rcu_call(lock, cbi, free_old_foo, oldfoo);
 
     /*
-     * 6) wait for any read side holds on the old data
-     *    to be released
+     * 8) Release the write side lock
      */
-    ossl_synchronize_rcu(lock);
+    ossl_rcu_write_unlock(lock);
 
     /*
-     * 7) free the old pointer, now that there are no
-     *    further readers
+     * 9) wait for any read side holds on the old data
+     *    to be released, after which free_old_foo will run
      */
-    OPENSSL_free(oldfoo);
+    ossl_synchronize_rcu(lock);
+
+    return 1;
  }
 
- static void reader_thread()
+ static void reader_thread(void)
  {
     struct foo *myfoo = NULL;
     int a;
@@ -249,7 +293,7 @@ L<crypto(7)>, L<openssl-threads(7)>.
 
 =head1 COPYRIGHT
 
-Copyright 2023-2024 The OpenSSL Project Authors. All Rights Reserved.
+Copyright 2023-2026 The OpenSSL Project Authors. All Rights Reserved.
 
 Licensed under the Apache License 2.0 (the "License").  You may not use
 this file except in compliance with the License.  You can obtain a copy
index c6fdc93e9d1e7ca1abddb644a8cf50e718a9af3a..7090e5b2562c2574cf81d90ec58684e0374c3bc1 100644 (file)
@@ -17,6 +17,8 @@ typedef void (*rcu_cb_fn)(void *data);
 
 typedef struct rcu_lock_st CRYPTO_RCU_LOCK;
 
+typedef struct rcu_cb_item CRYPTO_RCU_CB_ITEM;
+
 CRYPTO_RCU_LOCK *ossl_rcu_lock_new(int num_writers, OSSL_LIB_CTX *ctx);
 void ossl_rcu_lock_free(CRYPTO_RCU_LOCK *lock);
 int ossl_rcu_read_lock(CRYPTO_RCU_LOCK *lock);
@@ -24,7 +26,10 @@ 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);
 void ossl_synchronize_rcu(CRYPTO_RCU_LOCK *lock);
-int ossl_rcu_call(CRYPTO_RCU_LOCK *lock, rcu_cb_fn cb, void *data);
+CRYPTO_RCU_CB_ITEM *ossl_rcu_cb_item_new(void);
+void ossl_rcu_cb_item_free(CRYPTO_RCU_CB_ITEM *item);
+void ossl_rcu_call(CRYPTO_RCU_LOCK *lock, CRYPTO_RCU_CB_ITEM *item,
+    rcu_cb_fn cb, void *data);
 void *ossl_rcu_uptr_deref(void **p);
 void ossl_rcu_assign_uptr(void **p, void **v);
 #define ossl_rcu_deref(p) ossl_rcu_uptr_deref((void **)p)
index 8dcb3f1afe7af954a03017511c7504a8b8dd2642..6915df3a3e20dcc44b3526c2f41eb1b099bcf052 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2025 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 2016-2026 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the Apache License 2.0 (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -320,6 +320,7 @@ static void writer_fn(int id, int *iterations)
     int count;
     OSSL_TIME t1, t2;
     uint64_t *old, *new;
+    CRYPTO_RCU_CB_ITEM *cbi = NULL;
 
     t1 = ossl_time_now();
 
@@ -327,6 +328,12 @@ static void writer_fn(int id, int *iterations)
         new = OPENSSL_zalloc(sizeof(uint64_t));
         OPENSSL_assert(new != NULL);
         *new = (uint64_t)0xBAD;
+
+        if (contention == 0) {
+            cbi = ossl_rcu_cb_item_new();
+            OPENSSL_assert(cbi != NULL);
+        }
+
         if (contention == 0)
             OSSL_sleep(1000);
         ossl_rcu_write_lock(rcu_lock);
@@ -335,7 +342,7 @@ static void writer_fn(int id, int *iterations)
         *new = global_ctr++;
         ossl_rcu_assign_ptr(&writer_ptr, &new);
         if (contention == 0)
-            ossl_rcu_call(rcu_lock, free_old_rcu_data, old);
+            ossl_rcu_call(rcu_lock, cbi, free_old_rcu_data, old);
         ossl_rcu_write_unlock(rcu_lock);
         if (contention != 0) {
             ossl_synchronize_rcu(rcu_lock);