Properties: make query cache reference count aware.

The property query cache was not reference count aware and this could cause
problems if the property store removes an algorithm while it is being returned
from an asynchronous query.  This change makes the cache reference count aware
and avoids disappearing algorithms.

A side effect of this change is that the reference counts are now owned by the
cache and store.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10408)

diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c
index 260471a88d0e86a6749880c9acd87411d5a748cc..24a748c716027688b1d8130c3e2b02f84a25a0ee 100644 (file)
--- a/crypto/evp/evp_fetch.c
+++ b/crypto/evp/evp_fetch.c
@@ -186,13 +186,9 @@ static void *get_evp_method_from_store(OPENSSL_CTX *libctx, void *store,
         && (store = get_evp_method_store(libctx)) == NULL)
         return NULL;
 
-    (void)ossl_method_store_fetch(store, meth_id, methdata->propquery,
-                                  &method);
-
-    if (method != NULL
-        && !methdata->refcnt_up_method(method)) {
-        method = NULL;
-    }
+    if (!ossl_method_store_fetch(store, meth_id, methdata->propquery,
+                                 &method))
+        return NULL;
     return method;
 }
 
@@ -328,7 +324,6 @@ inner_evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id,
         mcmdata.names = name;
         mcmdata.propquery = properties;
         mcmdata.method_from_dispatch = new_method;
