Don't empty the method store when flushing the query cache

When evp_method_store_flush() flushed the query cache, it also freed all methods in the EVP method store, through an unfortunate call of ossl_method_store_flush_cache() with an argument saying that all methods should indeed be dropped. To undo some of the confusion, ossl_method_store_flush_cache() is renamed to ossl_method_store_cache_flush_all(), and limited to do only that. Some if the items in the internal ALGORITHM structure are also renamed and commented to clarify what they are for. Fixes #18150 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #18151)
openssl · May 5, 2022 · 60640d7 · 60640d7
1 parent 10937d5
commit 60640d7
Show file tree

Hide file tree

Showing 6 changed files with 33 additions and 37 deletions.
diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c
@@ -409,12 +409,12 @@ void *evp_generic_fetch_from_prov(OSSL_PROVIDER *prov, int operation_id,
     return method;
 }
 
-int evp_method_store_flush(OSSL_LIB_CTX *libctx)
+int evp_method_store_cache_flush(OSSL_LIB_CTX *libctx)
 {
     OSSL_METHOD_STORE *store = get_evp_method_store(libctx);
 
     if (store != NULL)
-        return ossl_method_store_flush_cache(store, 1);
+        return ossl_method_store_cache_flush_all(store);
     return 1;
 }
 
@@ -461,7 +461,7 @@ static int evp_set_parsed_default_properties(OSSL_LIB_CTX *libctx,
         ossl_property_free(*plp);
         *plp = def_prop;
         if (store != NULL)
-            return ossl_method_store_flush_cache(store, 0);
+            return ossl_method_store_cache_flush_all(store);
     }
     ERR_raise(ERR_LIB_EVP, ERR_R_INTERNAL_ERROR);
     return 0;

diff --git a/crypto/property/property.c b/crypto/property/property.c
@@ -62,10 +62,16 @@ typedef struct {
 
 struct ossl_method_store_st {
     OSSL_LIB_CTX *ctx;
-    size_t nelem;
     SPARSE_ARRAY_OF(ALGORITHM) *algs;
-    int need_flush;
     CRYPTO_RWLOCK *lock;
+
+    /* query cache specific values */
+
+    /* Count of the query cache entries for all algs */
+    size_t cache_nelem;
+
+    /* Flag: 1 if query cache entries for all algs need flushing */
+    int cache_need_flush;
 };
 
 typedef struct {
@@ -478,39 +484,28 @@ int ossl_method_store_fetch(OSSL_METHOD_STORE *store,
     return ret;
 }
 
-static void impl_cache_flush_alg(ossl_uintmax_t idx, ALGORITHM *alg, void *arg)
+static void impl_cache_flush_alg(ossl_uintmax_t idx, ALGORITHM *alg)
 {
-    SPARSE_ARRAY_OF(ALGORITHM) *algs = arg;
-
     lh_QUERY_doall(alg->cache, &impl_cache_free);
-    if (algs != NULL) {
-        sk_IMPLEMENTATION_pop_free(alg->impls, &impl_free);
-        lh_QUERY_free(alg->cache);
-        OPENSSL_free(alg);
-        ossl_sa_ALGORITHM_set(algs, idx, NULL);
-    } else {
-        lh_QUERY_flush(alg->cache);
-    }
+    lh_QUERY_flush(alg->cache);
 }
 
 static void ossl_method_cache_flush(OSSL_METHOD_STORE *store, int nid)
 {
     ALGORITHM *alg = ossl_method_store_retrieve(store, nid);
 
     if (alg != NULL) {
-        store->nelem -= lh_QUERY_num_items(alg->cache);
-        impl_cache_flush_alg(0, alg, NULL);
+        store->cache_nelem -= lh_QUERY_num_items(alg->cache);
+        impl_cache_flush_alg(0, alg);
     }
 }
 
-int ossl_method_store_flush_cache(OSSL_METHOD_STORE *store, int all)
+int ossl_method_store_cache_flush_all(OSSL_METHOD_STORE *store)
 {
-    void *arg = (all != 0 ? store->algs : NULL);
-
     if (!ossl_property_write_lock(store))
         return 0;
-    ossl_sa_ALGORITHM_doall_arg(store->algs, &impl_cache_flush_alg, arg);
-    store->nelem = 0;
+    ossl_sa_ALGORITHM_doall(store->algs, &impl_cache_flush_alg);
+    store->cache_nelem = 0;
     ossl_property_unlock(store);
     return 1;
 }
@@ -573,9 +568,9 @@ static void ossl_method_cache_flush_some(OSSL_METHOD_STORE *store)
     state.nelem = 0;
     if ((state.seed = OPENSSL_rdtsc()) == 0)
         state.seed = 1;
-    store->need_flush = 0;
+    store->cache_need_flush = 0;
     ossl_sa_ALGORITHM_doall_arg(store->algs, &impl_cache_flush_one_alg, &state);
-    store->nelem = state.nelem;
+    store->cache_nelem = state.nelem;
 }
 
 int ossl_method_store_cache_get(OSSL_METHOD_STORE *store, OSSL_PROVIDER *prov,
@@ -626,7 +621,7 @@ int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, OSSL_PROVIDER *prov,
 
     if (!ossl_property_write_lock(store))
         return 0;
-    if (store->need_flush)
+    if (store->cache_need_flush)
         ossl_method_cache_flush_some(store);
     alg = ossl_method_store_retrieve(store, nid);
     if (alg == NULL)
@@ -637,7 +632,7 @@ int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, OSSL_PROVIDER *prov,
         elem.provider = prov;
         if ((old = lh_QUERY_delete(alg->cache, &elem)) != NULL) {
             impl_cache_free(old);
-            store->nelem--;
+            store->cache_nelem--;
         }
         goto end;
     }
@@ -656,8 +651,8 @@ int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, OSSL_PROVIDER *prov,
             goto end;
         }
         if (!lh_QUERY_error(alg->cache)) {
-            if (++store->nelem >= IMPL_CACHE_FLUSH_THRESHOLD)
-                store->need_flush = 1;
+            if (++store->cache_nelem >= IMPL_CACHE_FLUSH_THRESHOLD)
+                store->cache_need_flush = 1;
             goto end;
         }
         ossl_method_free(&p->method);

