"Reserve" the method store when constructing methods

Introducing the concept of reserving the store where a number of
provided operation methods are to be stored.

This avoids racing when constructing provided methods, which is
especially pertinent when multiple threads are trying to fetch the
same method, or even any implementation for the same given operation
type.

This introduces a |biglock| in OSSL_METHOD_STORE, which is separate
from the |lock| which is used for more internal and finer grained
locking.

Fixes #18152

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18153)

diff --git a/crypto/core_algorithm.c b/crypto/core_algorithm.c
index 6d1192f098d7964b71af80c684525835374aa023..b685183ab826da5e5862dee35cad216484ea5337 100644 (file)
--- a/crypto/core_algorithm.c
+++ b/crypto/core_algorithm.c
@@ -18,8 +18,10 @@ struct algorithm_data_st {
     int operation_id;            /* May be zero for finding them all */
     int (*pre)(OSSL_PROVIDER *, int operation_id, int no_store, void *data,
                int *result);
+    int (*reserve_store)(int no_store, void *data);
     void (*fn)(OSSL_PROVIDER *, const OSSL_ALGORITHM *, int no_store,
                void *data);
+    int (*unreserve_store)(void *data);
     int (*post)(OSSL_PROVIDER *, int operation_id, int no_store, void *data,
                 int *result);
     void *data;
@@ -43,6 +45,10 @@ static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map,
     struct algorithm_data_st *data = cbdata;
     int ret = 0;
 
+    if (!data->reserve_store(no_store, data->data))
+        /* Error, bail out! */
+        return -1;
+
     /* Do we fulfill pre-conditions? */
     if (data->pre == NULL) {
         /* If there is no pre-condition function, assume "yes" */
@@ -50,7 +56,8 @@ static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map,
     } else if (!data->pre(provider, cur_operation, no_store, data->data,
                           &ret)) {
         /* Error, bail out! */
-        return -1;
+        ret = -1;
+        goto end;
     }
 
     /*
@@ -58,8 +65,10 @@ static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map,
      * but do continue with the next.  This simply means that another thread
      * got to it first.
      */
-    if (ret == 0)
-        return 1;
+    if (ret == 0) {
+        ret = 1;
+        goto end;
+    }
 
     if (map != NULL) {
         const OSSL_ALGORITHM *thismap;
@@ -75,9 +84,12 @@ static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map,
     } else if (!data->post(provider, cur_operation, no_store, data->data,
                            &ret)) {
         /* Error, bail out! */
-        return -1;
+        ret = -1;
     }
 
+ end:
+    data->unreserve_store(data->data);
+
     return ret;
 }
 
@@ -103,7 +115,7 @@ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata)
          cur_operation++) {
         int no_store = 0;        /* Assume caching is ok */
         const OSSL_ALGORITHM *map = NULL;
-        int ret;
+        int ret = 0;
 
         map = ossl_provider_query_operation(provider, cur_operation,
                                             &no_store);
@@ -126,9 +138,11 @@ void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id,
                            OSSL_PROVIDER *provider,
                            int (*pre)(OSSL_PROVIDER *, int operation_id,
                                       int no_store, void *data, int *result),
+                           int (*reserve_store)(int no_store, void *data),
                            void (*fn)(OSSL_PROVIDER *provider,
                                       const OSSL_ALGORITHM *algo,
                                       int no_store, void *data),
+                           int (*unreserve_store)(void *data),
                            int (*post)(OSSL_PROVIDER *, int operation_id,
                                        int no_store, void *data, int *result),
                            void *data)
@@ -138,7 +152,9 @@ void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id,
     cbdata.libctx = libctx;
     cbdata.operation_id = operation_id;
     cbdata.pre = pre;
+    cbdata.reserve_store = reserve_store;
     cbdata.fn = fn;
+    cbdata.unreserve_store = unreserve_store;
     cbdata.post = post;
     cbdata.data = data;
 

diff --git a/crypto/core_fetch.c b/crypto/core_fetch.c
index faa6ebdefd1784224fa6bfea4e3d1d4eb46a250d..d311158d7758909dbe147da6bf9b6532aeeb5995 100644 (file)
--- a/crypto/core_fetch.c
+++ b/crypto/core_fetch.c
@@ -31,6 +31,31 @@ static int is_temporary_method_store(int no_store, void *cbdata)
     return no_store && !data->force_store;
 }
 
