[openssl] master update
Richard Levitte
levitte at openssl.org
Thu Mar 4 15:11:19 UTC 2021
The branch master has been updated
via c2ec2bb7c146d1e48568f27d11dca02c06c36338 (commit)
via d60a8e0a2345205242e21aae35815645708580c4 (commit)
via 2f17e978a0ec5becda8a61dcf3e7840740ccdfd3 (commit)
from 8c631cfaa1f812ed990053c1b0c73f3a3f369aca (commit)
- Log -----------------------------------------------------------------
commit c2ec2bb7c146d1e48568f27d11dca02c06c36338
Author: Richard Levitte <levitte at openssl.org>
Date: Mon Mar 1 13:27:24 2021 +0100
Make provider provider_init thread safe, and flag checking/setting too
provider_init() makes changes in the provider structure, and needs a
bit of protection to ensure that doesn't happen concurrently with race
conditions.
This also demands a bit of protection of the flags, since they are
bits and presumably occupy the same byte in memory.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14354)
commit d60a8e0a2345205242e21aae35815645708580c4
Author: Richard Levitte <levitte at openssl.org>
Date: Mon Mar 1 13:27:15 2021 +0100
Make ossl_provider_disable_fallback_loading() thread safe
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14354)
commit 2f17e978a0ec5becda8a61dcf3e7840740ccdfd3
Author: Richard Levitte <levitte at openssl.org>
Date: Mon Mar 1 16:31:34 2021 +0100
test/threadstest.c: Add a test to load providers concurrently
If we don't synchronize properly in the core provider code, and build
with a thread sanitizer, this should cause a crash.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14354)
-----------------------------------------------------------------------
Summary of changes:
crypto/provider_core.c | 48 +++++++++++++++++++++++++++++++++++++-----------
test/threadstest.c | 29 +++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 11 deletions(-)
diff --git a/crypto/provider_core.c b/crypto/provider_core.c
index d210026e25..1326f83f7e 100644
--- a/crypto/provider_core.c
+++ b/crypto/provider_core.c
@@ -48,6 +48,9 @@ struct ossl_provider_st {
unsigned int flag_fallback:1; /* Can be used as fallback */
unsigned int flag_activated_as_fallback:1;
+ /* Getting and setting the flags require synchronization */
+ CRYPTO_RWLOCK *flag_lock;
+
/* OpenSSL library side data */
CRYPTO_REF_COUNT refcnt;
CRYPTO_RWLOCK *refcnt_lock; /* For the ref counter */
@@ -201,7 +204,9 @@ int ossl_provider_disable_fallback_loading(OSSL_LIB_CTX *libctx)
struct provider_store_st *store;
if ((store = get_provider_store(libctx)) != NULL) {
+ CRYPTO_THREAD_write_lock(store->lock);
store->use_fallbacks = 0;
+ CRYPTO_THREAD_unlock(store->lock);
return 1;
}
return 0;
@@ -255,6 +260,7 @@ static OSSL_PROVIDER *provider_new(const char *name,
#endif
|| !ossl_provider_up_ref(prov) /* +1 One reference to be returned */
|| (prov->opbits_lock = CRYPTO_THREAD_lock_new()) == NULL
+ || (prov->flag_lock = CRYPTO_THREAD_lock_new()) == NULL
|| (prov->name = OPENSSL_strdup(name)) == NULL) {
ossl_provider_free(prov);
ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE);
@@ -375,6 +381,7 @@ void ossl_provider_free(OSSL_PROVIDER *prov)
OPENSSL_free(prov->path);
sk_INFOPAIR_pop_free(prov->parameters, free_infopair);
CRYPTO_THREAD_lock_free(prov->opbits_lock);
+ CRYPTO_THREAD_lock_free(prov->flag_lock);
#ifndef HAVE_ATOMICS
CRYPTO_THREAD_lock_free(prov->refcnt_lock);
CRYPTO_THREAD_lock_free(prov->activatecnt_lock);
@@ -470,9 +477,19 @@ static int provider_init(OSSL_PROVIDER *prov)
OSSL_FUNC_provider_get_reason_strings_fn *p_get_reason_strings = NULL;
# endif
#endif
+ int ok = 0;
- if (prov->flag_initialized)
- return 1;
+ /*
+ * The flag lock is used to lock init, not only because the flag is
+ * checked here and set at the end, but also because this function
+ * modifies a number of things in the provider structure that this
+ * function needs to perform under lock anyway.
+ */
+ CRYPTO_THREAD_write_lock(prov->flag_lock);
+ if (prov->flag_initialized) {
+ ok = 1;
+ goto end;
+ }
/*
* If the init function isn't set, it indicates that this provider is
@@ -480,7 +497,7 @@ static int provider_init(OSSL_PROVIDER *prov)
*/
if (prov->init_function == NULL) {
#ifdef FIPS_MODULE
- return 0;
+ goto end;
#else
if (prov->module == NULL) {
char *allocated_path = NULL;
@@ -491,13 +508,14 @@ static int provider_init(OSSL_PROVIDER *prov)
if ((prov->module = DSO_new()) == NULL) {
/* DSO_new() generates an error already */
- return 0;
+ goto end;
}
if ((store = get_provider_store(prov->libctx)) == NULL
|| !CRYPTO_THREAD_read_lock(store->lock))
- return 0;
+ goto end;
load_dir = store->default_path;
+ CRYPTO_THREAD_unlock(store->lock);
if (load_dir == NULL) {
load_dir = ossl_safe_getenv("OPENSSL_MODULES");
@@ -514,7 +532,6 @@ static int provider_init(OSSL_PROVIDER *prov)
DSO_convert_filename(prov->module, prov->name);
if (module_path != NULL)
merged_path = DSO_merge(prov->module, module_path, load_dir);
- CRYPTO_THREAD_unlock(store->lock);
if (merged_path == NULL
|| (DSO_load(prov->module, merged_path, NULL, 0)) == NULL) {
@@ -542,7 +559,7 @@ static int provider_init(OSSL_PROVIDER *prov)
DSO_free(prov->module);
prov->module = NULL;
#endif
- return 0;
+ goto end;
}
prov->provctx = tmp_provctx;
@@ -603,7 +620,7 @@ static int provider_init(OSSL_PROVIDER *prov)
cnt = 0;
while (reasonstrings[cnt].id != 0) {
if (ERR_GET_LIB(reasonstrings[cnt].id) != 0)
- return 0;
+ goto end;
cnt++;
}
cnt++; /* One for the terminating item */
@@ -612,7 +629,7 @@ static int provider_init(OSSL_PROVIDER *prov)
prov->error_strings =
OPENSSL_zalloc(sizeof(ERR_STRING_DATA) * (cnt + 1));
if (prov->error_strings == NULL)
- return 0;
+ goto end;
/*
* Set the "library" name.
@@ -635,7 +652,11 @@ static int provider_init(OSSL_PROVIDER *prov)
/* With this flag set, this provider has become fully "loaded". */
prov->flag_initialized = 1;
- return 1;
+ ok = 1;
+
+ end:
+ CRYPTO_THREAD_unlock(prov->flag_lock);
+ return ok;
}
static int provider_deactivate(OSSL_PROVIDER *prov)
@@ -648,8 +669,11 @@ static int provider_deactivate(OSSL_PROVIDER *prov)
if (CRYPTO_DOWN_REF(&prov->activatecnt, &ref, prov->activatecnt_lock) <= 0)
return 0;
- if (ref < 1)
+ if (ref < 1) {
+ CRYPTO_THREAD_write_lock(prov->flag_lock);
prov->flag_activated = 0;
+ CRYPTO_THREAD_unlock(prov->flag_lock);
+ }
/* We don't deinit here, that's done in ossl_provider_free() */
return 1;
@@ -663,7 +687,9 @@ static int provider_activate(OSSL_PROVIDER *prov)
return 0;
if (provider_init(prov)) {
+ CRYPTO_THREAD_write_lock(prov->flag_lock);
prov->flag_activated = 1;
+ CRYPTO_THREAD_unlock(prov->flag_lock);
return 1;
}
diff --git a/test/threadstest.c b/test/threadstest.c
index 9c8e2181d0..26807e294c 100644
--- a/test/threadstest.c
+++ b/test/threadstest.c
@@ -473,6 +473,34 @@ static int test_multi(int idx)
return testresult;
}
+/*
+ * 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
+static void test_multi_load_worker(void)
+{
+ OSSL_PROVIDER *prov;
+
+ TEST_ptr(prov = OSSL_PROVIDER_load(NULL, "default"));
+ TEST_true(OSSL_PROVIDER_unload(prov));
+}
+
+static int test_multi_load(void)
+{
+ thread_t threads[MULTI_LOAD_THREADS];
+ int i;
+
+ for (i = 0; i < MULTI_LOAD_THREADS; i++)
+ TEST_true(run_thread(&threads[i], test_multi_load_worker));
+
+ for (i = 0; i < MULTI_LOAD_THREADS; i++)
+ TEST_true(wait_for_thread(threads[i]));
+
+ return 1;
+}
+
typedef enum OPTION_choice {
OPT_ERR = -1,
OPT_EOF = 0,
@@ -518,6 +546,7 @@ int setup_tests(void)
ADD_TEST(test_once);
ADD_TEST(test_thread_local);
ADD_TEST(test_atomic);
+ ADD_TEST(test_multi_load);
ADD_ALL_TESTS(test_multi, 4);
return 1;
}
More information about the openssl-commits
mailing list