Convert the ENGINE struct_ref field to be an atomic

We use atomic primitives to up ref and down the struct_ref field rather
than relying on the global lock for this.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20950)

diff --git a/crypto/engine/eng_ctrl.c b/crypto/engine/eng_ctrl.c
index 5d7e15634e6ef02736130b4ee78c8932fcc15dac..2e085d6d8e949a291c1f597329d373afa2b7b153 100644 (file)
--- a/crypto/engine/eng_ctrl.c
+++ b/crypto/engine/eng_ctrl.c
@@ -127,20 +127,15 @@ static int int_ctrl_helper(ENGINE *e, int cmd, long i, void *p,
 
 int ENGINE_ctrl(ENGINE *e, int cmd, long i, void *p, void (*f) (void))
 {
-    int ctrl_exists, ref_exists;
+    int ctrl_exists;
+
     if (e == NULL) {
         ERR_raise(ERR_LIB_ENGINE, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
     }
-    if (!CRYPTO_THREAD_write_lock(global_engine_lock))
-        return 0;
-    ref_exists = ((e->struct_ref > 0) ? 1 : 0);
-    CRYPTO_THREAD_unlock(global_engine_lock);
+
     ctrl_exists = ((e->ctrl == NULL) ? 0 : 1);
-    if (!ref_exists) {
-        ERR_raise(ERR_LIB_ENGINE, ENGINE_R_NO_REFERENCE);
-        return 0;
-    }
+
     /*
      * Intercept any "root-level" commands before trying to hand them on to
      * ctrl() handlers.

diff --git a/crypto/engine/eng_init.c b/crypto/engine/eng_init.c
index a146a3983565fd6a80f0c66ca6bd6e373971aa08..2f4259367638617681632b729d6553d0b32df28b 100644 (file)
--- a/crypto/engine/eng_init.c
+++ b/crypto/engine/eng_init.c
@@ -28,11 +28,16 @@ int engine_unlocked_init(ENGINE *e)
          */
         to_return = e->init(e);
     if (to_return) {
+        int ref;
+
         /*
          * OK, we return a functional reference which is also a structural
          * reference.
          */
-        e->struct_ref++;
+        if (!CRYPTO_UP_REF(&e->struct_ref, &ref, e->refcnt_lock)) {
+            e->finish(e);
+            return 0;
+        }
         e->funct_ref++;
         ENGINE_REF_PRINT(e, 0, 1);
         ENGINE_REF_PRINT(e, 1, 1);

diff --git a/crypto/engine/eng_lib.c b/crypto/engine/eng_lib.c
index b10751336fac927f1511be85efabc08116eb5831..e80d77d6e3a77555dbc68fb7595c6d39949bf40c 100644 (file)
--- a/crypto/engine/eng_lib.c
+++ b/crypto/engine/eng_lib.c
@@ -37,7 +37,13 @@ ENGINE *ENGINE_new(void)
         return NULL;
     ret->struct_ref = 1;
     ENGINE_REF_PRINT(ret, 0, 1);
+    ret->refcnt_lock = CRYPTO_THREAD_lock_new();
+    if (ret->refcnt_lock == NULL) {
+        OPENSSL_free(ret);
+        return NULL;
+    }
     if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_ENGINE, ret, &ret->ex_data)) {
+        CRYPTO_THREAD_lock_free(ret->refcnt_lock);
         OPENSSL_free(ret);
         return NULL;
     }
@@ -76,10 +82,7 @@ int engine_free_util(ENGINE *e, int not_locked)
 
     if (e == NULL)
         return 1;
-    if (not_locked)
-        CRYPTO_DOWN_REF(&e->struct_ref, &i, global_engine_lock);
-    else
-        i = --e->struct_ref;
+    CRYPTO_DOWN_REF(&e->struct_ref, &i, e->refcnt_lock);
     ENGINE_REF_PRINT(e, 0, -1);
     if (i > 0)
         return 1;
@@ -95,6 +98,7 @@ int engine_free_util(ENGINE *e, int not_locked)
         e->destroy(e);
     engine_remove_dynamic_id(e, not_locked);
     CRYPTO_free_ex_data(CRYPTO_EX_INDEX_ENGINE, e, &e->ex_data);
+    CRYPTO_THREAD_lock_free(e->refcnt_lock);
     OPENSSL_free(e);
     return 1;
 }

diff --git a/crypto/engine/eng_list.c b/crypto/engine/eng_list.c
index dd9ada34c851f4191c3c4bc1fa3d85354651ac29..11db85805688d7d641ea2bec319b326481560fd5 100644 (file)
--- a/crypto/engine/eng_list.c
+++ b/crypto/engine/eng_list.c
@@ -58,6 +58,7 @@ static int engine_list_add(ENGINE *e)
 {
     int conflict = 0;
     ENGINE *iterator = NULL;
+    int ref;
 
     if (e == NULL) {
         ERR_raise(ERR_LIB_ENGINE, ERR_R_PASSED_NULL_PARAMETER);
@@ -72,9 +73,19 @@ static int engine_list_add(ENGINE *e)
         ERR_raise(ERR_LIB_ENGINE, ENGINE_R_CONFLICTING_ENGINE_ID);
         return 0;
     }
+
+    /*
+     * Having the engine in the list assumes a structural reference.
+     */
+    if (!CRYPTO_UP_REF(&e->struct_ref, &ref, e->refcnt_lock)) {
+            ERR_raise(ERR_LIB_ENGINE, ENGINE_R_INTERNAL_LIST_ERROR);
+            return 0;
+    }
+    ENGINE_REF_PRINT(e, 0, 1);
     if (engine_list_head == NULL) {
         /* We are adding to an empty list. */
-        if (engine_list_tail) {
+        if (engine_list_tail != NULL) {
+            CRYPTO_DOWN_REF(&e->struct_ref, &ref, e->refcnt_lock);
             ERR_raise(ERR_LIB_ENGINE, ENGINE_R_INTERNAL_LIST_ERROR);
             return 0;
         }
@@ -87,17 +98,14 @@ static int engine_list_add(ENGINE *e)
     } else {
         /* We are adding to the tail of an existing list. */
         if ((engine_list_tail == NULL) || (engine_list_tail->next != NULL)) {
+            CRYPTO_DOWN_REF(&e->struct_ref, &ref, e->refcnt_lock);
             ERR_raise(ERR_LIB_ENGINE, ENGINE_R_INTERNAL_LIST_ERROR);
             return 0;
         }
         engine_list_tail->next = e;
         e->prev = engine_list_tail;
     }
-    /*
-     * Having the engine in the list assumes a structural reference.
-     */
-    e->struct_ref++;
-    ENGINE_REF_PRINT(e, 0, 1);
+
     /* However it came to be, e is the last item in the list. */
     engine_list_tail = e;
     e->next = NULL;
@@ -228,7 +236,12 @@ ENGINE *ENGINE_get_first(void)
         return NULL;
     ret = engine_list_head;
     if (ret) {
-        ret->struct_ref++;
+        int ref;
+
+        if (!CRYPTO_UP_REF(&ret->struct_ref, &ref, ret->refcnt_lock)) {
+            ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
         ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
@@ -249,7 +262,12 @@ ENGINE *ENGINE_get_last(void)
         return NULL;
     ret = engine_list_tail;
     if (ret) {
-        ret->struct_ref++;
+        int ref;
+
+        if (!CRYPTO_UP_REF(&ret->struct_ref, &ref, ret->refcnt_lock)) {
+            ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
         ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
@@ -268,8 +286,13 @@ ENGINE *ENGINE_get_next(ENGINE *e)
         return NULL;
     ret = e->next;
     if (ret) {
+        int ref;
+
         /* Return a valid structural reference to the next ENGINE */
-        ret->struct_ref++;
+        if (!CRYPTO_UP_REF(&ret->struct_ref, &ref, ret->refcnt_lock)) {
+            ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
         ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
@@ -289,8 +312,13 @@ ENGINE *ENGINE_get_prev(ENGINE *e)
         return NULL;
     ret = e->prev;
     if (ret) {
+        int ref;
+
         /* Return a valid structural reference to the next ENGINE */
-        ret->struct_ref++;
+        if (!CRYPTO_UP_REF(&ret->struct_ref, &ref, ret->refcnt_lock)) {
+            ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
         ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
@@ -405,7 +433,14 @@ ENGINE *ENGINE_by_id(const char *id)
                 iterator = cp;
             }
         } else {
-            iterator->struct_ref++;
+            int ref;
+
+            if (!CRYPTO_UP_REF(&iterator->struct_ref, &ref,
+                               iterator->refcnt_lock)) {
+                CRYPTO_THREAD_unlock(global_engine_lock);
+                ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+                return NULL;
+            }
             ENGINE_REF_PRINT(iterator, 0, 1);
         }
     }

diff --git a/crypto/engine/eng_local.h b/crypto/engine/eng_local.h
index ea69a5464eb7d83c735f12be2fc2b6ab3f14a44a..a0e3a4734e44ee3ea8579b500ea19016e864ee4c 100644 (file)
--- a/crypto/engine/eng_local.h
+++ b/crypto/engine/eng_local.h
@@ -23,7 +23,8 @@ extern CRYPTO_RWLOCK *global_engine_lock;
  * This prints the engine's pointer address, "struct" or "funct" to
  * indicate the reference type, the before and after reference count, and
  * the file:line-number pair. The "ENGINE_REF_PRINT" statements must come
- * *after* the change.
+ * *after* the change. Since this is for tracing only we do not concern
+ * ourselves with using atomic primitives for reading the struct_ref
  */
 # define ENGINE_REF_PRINT(e, isfunct, diff)                             \
     OSSL_TRACE6(ENGINE_REF_COUNT,                                       \
@@ -135,6 +136,7 @@ struct engine_st {
     int flags;
     /* reference count on the structure itself */
     CRYPTO_REF_COUNT struct_ref;
+    CRYPTO_RWLOCK *refcnt_lock;
     /*
      * reference count on usability of the engine type. NB: This controls the
      * loading and initialisation of any functionality required by this

diff --git a/crypto/engine/tb_asnmth.c b/crypto/engine/tb_asnmth.c
index fb3953437af35bbe2fc9f4b5d327722414398264..fac038356bf4bcfb6dee834ea011348f80c114ce 100644 (file)
--- a/crypto/engine/tb_asnmth.c
+++ b/crypto/engine/tb_asnmth.c
@@ -205,8 +205,14 @@ const EVP_PKEY_ASN1_METHOD *ENGINE_pkey_asn1_find_str(ENGINE **pe,
         return NULL;
     engine_table_doall(pkey_asn1_meth_table, look_str_cb, &fstr);
     /* If found obtain a structural reference to engine */
-    if (fstr.e) {
-        fstr.e->struct_ref++;
+    if (fstr.e != NULL) {
+        int ref;
+
+        if (!CRYPTO_UP_REF(&fstr.e->struct_ref, &ref, fstr.e->refcnt_lock)) {
+            CRYPTO_THREAD_unlock(global_engine_lock);
+            ERR_raise(ERR_LIB_ENGINE, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
         ENGINE_REF_PRINT(fstr.e, 0, 1);
     }
     *pe = fstr.e;