From: Matt Caswell Date: Fri, 5 Nov 2021 13:42:40 +0000 (+0000) Subject: Don't attempt to deactive child providers if we don't need to X-Git-Tag: openssl-3.2.0-alpha1~3351 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c59fc87b338880893286934f02c446854f5baabf;p=thirdparty%2Fopenssl.git Don't attempt to deactive child providers if we don't need to If a provider doesn't have any child providers then there is no need to attempt to remove them - so we should not do so. This removes some potentialy thread races. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/16980) --- diff --git a/crypto/provider.c b/crypto/provider.c index 82d980a8aee..974c636bc10 100644 --- a/crypto/provider.c +++ b/crypto/provider.c @@ -35,7 +35,7 @@ OSSL_PROVIDER *OSSL_PROVIDER_try_load(OSSL_LIB_CTX *libctx, const char *name, actual = prov; if (isnew && !ossl_provider_add_to_store(prov, &actual, retain_fallbacks)) { - ossl_provider_deactivate(prov); + ossl_provider_deactivate(prov, 1); ossl_provider_free(prov); return NULL; } @@ -53,7 +53,7 @@ OSSL_PROVIDER *OSSL_PROVIDER_load(OSSL_LIB_CTX *libctx, const char *name) int OSSL_PROVIDER_unload(OSSL_PROVIDER *prov) { - if (!ossl_provider_deactivate(prov)) + if (!ossl_provider_deactivate(prov, 1)) return 0; ossl_provider_free(prov); return 1; diff --git a/crypto/provider_child.c b/crypto/provider_child.c index 272d67a52d8..8f220c452f9 100644 --- a/crypto/provider_child.c +++ b/crypto/provider_child.c @@ -153,7 +153,7 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata) if (!ossl_provider_set_child(cprov, prov) || !ossl_provider_add_to_store(cprov, NULL, 0)) { - ossl_provider_deactivate(cprov); + ossl_provider_deactivate(cprov, 0); ossl_provider_free(cprov); goto err; } @@ -188,7 +188,7 @@ static int provider_remove_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata) */ ossl_provider_free(cprov); if (ossl_provider_is_child(cprov) - && !ossl_provider_deactivate(cprov)) + && !ossl_provider_deactivate(cprov, 1)) return 0; return 1; diff --git a/crypto/provider_conf.c b/crypto/provider_conf.c index 054261771aa..7acfe495646 100644 --- a/crypto/provider_conf.c +++ b/crypto/provider_conf.c @@ -222,7 +222,7 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name, if (!ossl_provider_activate(prov, 1, 0)) { ok = 0; } else if (!ossl_provider_add_to_store(prov, &actual, 0)) { - ossl_provider_deactivate(prov); + ossl_provider_deactivate(prov, 1); ok = 0; } else { if (pcgbl->activated_providers == NULL) diff --git a/crypto/provider_core.c b/crypto/provider_core.c index 3b8d3fbb6d0..884d71564a4 100644 --- a/crypto/provider_core.c +++ b/crypto/provider_core.c @@ -229,7 +229,7 @@ struct provider_store_st { static void provider_deactivate_free(OSSL_PROVIDER *prov) { if (prov->flag_activated) - ossl_provider_deactivate(prov); + ossl_provider_deactivate(prov, 1); ossl_provider_free(prov); } @@ -498,7 +498,7 @@ static int provider_up_ref_intern(OSSL_PROVIDER *prov, int activate) static int provider_free_intern(OSSL_PROVIDER *prov, int deactivate) { if (deactivate) - return ossl_provider_deactivate(prov); + return ossl_provider_deactivate(prov, 1); ossl_provider_free(prov); return 1; @@ -644,8 +644,11 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov, * name and raced to put them in the store. This thread lost. We * deactivate the one we just created and use the one that already * exists instead. + * If we get here then we know we did not create provider children + * above, so we inform ossl_provider_deactivate not to attempt to remove + * any. */ - ossl_provider_deactivate(prov); + ossl_provider_deactivate(prov, 0); ossl_provider_free(prov); } @@ -1002,15 +1005,18 @@ static int provider_init(OSSL_PROVIDER *prov) } /* - * Deactivate a provider. + * Deactivate a provider. If upcalls is 0 then we suppress any upcalls to a + * parent provider. If removechildren is 0 then we suppress any calls to remove + * child providers. * Return -1 on failure and the activation count on success */ -static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls) +static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls, + int removechildren) { int count; struct provider_store_st *store; #ifndef FIPS_MODULE - int freeparent = 0, removechildren = 0; + int freeparent = 0; #endif if (!ossl_assert(prov != NULL)) @@ -1039,12 +1045,12 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls) } #endif - if ((count = --prov->activatecnt) < 1) { + if ((count = --prov->activatecnt) < 1) prov->flag_activated = 0; #ifndef FIPS_MODULE - removechildren = 1; + else + removechildren = 0; #endif - } CRYPTO_THREAD_unlock(prov->flag_lock); @@ -1169,11 +1175,12 @@ int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild) return 0; } -int ossl_provider_deactivate(OSSL_PROVIDER *prov) +int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren) { int count; - if (prov == NULL || (count = provider_deactivate(prov, 1)) < 0) + if (prov == NULL + || (count = provider_deactivate(prov, 1, removechildren)) < 0) return 0; return count == 0 ? provider_flush_store_cache(prov) : 1; } @@ -1355,7 +1362,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx, for (curr++; curr < max; curr++) { OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr); - provider_deactivate(prov, 0); + provider_deactivate(prov, 0, 1); /* * As above where we did the up-ref, we don't call ossl_provider_free * to avoid making upcalls. There should always be at least one ref diff --git a/doc/internal/man3/ossl_provider_new.pod b/doc/internal/man3/ossl_provider_new.pod index f6bdaecde24..7f6934a59e3 100644 --- a/doc/internal/man3/ossl_provider_new.pod +++ b/doc/internal/man3/ossl_provider_new.pod @@ -53,7 +53,7 @@ ossl_provider_get_capabilities * If the Provider is a module, the module will be loaded */ int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild); - int ossl_provider_deactivate(OSSL_PROVIDER *prov); + int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren); int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov, int retain_fallbacks); @@ -220,7 +220,9 @@ no action is taken and ossl_provider_activate() returns success. ossl_provider_deactivate() "deactivates" the provider for the given provider object I by decrementing its activation count. When -that count reaches zero, the activation flag is cleared. +that count reaches zero, the activation flag is cleared. If the +I parameter is 0 then no attempt is made to remove any +associated child providers. ossl_provider_add_to_store() adds the provider I to the provider store and makes it available to other threads. This will prevent future automatic loading diff --git a/include/internal/provider.h b/include/internal/provider.h index 9b9c62ebe87..ae8d7434374 100644 --- a/include/internal/provider.h +++ b/include/internal/provider.h @@ -56,7 +56,7 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx); * If the Provider is a module, the module will be loaded */ int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild); -int ossl_provider_deactivate(OSSL_PROVIDER *prov); +int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren); int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov, int retain_fallbacks); diff --git a/test/provider_internal_test.c b/test/provider_internal_test.c index d9cc68d59dc..cb7d5efcf54 100644 --- a/test/provider_internal_test.c +++ b/test/provider_internal_test.c @@ -31,7 +31,7 @@ static int test_provider(OSSL_PROVIDER *prov, const char *expected_greeting) && TEST_ptr(greeting = greeting_request[0].data) && TEST_size_t_gt(greeting_request[0].data_size, 0) && TEST_str_eq(greeting, expected_greeting) - && TEST_true(ossl_provider_deactivate(prov)); + && TEST_true(ossl_provider_deactivate(prov, 1)); TEST_info("Got this greeting: %s\n", greeting); ossl_provider_free(prov);