Refactor method construction pre- and post-condition

The existing pre- and post-condition functions are supposed to check if
methods have already been created and stored, using provider operation
bits.  This is supposed to only be done for "permanent" method stores.

However, the way the pre-condition was called, it could not know if the
set of implementations to be stored is likely to end up in a "permanent"
or a temporary store.  It needs access to the |no_store| flag returned
by the provider's operation query function, because that call was done
after the pre-condition was called.

This requires a bit of refactoring, primarly of |algorithm_do_this()|,
but also of |ossl_method_construct_precondition()|.

Fixes #18150

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #18151)

crypto/core_algorithm.c

@@ -16,18 +16,80 @@
 struct algorithm_data_st {
     OSSL_LIB_CTX *libctx;
     int operation_id;            /* May be zero for finding them all */
-    int (*pre)(OSSL_PROVIDER *, int operation_id, void *data, int *result);
+    int (*pre)(OSSL_PROVIDER *, int operation_id, int no_store, void *data,
+               int *result);
     void (*fn)(OSSL_PROVIDER *, const OSSL_ALGORITHM *, int no_store,
                void *data);
     int (*post)(OSSL_PROVIDER *, int operation_id, int no_store, void *data,
                 int *result);
     void *data;
 };
 
+/*
+ * Process one OSSL_ALGORITHM array, for the operation |cur_operation|,
+ * by constructing methods for all its implementations and adding those
+ * to the appropriate method store.
+ * Which method store is appropriate is given by |no_store| ("permanent"
+ * if 0, temporary if 1) and other data in |data->data|.
+ *
+ * Returns:
+ * -1 to quit adding algorithm implementations immediately
+ * 0 if not successful, but adding should continue
+ * 1 if successful so far, and adding should continue
+ */
+static int algorithm_do_map(OSSL_PROVIDER *provider, const OSSL_ALGORITHM *map,
+                            int cur_operation, int no_store, void *cbdata)
+{
+    struct algorithm_data_st *data = cbdata;
+    int ret = 0;
+
+    /* Do we fulfill pre-conditions? */
+    if (data->pre == NULL) {
+        /* If there is no pre-condition function, assume "yes" */
+        ret = 1;
+    } else if (!data->pre(provider, cur_operation, no_store, data->data,
+                          &ret)) {
+        /* Error, bail out! */
+        return -1;
+    }
+
+    /*
+     * If pre-condition not fulfilled don't add this set of implementations,
+     * but do continue with the next.  This simply means that another thread
+     * got to it first.
+     */
+    if (ret == 0)
+        return 1;
+
+    if (map != NULL) {
+        const OSSL_ALGORITHM *thismap;
+
+        for (thismap = map; thismap->algorithm_names != NULL; thismap++)
+            data->fn(provider, thismap, no_store, data->data);
+    }
+
+    /* Do we fulfill post-conditions? */
+    if (data->post == NULL) {
+        /* If there is no post-condition function, assume "yes" */
+        ret = 1;
+    } else if (!data->post(provider, cur_operation, no_store, data->data,
+                           &ret)) {
+        /* Error, bail out! */
+        return -1;
+    }
+
+    return ret;
+}
+
+/*
+ * Given a provider, process one operation given by |data->operation_id|, or
+ * if that's zero, process all known operations.
+ * For each such operation, query the associated OSSL_ALGORITHM array from
+ * the provider, then process that array with |algorithm_do_map()|.
+ */
 static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata)
 {
     struct algorithm_data_st *data = cbdata;
-    int no_store = 0;    /* Assume caching is ok */
     int first_operation = 1;
     int last_operation = OSSL_OP__HIGHEST;
     int cur_operation;
@@ -39,43 +101,18 @@ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata)
     for (cur_operation = first_operation;
          cur_operation <= last_operation;
          cur_operation++) {
+        int no_store = 0;        /* Assume caching is ok */
         const OSSL_ALGORITHM *map = NULL;
         int ret;
 
-        /* Do we fulfill pre-conditions? */
-        if (data->pre == NULL) {
-            /* If there is no pre-condition function, assume "yes" */
-            ret = 1;
-        } else {
-            if (!data->pre(provider, cur_operation, data->data, &ret))
-                /* Error, bail out! */
-                return 0;
-        }
-
-        /* If pre-condition not fulfilled, go to the next operation */
-        if (!ret)
-            continue;
-
         map = ossl_provider_query_operation(provider, cur_operation,
                                             &no_store);
-        if (map != NULL) {
-            const OSSL_ALGORITHM *thismap;
-
-            for (thismap = map; thismap->algorithm_names != NULL; thismap++)
-                data->fn(provider, thismap, no_store, data->data);
-        }
+        ret = algorithm_do_map(provider, map, cur_operation, no_store, data);
         ossl_provider_unquery_operation(provider, cur_operation, map);
 
-        /* Do we fulfill post-conditions? */
-        if (data->post == NULL) {
-            /* If there is no post-condition function, assume "yes" */
-            ret = 1;
-        } else {
-            if (!data->post(provider, cur_operation, no_store, data->data,
-                            &ret))
-                /* Error, bail out! */
-                return 0;
-        }
+        if (ret < 0)
+            /* Hard error, bail out immediately! */
+            return 0;
 
         /* If post-condition not fulfilled, set general failure */
         if (!ret)
@@ -88,7 +125,7 @@ static int algorithm_do_this(OSSL_PROVIDER *provider, void *cbdata)
 void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id,
                            OSSL_PROVIDER *provider,
                            int (*pre)(OSSL_PROVIDER *, int operation_id,
-                                      void *data, int *result),
+                                      int no_store, void *data, int *result),
                            void (*fn)(OSSL_PROVIDER *provider,
                                       const OSSL_ALGORITHM *algo,
                                       int no_store, void *data),

