From 770de3462c0d655a6543a6c1a2c0bda7b57178f9 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 14 Aug 2019 14:43:11 +0100 Subject: [PATCH] Fix context locking Some parts of OPENSSL_CTX intialisation can get quite complex (e.g. RAND). This can lead to complex interactions where different parts of the library try to initialise while other parts are still initialising. This can lead to deadlocks because both parts want to obtain the init lock. We separate out the init lock so that it is only used to manage the dynamic list of indexes. Each part of the library gets its own initialisation lock. Fixes #9454 Reviewed-by: Paul Dale Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/9590) --- crypto/context.c | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/crypto/context.c b/crypto/context.c index cc1ec0e664..ad4e997a7c 100644 --- a/crypto/context.c +++ b/crypto/context.c @@ -28,6 +28,9 @@ struct openssl_ctx_st { /* Map internal static indexes to dynamically created indexes */ int dyn_indexes[OPENSSL_CTX_MAX_INDEXES]; + /* Keep a separate lock for each index */ + CRYPTO_RWLOCK *index_locks[OPENSSL_CTX_MAX_INDEXES]; + CRYPTO_RWLOCK *oncelock; int run_once_done[OPENSSL_CTX_MAX_RUN_ONCE]; int run_once_ret[OPENSSL_CTX_MAX_RUN_ONCE]; @@ -53,8 +56,12 @@ static int context_init(OPENSSL_CTX *ctx) if (ctx->oncelock == NULL) goto err; - for (i = 0; i < OPENSSL_CTX_MAX_INDEXES; i++) + for (i = 0; i < OPENSSL_CTX_MAX_INDEXES; i++) { + ctx->index_locks[i] = CRYPTO_THREAD_lock_new(); ctx->dyn_indexes[i] = -1; + if (ctx->index_locks[i] == NULL) + goto err; + } if (!do_ex_data_init(ctx)) goto err; @@ -76,6 +83,7 @@ static int context_init(OPENSSL_CTX *ctx) static int context_deinit(OPENSSL_CTX *ctx) { struct openssl_ctx_onfree_list_st *tmp, *onfree; + int i; if (ctx == NULL) return 1; @@ -91,6 +99,9 @@ static int context_deinit(OPENSSL_CTX *ctx) } CRYPTO_free_ex_data(CRYPTO_EX_INDEX_OPENSSL_CTX, NULL, &ctx->data); crypto_cleanup_all_ex_data_int(ctx); + for (i = 0; i < OPENSSL_CTX_MAX_INDEXES; i++) + CRYPTO_THREAD_lock_free(ctx->index_locks[i]); + CRYPTO_THREAD_lock_free(ctx->oncelock); CRYPTO_THREAD_lock_free(ctx->lock); ctx->lock = NULL; @@ -187,25 +198,48 @@ void *openssl_ctx_get_data(OPENSSL_CTX *ctx, int index, const OPENSSL_CTX_METHOD *meth) { void *data = NULL; + int dynidx; ctx = openssl_ctx_get_concrete(ctx); if (ctx == NULL) return NULL; CRYPTO_THREAD_read_lock(ctx->lock); + dynidx = ctx->dyn_indexes[index]; + CRYPTO_THREAD_unlock(ctx->lock); + + if (dynidx != -1) { + CRYPTO_THREAD_read_lock(ctx->index_locks[index]); + data = CRYPTO_get_ex_data(&ctx->data, dynidx); + CRYPTO_THREAD_unlock(ctx->index_locks[index]); + return data; + } - if (ctx->dyn_indexes[index] == -1 - && !openssl_ctx_init_index(ctx, index, meth)) { + CRYPTO_THREAD_write_lock(ctx->index_locks[index]); + CRYPTO_THREAD_write_lock(ctx->lock); + + dynidx = ctx->dyn_indexes[index]; + if (dynidx != -1) { + CRYPTO_THREAD_unlock(ctx->lock); + data = CRYPTO_get_ex_data(&ctx->data, dynidx); + CRYPTO_THREAD_unlock(ctx->index_locks[index]); + return data; + } + + if (!openssl_ctx_init_index(ctx, index, meth)) { CRYPTO_THREAD_unlock(ctx->lock); + CRYPTO_THREAD_unlock(ctx->index_locks[index]); return NULL; } + CRYPTO_THREAD_unlock(ctx->lock); + /* The alloc call ensures there's a value there */ if (CRYPTO_alloc_ex_data(CRYPTO_EX_INDEX_OPENSSL_CTX, NULL, &ctx->data, ctx->dyn_indexes[index])) data = CRYPTO_get_ex_data(&ctx->data, ctx->dyn_indexes[index]); - CRYPTO_THREAD_unlock(ctx->lock); + CRYPTO_THREAD_unlock(ctx->index_locks[index]); return data; } -- 2.25.1