diff options
author | Matt Caswell <matt@openssl.org> | 2021-11-05 13:42:40 +0000 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2021-11-12 17:16:14 +0000 |
commit | c59fc87b338880893286934f02c446854f5baabf (patch) | |
tree | 81f9329b87f63d8806e4faea3826e4c644180272 | |
parent | 6de9214a5062e9d015c84cbbab681184e16fccaa (diff) |
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 <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16980)
-rw-r--r-- | crypto/provider.c | 4 | ||||
-rw-r--r-- | crypto/provider_child.c | 4 | ||||
-rw-r--r-- | crypto/provider_conf.c | 2 | ||||
-rw-r--r-- | crypto/provider_core.c | 31 | ||||
-rw-r--r-- | doc/internal/man3/ossl_provider_new.pod | 6 | ||||
-rw-r--r-- | include/internal/provider.h | 2 | ||||
-rw-r--r-- | test/provider_internal_test.c | 2 |
7 files changed, 30 insertions, 21 deletions
diff --git a/crypto/provider.c b/crypto/provider.c index 82d980a8ae..974c636bc1 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 272d67a52d..8f220c452f 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 054261771a..7acfe49564 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 3b8d3fbb6d..884d71564a 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 f6bdaecde2..7f6934a59e 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<prov> 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<removechildren> parameter is 0 then no attempt is made to remove any +associated child providers. ossl_provider_add_to_store() adds the provider I<prov> 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 9b9c62ebe8..ae8d743437 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 d9cc68d59d..cb7d5efcf5 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); |