crypto/core_fetch.c

@@ -24,16 +24,28 @@ struct construct_data_st {
     void *mcm_data;
 };
 
+static int is_temporary_method_store(int no_store, void *cbdata)
+{
+    struct construct_data_st *data = cbdata;
+
+    return no_store && !data->force_store;
+}
+
 static int ossl_method_construct_precondition(OSSL_PROVIDER *provider,
-                                              int operation_id, void *cbdata,
-                                              int *result)
+                                              int operation_id, int no_store,
+                                              void *cbdata, int *result)
 {
     if (!ossl_assert(result != NULL)) {
         ERR_raise(ERR_LIB_CRYPTO, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
     }
 
-    if (!ossl_provider_test_operation_bit(provider, operation_id, result))
+    /* Assume that no bits are set */
+    *result = 0;
+
+    /* No flag bits for temporary stores */
+    if (!is_temporary_method_store(no_store, cbdata)
+        && !ossl_provider_test_operation_bit(provider, operation_id, result))
         return 0;
 
     /*
@@ -56,7 +68,9 @@ static int ossl_method_construct_postcondition(OSSL_PROVIDER *provider,
     }
 
     *result = 1;
-    return no_store != 0
+
+    /* No flag bits for temporary stores */
+    return is_temporary_method_store(no_store, cbdata)
         || ossl_provider_set_operation_bit(provider, operation_id);
 }
 
@@ -82,7 +96,7 @@ static void ossl_method_construct_this(OSSL_PROVIDER *provider,
      * of the passed method.
      */
 
-    if (data->force_store || !no_store) {
+    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);

include/internal/core.h

@@ -49,7 +49,7 @@ void *ossl_method_construct(OSSL_LIB_CTX *ctx, int operation_id,
 void ossl_algorithm_do_all(OSSL_LIB_CTX *libctx, int operation_id,
                            OSSL_PROVIDER *provider,
                            int (*pre)(OSSL_PROVIDER *, int operation_id,
-                                      void *data, int *result),
+                                      int no_store, void *data, int *result),
                            void (*fn)(OSSL_PROVIDER *provider,
                                       const OSSL_ALGORITHM *algo,
                                       int no_store, void *data),