+static int ossl_method_construct_reserve_store(int no_store, void *cbdata)
+{
+    struct construct_data_st *data = cbdata;
+
+    if (is_temporary_method_store(no_store, data) && data->store == NULL) {
+        /*
+         * If we have been told not to store the method "permanently", we
+         * ask for a temporary store, and store the method there.
+         * The owner of |data->mcm| is completely responsible for managing
+         * that temporary store.
+         */
+        if ((data->store = data->mcm->get_tmp_store(data->mcm_data)) == NULL)
+            return 0;
+    }
+
+    return data->mcm->lock_store(data->store, data->mcm_data);
+}
+
+static int ossl_method_construct_unreserve_store(void *cbdata)
+{
+    struct construct_data_st *data = cbdata;
+
+    return data->mcm->unlock_store(data->store, data->mcm_data);
+}
+
 static int ossl_method_construct_precondition(OSSL_PROVIDER *provider,
                                               int operation_id, int no_store,
                                               void *cbdata, int *result)
@@ -95,24 +120,8 @@ static void ossl_method_construct_this(OSSL_PROVIDER *provider,
      * It is *expected* that the put function increments the refcnt
      * of the passed method.
      */
-
-    if (!is_temporary_method_store(no_store, data)) {
-        /* If we haven't been told not to store, add to the global store */
-        data->mcm->put(NULL, method, provider, algo->algorithm_names,
-                       algo->property_definition, data->mcm_data);
-    } else {
-        /*
-         * If we have been told not to store the method "permanently", we
-         * ask for a temporary store, and store the method there.
-         * The owner of |data->mcm| is completely responsible for managing
-         * that temporary store.
-         */
-        if ((data->store = data->mcm->get_tmp_store(data->mcm_data)) == NULL)
-            return;
-
-        data->mcm->put(data->store, method, provider, algo->algorithm_names,
-                       algo->property_definition, data->mcm_data);
-    }
+    data->mcm->put(data->store, method, provider, algo->algorithm_names,
+                   algo->property_definition, data->mcm_data);
 
     /* refcnt-- because we're dropping the reference */
     data->mcm->destruct(method, data->mcm_data);
@@ -143,7 +152,9 @@ void *ossl_method_construct(OSSL_LIB_CTX *libctx, int operation_id,
     cbdata.mcm_data = mcm_data;
     ossl_algorithm_do_all(libctx, operation_id, provider,
                           ossl_method_construct_precondition,
+                          ossl_method_construct_reserve_store,
                           ossl_method_construct_this,
+                          ossl_method_construct_unreserve_store,
                           ossl_method_construct_postcondition,
                           &cbdata);
 

diff --git a/crypto/encode_decode/decoder_meth.c b/crypto/encode_decode/decoder_meth.c
index 11e94dbcc45f75733c83572d59a30b5b3ff89975..74c86a8fe85dc61c253a4749824cf2593bf4bfc9 100644 (file)
--- a/crypto/encode_decode/decoder_meth.c
+++ b/crypto/encode_decode/decoder_meth.c
@@ -105,6 +105,28 @@ static OSSL_METHOD_STORE *get_decoder_store(OSSL_LIB_CTX *libctx)
     return ossl_lib_ctx_get_data(libctx, OSSL_LIB_CTX_DECODER_STORE_INDEX);
 }
 
+static int reserve_decoder_store(void *store, void *data)
+{
+    struct decoder_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_decoder_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_lock_store(store);
+}
+
+static int unreserve_decoder_store(void *store, void *data)
+{
+    struct decoder_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_decoder_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_unlock_store(store);
+}
+
 /* Get decoder methods from a store, or put one in */
 static void *get_decoder_from_store(void *store, const OSSL_PROVIDER **prov,
                                     void *data)
@@ -344,6 +366,8 @@ inner_ossl_decoder_fetch(struct decoder_data_st *methdata,
         || !ossl_method_store_cache_get(store, NULL, id, propq, &method)) {
         OSSL_METHOD_CONSTRUCT_METHOD mcm = {
             get_tmp_decoder_store,
+            reserve_decoder_store,
+            unreserve_decoder_store,
             get_decoder_from_store,
             put_decoder_in_store,
             construct_decoder,

diff --git a/crypto/encode_decode/encoder_meth.c b/crypto/encode_decode/encoder_meth.c
index 7a28894b2ca55e309a514d98309985956aee46d4..7092ba7ef8e99a38d1c606c2fad171dc844822fc 100644 (file)
--- a/crypto/encode_decode/encoder_meth.c
+++ b/crypto/encode_decode/encoder_meth.c
@@ -105,6 +105,28 @@ static OSSL_METHOD_STORE *get_encoder_store(OSSL_LIB_CTX *libctx)
     return ossl_lib_ctx_get_data(libctx, OSSL_LIB_CTX_ENCODER_STORE_INDEX);
 }
 
+static int reserve_encoder_store(void *store, void *data)
+{
+    struct encoder_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_encoder_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_lock_store(store);
+}
+
+static int unreserve_encoder_store(void *store, void *data)
+{
+    struct encoder_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_encoder_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_unlock_store(store);
+}
+
 /* Get encoder methods from a store, or put one in */
 static void *get_encoder_from_store(void *store, const OSSL_PROVIDER **prov,
                                     void *data)
@@ -354,6 +376,8 @@ inner_ossl_encoder_fetch(struct encoder_data_st *methdata,
         || !ossl_method_store_cache_get(store, NULL, id, propq, &method)) {
         OSSL_METHOD_CONSTRUCT_METHOD mcm = {
             get_tmp_encoder_store,
+            reserve_encoder_store,
+            unreserve_encoder_store,
             get_encoder_from_store,
             put_encoder_in_store,
             construct_encoder,

diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c
index 9af92bd412a10f6d5de617cedc36a9b6f97c8b6c..4908f6cfee8c7ce8ef37c976fb5a774bb24350cb 100644 (file)
--- a/crypto/evp/evp_fetch.c
+++ b/crypto/evp/evp_fetch.c
@@ -63,6 +63,28 @@ static OSSL_METHOD_STORE *get_evp_method_store(OSSL_LIB_CTX *libctx)
     return ossl_lib_ctx_get_data(libctx, OSSL_LIB_CTX_EVP_METHOD_STORE_INDEX);
 }
 
+static int reserve_evp_method_store(void *store, void *data)
+{
+    struct evp_method_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_evp_method_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_lock_store(store);
+}
+
+static int unreserve_evp_method_store(void *store, void *data)
+{
+    struct evp_method_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_evp_method_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_unlock_store(store);
+}
+
 /*
  * To identify the method in the EVP method store, we mix the name identity
  * with the operation identity, under the assumption that we don't have more
@@ -271,6 +293,8 @@ inner_evp_generic_fetch(struct evp_method_data_st *methdata,
         || !ossl_method_store_cache_get(store, prov, meth_id, propq, &method)) {
         OSSL_METHOD_CONSTRUCT_METHOD mcm = {
             get_tmp_evp_method_store,
+            reserve_evp_method_store,
+            unreserve_evp_method_store,
             get_evp_method_from_store,
             put_evp_method_in_store,
             construct_evp_method,

diff --git a/crypto/property/property.c b/crypto/property/property.c
index 496940c378be2255b9a29dd838d27cfa67cda751..672cc3560775a0261f648d81fd0a82b6aebdaf71 100644 (file)
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -63,7 +63,19 @@ typedef struct {
 struct ossl_method_store_st {
     OSSL_LIB_CTX *ctx;
     SPARSE_ARRAY_OF(ALGORITHM) *algs;
+    /*
+     * Lock to protect the |algs| array from concurrent writing, when
+     * individual implementations or queries are inserted.  This is used
+     * by the appropriate functions here.
+     */
     CRYPTO_RWLOCK *lock;
+    /*
+     * Lock to reserve the whole store.  This is used when fetching a set
+     * of algorithms, via these functions, found in crypto/core_fetch.c:
+     * ossl_method_construct_reserve_store()
+     * ossl_method_construct_unreserve_store()
+     */
+    CRYPTO_RWLOCK *biglock;
 
     /* query cache specific values */
 
@@ -230,13 +242,10 @@ OSSL_METHOD_STORE *ossl_method_store_new(OSSL_LIB_CTX *ctx)
     res = OPENSSL_zalloc(sizeof(*res));
     if (res != NULL) {
         res->ctx = ctx;
-        if ((res->algs = ossl_sa_ALGORITHM_new()) == NULL) {
-            OPENSSL_free(res);
-            return NULL;
-        }
-        if ((res->lock = CRYPTO_THREAD_lock_new()) == NULL) {
-            ossl_sa_ALGORITHM_free(res->algs);
-            OPENSSL_free(res);
+        if ((res->algs = ossl_sa_ALGORITHM_new()) == NULL
+            || (res->lock = CRYPTO_THREAD_lock_new()) == NULL
+            || (res->biglock = CRYPTO_THREAD_lock_new()) == NULL) {
+            ossl_method_store_free(res);
             return NULL;
         }
     }
@@ -246,13 +255,25 @@ OSSL_METHOD_STORE *ossl_method_store_new(OSSL_LIB_CTX *ctx)
 void ossl_method_store_free(OSSL_METHOD_STORE *store)
 {
     if (store != NULL) {
-        ossl_sa_ALGORITHM_doall_arg(store->algs, &alg_cleanup, store);
+        if (store->algs != NULL)
+            ossl_sa_ALGORITHM_doall_arg(store->algs, &alg_cleanup, store);
         ossl_sa_ALGORITHM_free(store->algs);
         CRYPTO_THREAD_lock_free(store->lock);
+        CRYPTO_THREAD_lock_free(store->biglock);
         OPENSSL_free(store);
     }
 }
 
+int ossl_method_lock_store(OSSL_METHOD_STORE *store)
+{
+    return store != NULL ? CRYPTO_THREAD_write_lock(store->biglock) : 0;
+}
+
+int ossl_method_unlock_store(OSSL_METHOD_STORE *store)
+{
+    return store != NULL ? CRYPTO_THREAD_unlock(store->biglock) : 0;
+}
+
 static ALGORITHM *ossl_method_store_retrieve(OSSL_METHOD_STORE *store, int nid)
 {
     return ossl_sa_ALGORITHM_get(store->algs, nid);
@@ -260,7 +281,7 @@ static ALGORITHM *ossl_method_store_retrieve(OSSL_METHOD_STORE *store, int nid)
 
 static int ossl_method_store_insert(OSSL_METHOD_STORE *store, ALGORITHM *alg)
 {
-        return ossl_sa_ALGORITHM_set(store->algs, alg->nid, alg);
+    return ossl_sa_ALGORITHM_set(store->algs, alg->nid, alg);
 }
 
 int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov,

diff --git a/crypto/store/store_meth.c b/crypto/store/store_meth.c
index 6f21d8f98f1917b06049f2ed3b2a8edc46b4e8e4..ab1016853e8b9285536a487749114277c263077c 100644 (file)
--- a/crypto/store/store_meth.c
+++ b/crypto/store/store_meth.c
@@ -108,6 +108,28 @@ static OSSL_METHOD_STORE *get_loader_store(OSSL_LIB_CTX *libctx)
     return ossl_lib_ctx_get_data(libctx, OSSL_LIB_CTX_STORE_LOADER_STORE_INDEX);
 }
 
+static int reserve_loader_store(void *store, void *data)
+{
+    struct loader_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_loader_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_lock_store(store);
+}
+
+static int unreserve_loader_store(void *store, void *data)
+{
+    struct loader_data_st *methdata = data;
+
+    if (store == NULL
+        && (store = get_loader_store(methdata->libctx)) == NULL)
+        return 0;
+
+    return ossl_method_unlock_store(store);
+}
+
 /* Get loader methods from a store, or put one in */
 static void *get_loader_from_store(void *store, const OSSL_PROVIDER **prov,
                                    void *data)
@@ -283,6 +305,8 @@ inner_loader_fetch(struct loader_data_st *methdata,
         || !ossl_method_store_cache_get(store, NULL, id, propq, &method)) {
         OSSL_METHOD_CONSTRUCT_METHOD mcm = {
             get_tmp_loader_store,
+            reserve_loader_store,
+            unreserve_loader_store,
             get_loader_from_store,
             put_loader_in_store,
             construct_loader,

diff --git a/include/internal/core.h b/include/internal/core.h
index 48e1ba465a694174548e71375ef941e920ff35a6..03adb66bd342f9f6de3f0e729076700a55bfef98 100644 (file)
--- a/include/internal/core.h
+++ b/include/internal/core.h
@@ -30,6 +30,10 @@
 typedef struct ossl_method_construct_method_st {
     /* Get a temporary store */
     void *(*get_tmp_store)(void *data);
+    /* Reserve the appropriate method store */
+    int (*lock_store)(void *store, void *data);
+    /* Unreserve the appropriate method store */
+    int (*unlock_store)(void *store, void *data);
     /* Get an already existing method from a store */
     void *(*get)(void *store, const OSSL_PROVIDER **prov, void *data);
     /* Store a method in a store */
@@ -50,9 +54,11 @@ void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id,
                            OSSL_PROVIDER *provider,
                            int (*pre)(OSSL_PROVIDER *, int operation_id,
                                       int no_store, void *data, int *result),
+                           int (*reserve_store)(int no_store, void *data),
                            void (*fn)(OSSL_PROVIDER *provider,
                                       const OSSL_ALGORITHM *algo,
                                       int no_store, void *data),
+                           int (*unreserve_store)(void *data),
                            int (*post)(OSSL_PROVIDER *, int operation_id,
                                        int no_store, void *data, int *result),
                            void *data);

diff --git a/include/internal/property.h b/include/internal/property.h
index db08c33cc37e0e68c05c32f8f3ef521fba811899..3adff4994003e83a80e24ba3b046298c07ba61cf 100644 (file)
--- a/include/internal/property.h
+++ b/include/internal/property.h
@@ -52,6 +52,10 @@ int64_t ossl_property_get_number_value(const OSSL_PROPERTY_DEFINITION *prop);
 /* Implementation store functions */
 OSSL_METHOD_STORE *ossl_method_store_new(OSSL_LIB_CTX *ctx);
 void ossl_method_store_free(OSSL_METHOD_STORE *store);
+
+int ossl_method_lock_store(OSSL_METHOD_STORE *store);
+int ossl_method_unlock_store(OSSL_METHOD_STORE *store);
+
 int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov,
                           int nid, const char *properties, void *method,
                           int (*method_up_ref)(void *),