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 <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15854)

5 files changed, 51 insertions, 27 deletions

diff --git a/crypto/provider.c b/crypto/provider.c
index 5aec157c1f..12336acc57 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 7ab161b795..e808eafe24 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 d53e1be2dc..8e83264dc6 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 78a4b7f2ca..393aa006ca 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 6432f8a212..127ea2d2c5 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);