diff --git a/crypto/provider_core.c b/crypto/provider_core.c
@@ -15,7 +15,7 @@
 #include <openssl/params.h>
 #include <openssl/opensslv.h>
 #include "crypto/cryptlib.h"
-#include "crypto/evp.h" /* evp_method_store_flush */
+#include "crypto/evp.h" /* evp_method_store_cache_flush */
 #include "crypto/rand.h"
 #include "internal/nelem.h"
 #include "internal/thread_once.h"
@@ -1152,7 +1152,7 @@ static int provider_flush_store_cache(const OSSL_PROVIDER *prov)
     CRYPTO_THREAD_unlock(store->lock);
 
     if (!freeing)
-        return evp_method_store_flush(prov->libctx);
+        return evp_method_store_cache_flush(prov->libctx);
     return 1;
 }
 

diff --git a/doc/internal/man3/OSSL_METHOD_STORE.pod b/doc/internal/man3/OSSL_METHOD_STORE.pod
@@ -34,7 +34,7 @@ ossl_method_store_flush_cache
                                  int nid, const char *prop_query, void *method,
                                  int (*method_up_ref)(void *),
                                  void (*method_destruct)(void *));
- void ossl_method_store_flush_cache(OSSL_METHOD_STORE *store, int all);
+ void ossl_method_store_cache_flush_all(OSSL_METHOD_STORE *store);
 
 =head1 DESCRIPTION
 
@@ -83,9 +83,6 @@ I<*prop> may be a pointer to a provider, which will narrow the search
 to methods from that provider.
 The result, if any, is returned in I<*method>, and its provider in I<*prov>.
 
-ossl_method_store_flush_cache() flushes all cached entries associated with
-I<store>.
-
 =head2 Cache Functions
 
 ossl_method_store_cache_get() queries the cache associated with the I<store>
@@ -102,6 +99,9 @@ The I<method_up_ref> function is called to increment the
 reference count of the method and the I<method_destruct> function is called
 to decrement it.
 
+ossl_method_store_cache_flush_all() flushes all cached entries associated with
+I<store>.
+
 =head1 NOTES
 
 The I<prop_query> argument to ossl_method_store_cache_get() and

diff --git a/include/crypto/evp.h b/include/crypto/evp.h
@@ -893,7 +893,8 @@ int evp_pkey_ctx_get1_id_len_prov(EVP_PKEY_CTX *ctx, size_t *id_len);
 int evp_pkey_ctx_use_cached_data(EVP_PKEY_CTX *ctx);
 # endif /* !defined(FIPS_MODULE) */
 
-int evp_method_store_flush(OSSL_LIB_CTX *libctx);
+int evp_method_store_cache_flush(OSSL_LIB_CTX *libctx);
+
 int evp_default_properties_enable_fips_int(OSSL_LIB_CTX *libctx, int enable,
                                            int loadconfig);
 int evp_set_default_properties_int(OSSL_LIB_CTX *libctx, const char *propq,

diff --git a/include/internal/property.h b/include/internal/property.h
@@ -77,7 +77,7 @@ int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, OSSL_PROVIDER *prov,
                                 int (*method_up_ref)(void *),
                                 void (*method_destruct)(void *));
 
-__owur int ossl_method_store_flush_cache(OSSL_METHOD_STORE *store, int all);
+__owur int ossl_method_store_cache_flush_all(OSSL_METHOD_STORE *store);
 
 /* Merge two property queries together */
 OSSL_PROPERTY_LIST *ossl_property_merge(const OSSL_PROPERTY_LIST *a,