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)

diff --git a/crypto/provider.c b/crypto/provider.c
index 82d980a8aee3358d66d5bafe66feb954ddd7d40d..974c636bc101c98492bd2d8265e17e41e168468c 100644 (file)
--- 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 272d67a52d80ace5e503bc1b002751e2d0610118..8f220c452f946eab048234f44f5b47ee974807bb 100644 (file)
--- 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 054261771aa318626129251e9f837957da58760f..7acfe4956463161b174964a5f66aebcea5f28bfe 100644 (file)
--- 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 3b8d3fbb6d0969e1e47fd20fe37efef3c498bb62..884d71564a472493f5baef0adf7f8bf9c6fa2619 100644 (file)
--- 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 f6bdaecde2495968dc754f9946265e5425db30d6..7f6934a59e38ae599d2c349c8d6e47cc90db43cb 100644 (file)
--- 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 9b9c62ebe87fd746fab50aeaba4762861a259eed..ae8d7434374ddf39948c73044a734af417f61f05 100644 (file)
--- 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 d9cc68d59dc9b3bb1c3785a55fd3eccba4f58fb9..cb7d5efcf54889d678409f1b6043aecdfe22dc58 100644 (file)
--- 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);