[openssl] openssl-3.0 update
Matt Caswell
matt at openssl.org
Mon Nov 15 14:35:08 UTC 2021
The branch openssl-3.0 has been updated
via f6c418af502e11c88a3ecb6db1816ce27de0210a (commit)
via cff311bff1b652194c8979c685f3efd66e147661 (commit)
via 256ec496890da13c700d5a21b069d519fe02c0b2 (commit)
via 5ab06d33e6b3be15c8784f1e1fd6c3d845b5cc3a (commit)
via fc205cedd7b9f12a70117c5f9167ba689ec35a28 (commit)
via 0d46c3f8e2453b407f8385545437e65c13a2fd8f (commit)
via 2c1b9371447f823008856d847784287fe5e7d3ff (commit)
via 2f5772557ae7ab1c667d760281c802371aba5e35 (commit)
via 56d00c0e4882f28dc008c5841a7284f55f1c41dc (commit)
via d740c9d59bcff6879bc7d6ecf8b82c9300991b50 (commit)
via 4f224595162b7996bfb673457ea9d37f11478793 (commit)
from cf9a84a12dc4f3b314347e44f5c51b473a504926 (commit)
- Log -----------------------------------------------------------------
commit f6c418af502e11c88a3ecb6db1816ce27de0210a
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 18:31:24 2021 +0000
Extend the test_multi_load() test
Run more threads and load the legacy provider (which uses a child lib ctx)
in order to hit more possible thread failures.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit cff311bff1b652194c8979c685f3efd66e147661
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 16:23:34 2021 +0000
Hold the flag_lock when calling child callbacks
Not holding the flag lock when creating/removing child providers can
confuse the activation counts if the parent provider is loaded/unloaded
at the same time.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 256ec496890da13c700d5a21b069d519fe02c0b2
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 14:32:14 2021 +0000
Use a write lock during ossl_provider_find()
A "find" operation on a stack can end up sorting the underlying stack. In
this case it is necessary to use a "write" lock to synchronise access to
the stack across multiple threads.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 5ab06d33e6b3be15c8784f1e1fd6c3d845b5cc3a
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 14:20:31 2021 +0000
Correctly activate the provider in OSSL_PROVIDER_try_load
If during OSSL_PROVIDER_try_load() we attempt to load a provider, but
adding to the store gives back a different provider, then we need to
ensure this different provider has its activation count increased.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit fc205cedd7b9f12a70117c5f9167ba689ec35a28
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 13:48:31 2021 +0000
Stop receiving child callbacks in a child libctx when appropriate
We should stop receiving child callbacks if we're about to free up
the child libctx. Otherwise we can get callbacks when the libctx is half
freed up.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 0d46c3f8e2453b407f8385545437e65c13a2fd8f
Author: Matt Caswell <matt at openssl.org>
Date: Tue Nov 9 11:53:27 2021 +0000
Don't bail out during provider deactivation if we don't have store
A provider may have been activated, but failed when being added to
the store. At this point we still need to deactivate it.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 2c1b9371447f823008856d847784287fe5e7d3ff
Author: Matt Caswell <matt at openssl.org>
Date: Mon Nov 8 16:47:38 2021 +0000
Don't try and do ossl_provider_find in ossl_provider_new
We leave it to the caller to confirm that the provider does not exist
in the store. If it does exist then later adding it to the store will
fail.
It is possible that the provider could be added to the store in
between the caller checking, and the caller calling ossl_provider_new.
We leave it to the caller to properly handle the failure when it
attempts to add the provider to the store. This is simpler than
having ossl_provider_new try to handle it.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 2f5772557ae7ab1c667d760281c802371aba5e35
Author: Matt Caswell <matt at openssl.org>
Date: Mon Nov 8 16:30:43 2021 +0000
Remove the isinited variable from child_prov_globals
This variable might have made sense at some point but it not longer does
so. It was being used to check whether we are still initing or not. If we
are still initing then the assumption was that we already hold the lock.
That assumption was untrue. We need to always take the lock.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 56d00c0e4882f28dc008c5841a7284f55f1c41dc
Author: Matt Caswell <matt at openssl.org>
Date: Fri Nov 5 14:43:01 2021 +0000
Avoid a race in init_thread_stop()
init_thread_stop() is called when a thread is stopping. It calls all
the callbacks that need to know about the demise of this thread. However,
the list of callbacks is also available globally and may be updated by
other threads so we need to make sure we use the right lock.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit d740c9d59bcff6879bc7d6ecf8b82c9300991b50
Author: Matt Caswell <matt at openssl.org>
Date: Fri Nov 5 13:42:40 2021 +0000
Don't attempt to deactive child providers if we don't need to
If a provider doesn't have any child providers then there is no need
to attempt to remove them - so we should not do so. This removes some
potentialy thread races.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
commit 4f224595162b7996bfb673457ea9d37f11478793
Author: Matt Caswell <matt at openssl.org>
Date: Fri Nov 5 13:29:41 2021 +0000
Don't write to the globals ossl_property_true and ossl_property_false
These global variables were previously overwritten with the same value
every time we created a new OSSL_LIB_CTX. Instead we preinitialise them
with the correct values, and then confirm that settings for each
OSSL_LIB_CTX agree with the preinitialised values.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17018)
-----------------------------------------------------------------------
Summary of changes:
crypto/context.c | 4 ++
crypto/initthread.c | 15 ++++++
crypto/property/property_local.h | 3 +-
crypto/property/property_parse.c | 20 ++++----
crypto/property/property_query.c | 4 +-
crypto/provider.c | 10 +++-
crypto/provider_child.c | 28 ++++++-----
crypto/provider_conf.c | 17 +++++--
crypto/provider_core.c | 88 +++++++++++++++++++--------------
doc/internal/man3/ossl_provider_new.pod | 14 ++++--
doc/man3/DEFINE_STACK_OF.pod | 5 +-
include/internal/provider.h | 3 +-
test/provider_internal_test.c | 2 +-
test/threadstest.c | 25 ++++++++--
14 files changed, 159 insertions(+), 79 deletions(-)
diff --git a/crypto/context.c b/crypto/context.c
index 1e0dfa8e01..bba8e4208b 100644
--- a/crypto/context.c
+++ b/crypto/context.c
@@ -240,6 +240,10 @@ void OSSL_LIB_CTX_free(OSSL_LIB_CTX *ctx)
if (ossl_lib_ctx_is_default(ctx))
return;
+#ifndef FIPS_MODULE
+ if (ctx->ischild)
+ ossl_provider_deinit_child(ctx);
+#endif
context_deinit(ctx);
OPENSSL_free(ctx);
}
diff --git a/crypto/initthread.c b/crypto/initthread.c
index d86e280fc1..1bdaeda9fc 100644
--- a/crypto/initthread.c
+++ b/crypto/initthread.c
@@ -309,11 +309,23 @@ void ossl_ctx_thread_stop(OSSL_LIB_CTX *ctx)
static void init_thread_stop(void *arg, THREAD_EVENT_HANDLER **hands)
{
THREAD_EVENT_HANDLER *curr, *prev = NULL, *tmp;
+#ifndef FIPS_MODULE
+ GLOBAL_TEVENT_REGISTER *gtr;
+#endif
/* Can't do much about this */
if (hands == NULL)
return;
+#ifndef FIPS_MODULE
+ gtr = get_global_tevent_register();
+ if (gtr == NULL)
+ return;
+
+ if (!CRYPTO_THREAD_write_lock(gtr->lock))
+ return;
+#endif
+
curr = *hands;
while (curr != NULL) {
if (arg != NULL && curr->arg != arg) {
@@ -332,6 +344,9 @@ static void init_thread_stop(void *arg, THREAD_EVENT_HANDLER **hands)
OPENSSL_free(tmp);
}
+#ifndef FIPS_MODULE
+ CRYPTO_THREAD_unlock(gtr->lock);
+#endif
}
int ossl_init_thread_start(const void *index, void *arg,
diff --git a/crypto/property/property_local.h b/crypto/property/property_local.h
index 46c5dbe3cc..6b85ce1586 100644
--- a/crypto/property/property_local.h
+++ b/crypto/property/property_local.h
@@ -34,7 +34,8 @@ struct ossl_property_list_st {
OSSL_PROPERTY_DEFINITION properties[1];
};
-extern OSSL_PROPERTY_IDX ossl_property_true, ossl_property_false;
+#define OSSL_PROPERTY_TRUE 1
+#define OSSL_PROPERTY_FALSE 2
/* Property string functions */
OSSL_PROPERTY_IDX ossl_property_name(OSSL_LIB_CTX *ctx, const char *s,
diff --git a/crypto/property/property_parse.c b/crypto/property/property_parse.c
index 3673fd7b05..8954ec7246 100644
--- a/crypto/property/property_parse.c
+++ b/crypto/property/property_parse.c
@@ -19,8 +19,6 @@
#include "property_local.h"
#include "e_os.h"
-OSSL_PROPERTY_IDX ossl_property_true, ossl_property_false;
-
DEFINE_STACK_OF(OSSL_PROPERTY_DEFINITION)
static const char *skip_space(const char *s)
@@ -352,7 +350,7 @@ OSSL_PROPERTY_LIST *ossl_parse_property(OSSL_LIB_CTX *ctx, const char *defn)
} else {
/* A name alone means a true Boolean */
prop->type = OSSL_PROPERTY_TYPE_STRING;
- prop->v.str_val = ossl_property_true;
+ prop->v.str_val = OSSL_PROPERTY_TRUE;
}
if (!sk_OSSL_PROPERTY_DEFINITION_push(sk, prop))
@@ -411,7 +409,7 @@ OSSL_PROPERTY_LIST *ossl_parse_query(OSSL_LIB_CTX *ctx, const char *s,
/* A name alone is a Boolean comparison for true */
prop->oper = OSSL_PROPERTY_OPER_EQ;
prop->type = OSSL_PROPERTY_TYPE_STRING;
- prop->v.str_val = ossl_property_true;
+ prop->v.str_val = OSSL_PROPERTY_TRUE;
goto skip_value;
}
if (!parse_value(ctx, &s, prop, create_values))
@@ -485,9 +483,9 @@ int ossl_property_match_count(const OSSL_PROPERTY_LIST *query,
return -1;
} else if (q[i].type != OSSL_PROPERTY_TYPE_STRING
|| (oper == OSSL_PROPERTY_OPER_EQ
- && q[i].v.str_val != ossl_property_false)
+ && q[i].v.str_val != OSSL_PROPERTY_FALSE)
|| (oper == OSSL_PROPERTY_OPER_NE
- && q[i].v.str_val == ossl_property_false)) {
+ && q[i].v.str_val == OSSL_PROPERTY_FALSE)) {
if (!q[i].optional)
return -1;
} else {
@@ -560,9 +558,13 @@ int ossl_property_parse_init(OSSL_LIB_CTX *ctx)
if (ossl_property_name(ctx, predefined_names[i], 1) == 0)
goto err;
- /* Pre-populate the two Boolean values */
- if ((ossl_property_true = ossl_property_value(ctx, "yes", 1)) == 0
- || (ossl_property_false = ossl_property_value(ctx, "no", 1)) == 0)
+ /*
+ * Pre-populate the two Boolean values. We must do them before any other
+ * values and in this order so that we get the same index as the global
+ * OSSL_PROPERTY_TRUE and OSSL_PROPERTY_FALSE values
+ */
+ if ((ossl_property_value(ctx, "yes", 1) != OSSL_PROPERTY_TRUE)
+ || (ossl_property_value(ctx, "no", 1) != OSSL_PROPERTY_FALSE))
goto err;
return 1;
diff --git a/crypto/property/property_query.c b/crypto/property/property_query.c
index 1352bc009e..28cc704840 100644
--- a/crypto/property/property_query.c
+++ b/crypto/property/property_query.c
@@ -75,8 +75,8 @@ int ossl_property_is_enabled(OSSL_LIB_CTX *ctx, const char *property_name,
return 0;
return (prop->type == OSSL_PROPERTY_TYPE_STRING
&& ((prop->oper == OSSL_PROPERTY_OPER_EQ
- && prop->v.str_val == ossl_property_true)
+ && prop->v.str_val == OSSL_PROPERTY_TRUE)
|| (prop->oper == OSSL_PROPERTY_OPER_NE
- && prop->v.str_val != ossl_property_true)));
+ && prop->v.str_val != OSSL_PROPERTY_TRUE)));
}
diff --git a/crypto/provider.c b/crypto/provider.c
index 82d980a8ae..114b426929 100644
--- a/crypto/provider.c
+++ b/crypto/provider.c
@@ -35,10 +35,16 @@ OSSL_PROVIDER *OSSL_PROVIDER_try_load(OSSL_LIB_CTX *libctx, const char *name,
actual = prov;
if (isnew && !ossl_provider_add_to_store(prov, &actual, retain_fallbacks)) {
- ossl_provider_deactivate(prov);
+ ossl_provider_deactivate(prov, 1);
ossl_provider_free(prov);
return NULL;
}
+ if (actual != prov) {
+ if (!ossl_provider_activate(actual, 1, 0)) {
+ ossl_provider_free(actual);
+ return NULL;
+ }
+ }
return actual;
}
@@ -53,7 +59,7 @@ OSSL_PROVIDER *OSSL_PROVIDER_load(OSSL_LIB_CTX *libctx, const char *name)
int OSSL_PROVIDER_unload(OSSL_PROVIDER *prov)
{
- if (!ossl_provider_deactivate(prov))
+ if (!ossl_provider_deactivate(prov, 1))
return 0;
ossl_provider_free(prov);
return 1;
diff --git a/crypto/provider_child.c b/crypto/provider_child.c
index 272d67a52d..977ea4db3b 100644
--- a/crypto/provider_child.c
+++ b/crypto/provider_child.c
@@ -22,7 +22,6 @@ DEFINE_STACK_OF(OSSL_PROVIDER)
struct child_prov_globals {
const OSSL_CORE_HANDLE *handle;
const OSSL_CORE_HANDLE *curr_prov;
- unsigned int isinited:1;
CRYPTO_RWLOCK *lock;
OSSL_FUNC_core_get_libctx_fn *c_get_libctx;
OSSL_FUNC_provider_register_child_cb_fn *c_provider_register_child_cb;
@@ -43,7 +42,6 @@ static void child_prov_ossl_ctx_free(void *vgbl)
{
struct child_prov_globals *gbl = vgbl;
- gbl->c_provider_deregister_child_cb(gbl->handle);
CRYPTO_THREAD_lock_free(gbl->lock);
OPENSSL_free(gbl);
}
@@ -110,11 +108,7 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
if (gbl == NULL)
return 0;
- /*
- * If !gbl->isinited, then we are still initing and we already hold the
- * lock - so don't take it again.
- */
- if (gbl->isinited && !CRYPTO_THREAD_write_lock(gbl->lock))
+ if (!CRYPTO_THREAD_write_lock(gbl->lock))
return 0;
provname = gbl->c_prov_name(prov);
@@ -153,7 +147,7 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
if (!ossl_provider_set_child(cprov, prov)
|| !ossl_provider_add_to_store(cprov, NULL, 0)) {
- ossl_provider_deactivate(cprov);
+ ossl_provider_deactivate(cprov, 0);
ossl_provider_free(cprov);
goto err;
}
@@ -161,8 +155,7 @@ static int provider_create_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
ret = 1;
err:
- if (gbl->isinited)
- CRYPTO_THREAD_unlock(gbl->lock);
+ CRYPTO_THREAD_unlock(gbl->lock);
return ret;
}
@@ -188,7 +181,7 @@ static int provider_remove_child_cb(const OSSL_CORE_HANDLE *prov, void *cbdata)
*/
ossl_provider_free(cprov);
if (ossl_provider_is_child(cprov)
- && !ossl_provider_deactivate(cprov))
+ && !ossl_provider_deactivate(cprov, 1))
return 0;
return 1;
@@ -272,11 +265,20 @@ int ossl_provider_init_as_child(OSSL_LIB_CTX *ctx,
ctx))
return 0;
- gbl->isinited = 1;
-
return 1;
}
+void ossl_provider_deinit_child(OSSL_LIB_CTX *ctx)
+{
+ struct child_prov_globals *gbl
+ = ossl_lib_ctx_get_data(ctx, OSSL_LIB_CTX_CHILD_PROVIDER_INDEX,
+ &child_prov_ossl_ctx_method);
+ if (gbl == NULL)
+ return;
+
+ gbl->c_provider_deregister_child_cb(gbl->handle);
+}
+
int ossl_provider_up_ref_parent(OSSL_PROVIDER *prov, int activate)
{
struct child_prov_globals *gbl;
diff --git a/crypto/provider_conf.c b/crypto/provider_conf.c
index 054261771a..c13c887c3d 100644
--- a/crypto/provider_conf.c
+++ b/crypto/provider_conf.c
@@ -222,13 +222,24 @@ static int provider_conf_load(OSSL_LIB_CTX *libctx, const char *name,
if (!ossl_provider_activate(prov, 1, 0)) {
ok = 0;
} else if (!ossl_provider_add_to_store(prov, &actual, 0)) {
- ossl_provider_deactivate(prov);
+ ossl_provider_deactivate(prov, 1);
+ ok = 0;
+ } else if (actual != prov
+ && !ossl_provider_activate(actual, 1, 0)) {
+ ossl_provider_free(actual);
ok = 0;
} else {
if (pcgbl->activated_providers == NULL)
pcgbl->activated_providers = sk_OSSL_PROVIDER_new_null();
- sk_OSSL_PROVIDER_push(pcgbl->activated_providers, actual);
- ok = 1;
+ if (pcgbl->activated_providers == NULL
+ || !sk_OSSL_PROVIDER_push(pcgbl->activated_providers,
+ actual)) {
+ ossl_provider_deactivate(actual, 1);
+ ossl_provider_free(actual);
+ ok = 0;
+ } else {
+ ok = 1;
+ }
}
}
if (!ok)
diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index e4069eb4f7..cb4c07c781 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -107,8 +107,8 @@
* some other function while holding a lock make sure you know whether it
* will make any upcalls or not. For example ossl_provider_up_ref() can call
* ossl_provider_up_ref_parent() which can call the c_prov_up_ref() upcall.
- * - It is permissible to hold the store lock when calling child provider
- * callbacks. No other locks may be held during such callbacks.
+ * - It is permissible to hold the store and flag locks when calling child
+ * provider callbacks. No other locks may be held during such callbacks.
*/
static OSSL_PROVIDER *provider_new(const char *name,
@@ -230,7 +230,7 @@ struct provider_store_st {
static void provider_deactivate_free(OSSL_PROVIDER *prov)
{
if (prov->flag_activated)
- ossl_provider_deactivate(prov);
+ ossl_provider_deactivate(prov, 1);
ossl_provider_free(prov);
}
@@ -424,7 +424,11 @@ OSSL_PROVIDER *ossl_provider_find(OSSL_LIB_CTX *libctx, const char *name,
#endif
tmpl.name = (char *)name;
- if (!CRYPTO_THREAD_read_lock(store->lock))
+ /*
+ * A "find" operation can sort the stack, and therefore a write lock is
+ * required.
+ */
+ if (!CRYPTO_THREAD_write_lock(store->lock))
return NULL;
if ((i = sk_OSSL_PROVIDER_find(store->providers, &tmpl)) != -1)
prov = sk_OSSL_PROVIDER_value(store->providers, i);
@@ -499,13 +503,18 @@ static int provider_up_ref_intern(OSSL_PROVIDER *prov, int activate)
static int provider_free_intern(OSSL_PROVIDER *prov, int deactivate)
{
if (deactivate)
- return ossl_provider_deactivate(prov);
+ return ossl_provider_deactivate(prov, 1);
ossl_provider_free(prov);
return 1;
}
#endif
+/*
+ * We assume that the requested provider does not already exist in the store.
+ * The caller should check. If it does exist then adding it to the store later
+ * will fail.
+ */
OSSL_PROVIDER *ossl_provider_new(OSSL_LIB_CTX *libctx, const char *name,
OSSL_provider_init_fn *init_function,
int noconfig)
@@ -517,14 +526,6 @@ OSSL_PROVIDER *ossl_provider_new(OSSL_LIB_CTX *libctx, const char *name,
if ((store = get_provider_store(libctx)) == NULL)
return NULL;
- if ((prov = ossl_provider_find(libctx, name,
- noconfig)) != NULL) { /* refcount +1 */
- ossl_provider_free(prov); /* refcount -1 */
- ERR_raise_data(ERR_LIB_CRYPTO, CRYPTO_R_PROVIDER_ALREADY_EXISTS,
- "name=%s", name);
- return NULL;
- }
-
memset(&template, 0, sizeof(template));
if (init_function == NULL) {
const OSSL_PROVIDER_INFO *p;
@@ -645,8 +646,11 @@ int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
* name and raced to put them in the store. This thread lost. We
* deactivate the one we just created and use the one that already
* exists instead.
+ * If we get here then we know we did not create provider children
+ * above, so we inform ossl_provider_deactivate not to attempt to remove
+ * any.
*/
- ossl_provider_deactivate(prov);
+ ossl_provider_deactivate(prov, 0);
ossl_provider_free(prov);
}
@@ -1003,27 +1007,35 @@ static int provider_init(OSSL_PROVIDER *prov)
}
/*
- * Deactivate a provider.
+ * Deactivate a provider. If upcalls is 0 then we suppress any upcalls to a
+ * parent provider. If removechildren is 0 then we suppress any calls to remove
+ * child providers.
* Return -1 on failure and the activation count on success
*/
-static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls)
+static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls,
+ int removechildren)
{
int count;
struct provider_store_st *store;
#ifndef FIPS_MODULE
- int freeparent = 0, removechildren = 0;
+ int freeparent = 0;
#endif
+ int lock = 1;
if (!ossl_assert(prov != NULL))
return -1;
+ /*
+ * No need to lock if we've got no store because we've not been shared with
+ * other threads.
+ */
store = get_provider_store(prov->libctx);
if (store == NULL)
- return -1;
+ lock = 0;
- if (!CRYPTO_THREAD_read_lock(store->lock))
+ if (lock && !CRYPTO_THREAD_read_lock(store->lock))
return -1;
- if (!CRYPTO_THREAD_write_lock(prov->flag_lock)) {
+ if (lock && !CRYPTO_THREAD_write_lock(prov->flag_lock)) {
CRYPTO_THREAD_unlock(store->lock);
return -1;
}
@@ -1040,17 +1052,15 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls)
}
#endif
- if ((count = --prov->activatecnt) < 1) {
+ if ((count = --prov->activatecnt) < 1)
prov->flag_activated = 0;
#ifndef FIPS_MODULE
- removechildren = 1;
+ else
+ removechildren = 0;
#endif
- }
-
- CRYPTO_THREAD_unlock(prov->flag_lock);
#ifndef FIPS_MODULE
- if (removechildren) {
+ if (removechildren && store != NULL) {
int i, max = sk_OSSL_PROVIDER_CHILD_CB_num(store->child_cbs);
OSSL_PROVIDER_CHILD_CB *child_cb;
@@ -1060,7 +1070,10 @@ static int provider_deactivate(OSSL_PROVIDER *prov, int upcalls)
}
}
#endif
- CRYPTO_THREAD_unlock(store->lock);
+ if (lock) {
+ CRYPTO_THREAD_unlock(prov->flag_lock);
+ CRYPTO_THREAD_unlock(store->lock);
+ }
#ifndef FIPS_MODULE
if (freeparent)
ossl_provider_free_parent(prov, 1);
@@ -1078,7 +1091,7 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
{
int count = -1;
struct provider_store_st *store;
- int ret = 1, createchildren = 0;
+ int ret = 1;
store = prov->store;
/*
@@ -1116,15 +1129,13 @@ static int provider_activate(OSSL_PROVIDER *prov, int lock, int upcalls)
count = ++prov->activatecnt;
prov->flag_activated = 1;
- if (prov->activatecnt == 1 && store != NULL)
- createchildren = 1;
-
- if (lock)
- CRYPTO_THREAD_unlock(prov->flag_lock);
- if (createchildren)
+ if (prov->activatecnt == 1 && store != NULL) {
ret = create_provider_children(prov);
- if (lock)
+ }
+ if (lock) {
+ CRYPTO_THREAD_unlock(prov->flag_lock);
CRYPTO_THREAD_unlock(store->lock);
+ }
if (!ret)
return -1;
@@ -1170,11 +1181,12 @@ int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild)
return 0;
}
-int ossl_provider_deactivate(OSSL_PROVIDER *prov)
+int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren)
{
int count;
- if (prov == NULL || (count = provider_deactivate(prov, 1)) < 0)
+ if (prov == NULL
+ || (count = provider_deactivate(prov, 1, removechildren)) < 0)
return 0;
return count == 0 ? provider_flush_store_cache(prov) : 1;
}
@@ -1356,7 +1368,7 @@ int ossl_provider_doall_activated(OSSL_LIB_CTX *ctx,
for (curr++; curr < max; curr++) {
OSSL_PROVIDER *prov = sk_OSSL_PROVIDER_value(provs, curr);
- provider_deactivate(prov, 0);
+ provider_deactivate(prov, 0, 1);
/*
* As above where we did the up-ref, we don't call ossl_provider_free
* to avoid making upcalls. There should always be at least one ref
diff --git a/doc/internal/man3/ossl_provider_new.pod b/doc/internal/man3/ossl_provider_new.pod
index 10d197bcfc..0cf51a163f 100644
--- a/doc/internal/man3/ossl_provider_new.pod
+++ b/doc/internal/man3/ossl_provider_new.pod
@@ -8,7 +8,7 @@ ossl_provider_set_fallback, ossl_provider_set_module_path,
ossl_provider_add_parameter, ossl_provider_set_child, ossl_provider_get_parent,
ossl_provider_up_ref_parent, ossl_provider_free_parent,
ossl_provider_default_props_update, ossl_provider_get0_dispatch,
-ossl_provider_init_as_child,
+ossl_provider_init_as_child, ossl_provider_deinit_child,
ossl_provider_activate, ossl_provider_deactivate, ossl_provider_add_to_store,
ossl_provider_ctx,
ossl_provider_doall_activated,
@@ -54,7 +54,7 @@ ossl_provider_get_capabilities
* If the Provider is a module, the module will be loaded
*/
int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild);
- int ossl_provider_deactivate(OSSL_PROVIDER *prov);
+ int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren);
int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
int retain_fallbacks);
@@ -99,7 +99,7 @@ ossl_provider_get_capabilities
int ossl_provider_init_as_child(OSSL_LIB_CTX *ctx,
const OSSL_CORE_HANDLE *handle,
const OSSL_DISPATCH *in);
-
+ void ossl_provider_deinit_child(OSSL_LIB_CTX *ctx);
=head1 DESCRIPTION
@@ -226,7 +226,9 @@ no action is taken and ossl_provider_activate() returns success.
ossl_provider_deactivate() "deactivates" the provider for the given
provider object I<prov> by decrementing its activation count. When
-that count reaches zero, the activation flag is cleared.
+that count reaches zero, the activation flag is cleared. If the
+I<removechildren> parameter is 0 then no attempt is made to remove any
+associated child providers.
ossl_provider_add_to_store() adds the provider I<prov> to the provider store and
makes it available to other threads. This will prevent future automatic loading
@@ -306,6 +308,10 @@ the necessary upcalls for managing child providers. The I<handle> and I<in>
parameters are the B<OSSL_CORE_HANDLE> and B<OSSL_DISPATCH> pointers that were
passed to the provider's B<OSSL_provider_init> function.
+ossl_provider_deinit_child() deregisters callbacks from the parent library
+context about provider creation or removal events for the child library context
+I<ctx>. Must only be called if I<ctx> is a child library context.
+
=head1 NOTES
Locating a provider module happens as follows:
diff --git a/doc/man3/DEFINE_STACK_OF.pod b/doc/man3/DEFINE_STACK_OF.pod
index d7152466f4..ec9eda81c6 100644
--- a/doc/man3/DEFINE_STACK_OF.pod
+++ b/doc/man3/DEFINE_STACK_OF.pod
@@ -178,7 +178,10 @@ where a comparison function has been specified, I<sk> is sorted and
B<sk_I<TYPE>_find>() returns the index of a matching element or B<-1> if there
is no match. Note that, in this case the comparison function will usually
compare the values pointed to rather than the pointers themselves and
-the order of elements in I<sk> can change.
+the order of elements in I<sk> can change. Note that because the stack may be
+sorted as the result of a B<sk_I<TYPE>_find>() call, if a lock is being used to
+synchronise access to the stack across multiple threads, then that lock must be
+a "write" lock.
B<sk_I<TYPE>_find_ex>() operates like B<sk_I<TYPE>_find>() except when a
comparison function has been specified and no matching element is found.
diff --git a/include/internal/provider.h b/include/internal/provider.h
index 237c852e8d..d09829d05e 100644
--- a/include/internal/provider.h
+++ b/include/internal/provider.h
@@ -57,7 +57,7 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx);
* If the Provider is a module, the module will be loaded
*/
int ossl_provider_activate(OSSL_PROVIDER *prov, int upcalls, int aschild);
-int ossl_provider_deactivate(OSSL_PROVIDER *prov);
+int ossl_provider_deactivate(OSSL_PROVIDER *prov, int removechildren);
int ossl_provider_add_to_store(OSSL_PROVIDER *prov, OSSL_PROVIDER **actualprov,
int retain_fallbacks);
@@ -108,6 +108,7 @@ void ossl_provider_add_conf_module(void);
int ossl_provider_init_as_child(OSSL_LIB_CTX *ctx,
const OSSL_CORE_HANDLE *handle,
const OSSL_DISPATCH *in);
+void ossl_provider_deinit_child(OSSL_LIB_CTX *ctx);
# ifdef __cplusplus
}
diff --git a/test/provider_internal_test.c b/test/provider_internal_test.c
index d9cc68d59d..cb7d5efcf5 100644
--- a/test/provider_internal_test.c
+++ b/test/provider_internal_test.c
@@ -31,7 +31,7 @@ static int test_provider(OSSL_PROVIDER *prov, const char *expected_greeting)
&& TEST_ptr(greeting = greeting_request[0].data)
&& TEST_size_t_gt(greeting_request[0].data_size, 0)
&& TEST_str_eq(greeting, expected_greeting)
- && TEST_true(ossl_provider_deactivate(prov));
+ && TEST_true(ossl_provider_deactivate(prov, 1));
TEST_info("Got this greeting: %s\n", greeting);
ossl_provider_free(prov);
diff --git a/test/threadstest.c b/test/threadstest.c
index 3160d9e334..505dd79e95 100644
--- a/test/threadstest.c
+++ b/test/threadstest.c
@@ -464,18 +464,20 @@ static int test_multi(int idx)
return testresult;
}
+static char *multi_load_provider = "legacy";
/*
* This test attempts to load several providers at the same time, and if
* run with a thread sanitizer, should crash if the core provider code
* doesn't synchronize well enough.
*/
-#define MULTI_LOAD_THREADS 3
+#define MULTI_LOAD_THREADS 10
static void test_multi_load_worker(void)
{
OSSL_PROVIDER *prov;
- (void)TEST_ptr(prov = OSSL_PROVIDER_load(NULL, "default"));
- (void)TEST_true(OSSL_PROVIDER_unload(prov));
+ if (!TEST_ptr(prov = OSSL_PROVIDER_load(NULL, multi_load_provider))
+ || !TEST_true(OSSL_PROVIDER_unload(prov)))
+ multi_success = 0;
}
static int test_multi_default(void)
@@ -519,6 +521,7 @@ static int test_multi_load(void)
{
thread_t threads[MULTI_LOAD_THREADS];
int i, res = 1;
+ OSSL_PROVIDER *prov;
/* The multidefault test must run prior to this test */
if (!multidefault_run) {
@@ -526,13 +529,27 @@ static int test_multi_load(void)
res = test_multi_default();
}
+ /*
+ * We use the legacy provider in test_multi_load_worker because it uses a
+ * child libctx that might hit more codepaths that might be sensitive to
+ * threading issues. But in a no-legacy build that won't be loadable so
+ * we use the default provider instead.
+ */
+ prov = OSSL_PROVIDER_load(NULL, "legacy");
+ if (prov == NULL) {
+ TEST_info("Cannot load legacy provider - assuming this is a no-legacy build");
+ multi_load_provider = "default";
+ }
+ OSSL_PROVIDER_unload(prov);
+
+ multi_success = 1;
for (i = 0; i < MULTI_LOAD_THREADS; i++)
(void)TEST_true(run_thread(&threads[i], test_multi_load_worker));
for (i = 0; i < MULTI_LOAD_THREADS; i++)
(void)TEST_true(wait_for_thread(threads[i]));
- return res;
+ return res && multi_success;
}
typedef enum OPTION_choice {
More information about the openssl-commits
mailing list