From e568d64f9fd3505454704f333bc1e58286f3419d Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 11 May 2023 14:14:31 +0100 Subject: 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 Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/20950) --- crypto/engine/eng_ctrl.c | 13 ++++------- crypto/engine/eng_init.c | 7 +++++- crypto/engine/eng_lib.c | 12 ++++++---- crypto/engine/eng_list.c | 57 ++++++++++++++++++++++++++++++++++++++--------- crypto/engine/eng_local.h | 4 +++- crypto/engine/tb_asnmth.c | 10 +++++++-- 6 files changed, 75 insertions(+), 28 deletions(-) diff --git a/crypto/engine/eng_ctrl.c b/crypto/engine/eng_ctrl.c index 5d7e15634e..2e085d6d8e 100644 --- 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 a146a39835..2f42593676 100644 --- 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 b10751336f..e80d77d6e3 100644 --- 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 dd9ada34c8..11db858056 100644 --- 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 ea69a5464e..a0e3a4734e 100644 --- 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 fb3953437a..fac038356b 100644 --- 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; -- cgit v1.2.3