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)

diff --git a/crypto/provider.c b/crypto/provider.c
index 5aec157c1f14cffdf2b4b736a55d39f34eefc347..12336acc57070ea8103cdeb1e59046f9a15a7d2b 100644 (file)
--- 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 7ab161b795c20527153da726e92350d740afa0ff..e808eafe24d1a69487b2ad30d53e720f858fa35f 100644 (file)
--- 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 d53e1be2dc426e0a776024c908f8efb0cfbd6f01..8e83264dc6adf12bf563fc1722b62335b7f80339 100644 (file)
--- 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 78a4b7f2cab1d3a93fc2e9172ed67a500e9357c0..393aa006ca47116d4ad04956ed9a0e0c6d0e207f 100644 (file)
--- 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 6432f8a2125f0c6d96396d5e4733b1806f9fef50..127ea2d2c576b03d25fb4aa6a93b8e63eaf7cd0c 100644 (file)
--- 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);