-        mcmdata.destruct_method = free_method;
         mcmdata.refcnt_up_method = up_ref_method;
         mcmdata.destruct_method = free_method;
         if ((method = ossl_method_construct(libctx, operation_id,
@@ -343,10 +338,9 @@ inner_evp_generic_fetch(OPENSSL_CTX *libctx, int operation_id,
             if (name_id == 0)
                 name_id = ossl_namemap_name2num(namemap, name);
             meth_id = evp_method_id(operation_id, name_id);
-            ossl_method_store_cache_set(store, meth_id, properties, method);
+            ossl_method_store_cache_set(store, meth_id, properties, method,
+                                        up_ref_method, free_method);
         }
-    } else {
-        up_ref_method(method);
     }
 
     return method;

diff --git a/crypto/property/property.c b/crypto/property/property.c
index 2089e21d8cf81209d9a3fec1f7f482cc160b1a4e..20d1a758560d56f37d0f0022da91c23e7e38322e 100644 (file)
--- a/crypto/property/property.c
+++ b/crypto/property/property.c
@@ -22,20 +22,25 @@
 #include "property_local.h"
 
 /* The number of elements in the query cache before we initiate a flush */
-#define IMPL_CACHE_FLUSH_THRESHOLD  500
+#define IMPL_CACHE_FLUSH_THRESHOLD  50
+
+typedef struct {
+    void *method;
+    int (*up_ref)(void *);
+    void (*free)(void *);
+} METHOD;
 
 typedef struct {
     const OSSL_PROVIDER *provider;
     OSSL_PROPERTY_LIST *properties;
-    void *method;
-    void (*method_destruct)(void *);
+    METHOD method;
 } IMPLEMENTATION;
 
 DEFINE_STACK_OF(IMPLEMENTATION)
 
 typedef struct {
     const char *query;
-    void *method;
+    METHOD method;
     char body[1];
 } QUERY;
 
@@ -67,6 +72,16 @@ DEFINE_SPARSE_ARRAY_OF(ALGORITHM);
 static void ossl_method_cache_flush(OSSL_METHOD_STORE *store, int nid);
 static void ossl_method_cache_flush_all(OSSL_METHOD_STORE *c);
 
+static int ossl_method_up_ref(METHOD *method)
+{
+    return (*method->up_ref)(method->method);
+}
+
+static void ossl_method_free(METHOD *method)
+{
+    (*method->free)(method->method);
+}
+
 int ossl_property_read_lock(OSSL_METHOD_STORE *p)
 {
     return p != NULL ? CRYPTO_THREAD_read_lock(p->lock) : 0;
@@ -95,15 +110,17 @@ static int query_cmp(const QUERY *a, const QUERY *b)
 static void impl_free(IMPLEMENTATION *impl)
 {
     if (impl != NULL) {
-        if (impl->method_destruct)
-            impl->method_destruct(impl->method);
+        ossl_method_free(&impl->method);
         OPENSSL_free(impl);
     }
 }
 
 static void impl_cache_free(QUERY *elem)
 {
-    OPENSSL_free(elem);
+    if (elem != NULL) {
+        ossl_method_free(&elem->method);
+        OPENSSL_free(elem);
+    }
 }
 
 static void alg_cleanup(ossl_uintmax_t idx, ALGORITHM *a)
@@ -132,7 +149,7 @@ OSSL_METHOD_STORE *ossl_method_store_new(OPENSSL_CTX *ctx)
             return NULL;
         }
         if ((res->lock = CRYPTO_THREAD_lock_new()) == NULL) {
-            OPENSSL_free(res->algs);
+            ossl_sa_ALGORITHM_free(res->algs);
             OPENSSL_free(res);
             return NULL;
         }
@@ -180,13 +197,14 @@ int ossl_method_store_add(OSSL_METHOD_STORE *store, const OSSL_PROVIDER *prov,
     impl = OPENSSL_malloc(sizeof(*impl));
     if (impl == NULL)
         return 0;
-    if (method_up_ref != NULL && !method_up_ref(method)) {
+    impl->method.method = method;
+    impl->method.up_ref = method_up_ref;
+    impl->method.free = method_destruct;
+    if (!ossl_method_up_ref(&impl->method)) {
         OPENSSL_free(impl);
         return 0;
     }
     impl->provider = prov;
-    impl->method = method;
-    impl->method_destruct = method_destruct;
 
     /*
      * Insert into the hash table if required.
@@ -262,10 +280,10 @@ int ossl_method_store_remove(OSSL_METHOD_STORE *store, int nid,
     for (i = 0; i < sk_IMPLEMENTATION_num(alg->impls); i++) {
         IMPLEMENTATION *impl = sk_IMPLEMENTATION_value(alg->impls, i);
 
-        if (impl->method == method) {
+        if (impl->method.method == method) {
+            impl_free(impl);
             sk_IMPLEMENTATION_delete(alg->impls, i);
             ossl_property_unlock(store);
-            impl_free(impl);
             return 1;
         }
     }
@@ -279,6 +297,7 @@ int ossl_method_store_fetch(OSSL_METHOD_STORE *store, int nid,
     ALGORITHM *alg;
     IMPLEMENTATION *impl;
     OSSL_PROPERTY_LIST *pq = NULL, *p2;
+    METHOD *best_method = NULL;
     int ret = 0;
     int j, best = -1, score, optional;
 
@@ -302,7 +321,7 @@ int ossl_method_store_fetch(OSSL_METHOD_STORE *store, int nid,
 
     if (prop_query == NULL) {
         if ((impl = sk_IMPLEMENTATION_value(alg->impls, 0)) != NULL) {
-            *method = impl->method;
+            best_method = &impl->method;
             ret = 1;
         }
         goto fin;
@@ -322,14 +341,18 @@ int ossl_method_store_fetch(OSSL_METHOD_STORE *store, int nid,
         impl = sk_IMPLEMENTATION_value(alg->impls, j);
         score = ossl_property_match_count(pq, impl->properties);
         if (score > best) {
-            *method = impl->method;
+            best_method = &impl->method;
+            best = score;
             ret = 1;
             if (!optional)
                 goto fin;
-            best = score;
         }
     }
 fin:
+    if (ret && ossl_method_up_ref(best_method))
+        *method = best_method->method;
+    else
+        ret = 0;
     ossl_property_unlock(store);
     ossl_property_free(pq);
     return ret;
@@ -414,7 +437,7 @@ static void impl_cache_flush_cache(QUERY *c, IMPL_CACHE_FLUSH *state)
     state->seed = n;
 
     if ((n & 1) != 0)
-        OPENSSL_free(lh_QUERY_delete(state->cache, c));
+        impl_cache_free(lh_QUERY_delete(state->cache, c));
     else
         state->nelem++;
 }
@@ -446,34 +469,38 @@ int ossl_method_store_cache_get(OSSL_METHOD_STORE *store, int nid,
 {
     ALGORITHM *alg;
     QUERY elem, *r;
+    int res = 0;
 
     if (nid <= 0 || store == NULL)
         return 0;
 
     ossl_property_read_lock(store);
     alg = ossl_method_store_retrieve(store, nid);
-    if (alg == NULL) {
-        ossl_property_unlock(store);
-        return 0;
-    }
+    if (alg == NULL)
+        goto err;
 
     elem.query = prop_query != NULL ? prop_query : "";
     r = lh_QUERY_retrieve(alg->cache, &elem);
-    if (r == NULL) {
-        ossl_property_unlock(store);
-        return 0;
+    if (r == NULL)
+        goto err;
+    if (ossl_method_up_ref(&r->method)) {
+        *method = r->method.method;
+        res = 1;
     }
-    *method = r->method;
+err:
     ossl_property_unlock(store);
-    return 1;
+    return res;
 }
 
 int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, int nid,
-                                const char *prop_query, void *method)
+                                const char *prop_query, void *method,
+                                int (*method_up_ref)(void *),
+                                void (*method_destruct)(void *))
 {
     QUERY elem, *old, *p = NULL;
     ALGORITHM *alg;
     size_t len;
+    int res = 1;
 
     if (nid <= 0 || store == NULL)
         return 0;
@@ -484,36 +511,41 @@ int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, int nid,
     if (store->need_flush)
         ossl_method_cache_flush_some(store);
     alg = ossl_method_store_retrieve(store, nid);
-    if (alg == NULL) {
-        ossl_property_unlock(store);
-        return 0;
-    }
+    if (alg == NULL)
+        goto err;
 
     if (method == NULL) {
         elem.query = prop_query;
-        if (lh_QUERY_delete(alg->cache, &elem) != NULL)
+        if ((old = lh_QUERY_delete(alg->cache, &elem)) != NULL) {
+            impl_cache_free(old);
             store->nelem--;
-        ossl_property_unlock(store);
-        return 1;
+        }
+        goto end;
     }
     p = OPENSSL_malloc(sizeof(*p) + (len = strlen(prop_query)));
     if (p != NULL) {
         p->query = p->body;
-        p->method = method;
+        p->method.method = method;
+        p->method.up_ref = method_up_ref;
+        p->method.free = method_destruct;
+        if (!ossl_method_up_ref(&p->method))
+            goto err;
         memcpy((char *)p->query, prop_query, len + 1);
         if ((old = lh_QUERY_insert(alg->cache, p)) != NULL) {
-            OPENSSL_free(old);
-            ossl_property_unlock(store);
-            return 1;
+            impl_cache_free(old);
+            goto end;
         }
         if (!lh_QUERY_error(alg->cache)) {
             if (++store->nelem >= IMPL_CACHE_FLUSH_THRESHOLD)
                 store->need_flush = 1;
-            ossl_property_unlock(store);
-            return 1;
+            goto end;
         }
+        ossl_method_free(&p->method);
     }
-    ossl_property_unlock(store);
     OPENSSL_free(p);
-    return 0;
+err:
+    res = 0;
+end:
+    ossl_property_unlock(store);
+    return res;
 }

diff --git a/doc/internal/man3/OSSL_METHOD_STORE.pod b/doc/internal/man3/OSSL_METHOD_STORE.pod
index 2ffe820100c69207db3adfa058374378462da068..2768524e0c5b6d15560dcf4f5b378197003b060e 100644 (file)
--- a/doc/internal/man3/OSSL_METHOD_STORE.pod
+++ b/doc/internal/man3/OSSL_METHOD_STORE.pod
@@ -33,7 +33,9 @@ ossl_method_store_cache_get, ossl_method_store_cache_set
  int ossl_method_store_cache_get(OSSL_METHOD_STORE *store, int nid,
                                  const char *prop_query, void **method);
  int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, int nid,
-                                 const char *prop_query, void *method);
+                                 const char *prop_query, void *method,
+                                 int (*method_up_ref)(void *),
+                                 void (*method_destruct)(void *));
 
 =head1 DESCRIPTION
 
@@ -95,6 +97,9 @@ The result, if any, is returned in I<method>.
 ossl_method_store_cache_set() sets a cache entry identified by I<nid> with the
 property query I<prop_query> in the I<store>.
 Future calls to ossl_method_store_cache_get() will return the specified I<method>.
+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.
 
 =head1 RETURN VALUES
 

diff --git a/include/internal/property.h b/include/internal/property.h
index a3a4a62be6a24b2876b1f99236d53cf9fa0cb8ec..7ffd588073fa1b68c4598cb989c1bf584076340d 100644 (file)
--- a/include/internal/property.h
+++ b/include/internal/property.h
@@ -36,5 +36,7 @@ int ossl_method_store_set_global_properties(OSSL_METHOD_STORE *store,
 int ossl_method_store_cache_get(OSSL_METHOD_STORE *store, int nid,
                                 const char *prop_query, void **result);
 int ossl_method_store_cache_set(OSSL_METHOD_STORE *store, int nid,
-                                const char *prop_query, void *result);
+                                const char *prop_query, void *result,
+                                int (*method_up_ref)(void *),
+                                void (*method_destruct)(void *));
 #endif

diff --git a/test/property_test.c b/test/property_test.c
index 8b953e41e259840bf46a2c0450b831a9f509c459..ca407b2ba43a6d5786a92ad650e2c5d3d636659f 100644 (file)
--- a/test/property_test.c
+++ b/test/property_test.c
@@ -28,6 +28,15 @@ static int add_property_names(const char *n, ...)
     return res;
 }
 
+static int up_ref(void *p)
+{
+    return 1;
+}
+
+static void down_ref(void *p)
+{
+}
+
 static int test_property_string(void)
 {
     OSSL_METHOD_STORE *store;
@@ -242,7 +251,7 @@ static int test_register_deregister(void)
     for (i = 0; i < OSSL_NELEM(impls); i++)
         if (!TEST_true(ossl_method_store_add(store, NULL, impls[i].nid,
                                              impls[i].prop, impls[i].impl,
-                                             NULL, NULL))) {
+                                             &up_ref, &down_ref))) {
             TEST_note("iteration %zd", i + 1);
             goto err;
         }
@@ -310,7 +319,7 @@ static int test_property(void)
     for (i = 0; i < OSSL_NELEM(impls); i++)
         if (!TEST_true(ossl_method_store_add(store, NULL, impls[i].nid,
                                              impls[i].prop, impls[i].impl,
-                                             NULL, NULL))) {
+                                             &up_ref, &down_ref))) {
             TEST_note("iteration %zd", i + 1);
             goto err;
         }
@@ -350,10 +359,12 @@ static int test_query_cache_stochastic(void)
         v[i] = 2 * i;
         BIO_snprintf(buf, sizeof(buf), "n=%d\n", i);
         if (!TEST_true(ossl_method_store_add(store, NULL, i, buf, "abc",
-                                             NULL, NULL))
-                || !TEST_true(ossl_method_store_cache_set(store, i, buf, v + i))
+                                             &up_ref, &down_ref))
+                || !TEST_true(ossl_method_store_cache_set(store, i, buf, v + i,
+                                                          &up_ref, &down_ref))
                 || !TEST_true(ossl_method_store_cache_set(store, i, "n=1234",
-                                                          "miss"))) {
+                                                          "miss", &up_ref,
+                                                          &down_ref))) {
             TEST_note("iteration %d", i);
             goto err;
         }