From: Matt Caswell Date: Mon, 21 Jun 2021 08:23:30 +0000 (+0100) Subject: Add a new provider to the store only after we activate it X-Git-Tag: openssl-3.0.0-beta2~214 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=29aff653150c363be2d84f789a10b46d99d5cab9;p=thirdparty%2Fopenssl.git Add a new provider to the store only after we activate it Rather than creating the provider, adding to the store and then activating it, we do things the other way around, i.e. activate first and then add to the store. This means that the activation should occur before other threads are aware of the provider. Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/15854) --- diff --git a/crypto/provider.c b/crypto/provider.c index 5aec157c1f1..12336acc570 100644 --- a/crypto/provider.c +++ b/crypto/provider.c @@ -17,17 +17,26 @@ OSSL_PROVIDER *OSSL_PROVIDER_try_load(OSSL_LIB_CTX *libctx, const char *name, int retain_fallbacks) { OSSL_PROVIDER *prov = NULL; + int isnew = 0; /* Find it or create it */ - if ((prov = ossl_provider_find(libctx, name, 0)) == NULL - && (prov = ossl_provider_new(libctx, name, NULL, 0)) == NULL) - return NULL; + if ((prov = ossl_provider_find(libctx, name, 0)) == NULL) { + if ((prov = ossl_provider_new(libctx, name, NULL, 0)) == NULL) + return NULL; + isnew = 1; + } if (!ossl_provider_activate(prov, retain_fallbacks, 1)) { ossl_provider_free(prov); return NULL; } + if (isnew && !ossl_provider_add_to_store(prov)) { + ossl_provider_deactivate(prov); + ossl_provider_free(prov); + return NULL; + } + return prov; } diff --git a/crypto/provider_child.c b/crypto/provider_child.c index 7ab161b795c..e808eafe24d 100644 --- a/crypto/provider_child.c +++ b/crypto/provider_child.c @@ -150,19 +150,21 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata) 1)) == NULL) goto err; - /* - * We free the newly created ref. We rely on the provider sticking around - * in the provider store. - */ - ossl_provider_free(cprov); - if (!ossl_provider_activate(cprov, 0, 0)) goto err; - if (!ossl_provider_set_child(cprov, prov)) { + if (!ossl_provider_set_child(cprov, prov) + || !ossl_provider_add_to_store(cprov)) { ossl_provider_deactivate(cprov); + ossl_provider_free(cprov); goto err; } + + /* + * We free the newly created ref. We rely on the provider sticking around + * in the provider store. + */ + ossl_provider_free(cprov); } ret = 1; diff --git a/crypto/provider_conf.c b/crypto/provider_conf.c index d53e1be2dc4..8e83264dc6a 100644 --- a/crypto/provider_conf.c +++ b/crypto/provider_conf.c @@ -173,6 +173,9 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name, if (ok) { if (!ossl_provider_activate(prov, 0, 1)) { ok = 0; + } else if (!ossl_provider_add_to_store(prov)) { + ossl_provider_deactivate(prov); + ok = 0; } else { if (pcgbl->activated_providers == NULL) pcgbl->activated_providers = sk_OSSL_PROVIDER_new_null(); @@ -181,7 +184,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name, } } - if (!(activate && ok)) + if (!ok) ossl_provider_free(prov); } else { struct provider_info_st entry; diff --git a/crypto/provider_core.c b/crypto/provider_core.c index 78a4b7f2cab..393aa006ca4 100644 --- a/crypto/provider_core.c +++ b/crypto/provider_core.c @@ -509,29 +509,38 @@ OSSL_PROVIDER *ossl_provider_new(OSSL_LIB_CTX *libctx, const char *name, prov->error_lib = ERR_get_next_error_library(); #endif - if (!CRYPTO_THREAD_write_lock(store->lock)) - return NULL; - if (!ossl_provider_up_ref(prov)) { /* +1 One reference for the store */ - ossl_provider_free(prov); /* -1 Reference that was to be returned */ - prov = NULL; - } else if (sk_OSSL_PROVIDER_push(store->providers, prov) == 0) { - ossl_provider_free(prov); /* -1 Store reference */ - ossl_provider_free(prov); /* -1 Reference that was to be returned */ - prov = NULL; - } - CRYPTO_THREAD_unlock(store->lock); - - if (prov == NULL) - ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); - /* * At this point, the provider is only partially "loaded". To be - * fully "loaded", ossl_provider_activate() must also be called. + * fully "loaded", ossl_provider_activate() must also be called and it must + * then be added to the provider store. */ return prov; } +int ossl_provider_add_to_store(OSSL_PROVIDER *prov) +{ + struct provider_store_st *store = NULL; + int ret = 1; + + if ((store = get_provider_store(prov->libctx)) == NULL) + return 0; + + + if (!ossl_provider_up_ref(prov)) { + ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); + return 0; + } + if (!CRYPTO_THREAD_write_lock(store->lock) + || sk_OSSL_PROVIDER_push(store->providers, prov) == 0) { + ossl_provider_free(prov); + ret = 0; + } + CRYPTO_THREAD_unlock(store->lock); + + return ret; +} + void ossl_provider_free(OSSL_PROVIDER *prov) { if (prov != NULL) { diff --git a/include/internal/provider.h b/include/internal/provider.h index 6432f8a2125..127ea2d2c57 100644 --- a/include/internal/provider.h +++ b/include/internal/provider.h @@ -62,6 +62,7 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx); int ossl_provider_activate(OSSL_PROVIDER *prov, int retain_fallbacks, int upcalls); int ossl_provider_deactivate(OSSL_PROVIDER *prov); +int ossl_provider_add_to_store(OSSL_PROVIDER *prov); /* Return pointer to the provider's context */ void *ossl_provider_ctx(const OSSL_PROVIDER *prov);