[openssl] master update
Dr. Paul Dale
pauli at openssl.org
Mon Nov 18 08:52:14 UTC 2019
The branch master has been updated
via bdbf2df2e685ae653f3c683ce2f734eb0c0888e0 (commit)
from f75abcc0f073b1c3e2d81df3fcde8fe45dd1e61f (commit)
- Log -----------------------------------------------------------------
commit bdbf2df2e685ae653f3c683ce2f734eb0c0888e0
Author: Pauli <paul.dale at oracle.com>
Date: Mon Nov 11 11:17:32 2019 +1000
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 at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10408)
-----------------------------------------------------------------------
Summary of changes:
crypto/evp/evp_fetch.c | 16 ++---
crypto/property/property.c | 116 ++++++++++++++++++++------------
doc/internal/man3/OSSL_METHOD_STORE.pod | 7 +-
include/internal/property.h | 4 +-
test/property_test.c | 21 ++++--
5 files changed, 104 insertions(+), 60 deletions(-)
diff --git a/crypto/evp/evp_fetch.c b/crypto/evp/evp_fetch.c
index 260471a88d..24a748c716 100644
--- 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 2089e21d8c..20d1a75856 100644
--- 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 2ffe820100..2768524e0c 100644
--- 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 a3a4a62be6..7ffd588073 100644
--- 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 8b953e41e2..ca407b2ba4 100644
--- 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;
}
More information about the openssl-commits
mailing list