From 11078bd3c15fb4f47f1e832efbac81163673583d Mon Sep 17 00:00:00 2001 From: Arran Cudbard-Bell Date: Tue, 4 Jan 2022 13:15:43 -0600 Subject: [PATCH] Ensure all the thread-specific destructors run before the global destructors Rework the OpenSSL cleanup logic --- src/lib/tls/base.c | 61 ++++++++++++++++++++++++++++++++++++------- src/lib/tls/ctx.c | 2 +- src/lib/util/atexit.c | 13 ++++++++- 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/src/lib/tls/base.c b/src/lib/tls/base.c index cd188b578c..4d82af28f6 100644 --- a/src/lib/tls/base.c +++ b/src/lib/tls/base.c @@ -404,10 +404,16 @@ void fr_openssl_free(void) { if (--instance_count > 0) return; -#if OPENSSL_VERSION_NUMBER < 0x30000000L - fr_tls_engine_free_all(); + fr_dict_autofree(tls_dict); -#else + fr_tls_log_free(); + + fr_tls_bio_free(); +} + +#if OPENSSL_VERSION_NUMBER >= 0x30000000L +static void _openssl_provider_free(void) +{ if (openssl_default_provider && !OSSL_PROVIDER_unload(openssl_default_provider)) { fr_tls_log_error(NULL, "Failed unloading default provider"); } @@ -417,15 +423,19 @@ void fr_openssl_free(void) fr_tls_log_error(NULL, "Failed unloading legacy provider"); } openssl_legacy_provider = NULL; +} #endif - OPENSSL_cleanup(); - - fr_dict_autofree(tls_dict); - - fr_tls_log_free(); +#if OPENSSL_VERSION_NUMBER < 0x30000000L +static void _openssl_engine_free(void) +{ + fr_tls_engine_free_all(); +} +#endif - fr_tls_bio_free(); +static void fr_openssl_cleanup(UNUSED void *uctx) +{ + OPENSSL_cleanup(); } /** Add all the default ciphers and message digests to our context. @@ -449,6 +459,16 @@ int fr_openssl_init(void) return -1; } + /* + * NO_ATEXIT has no effect if init is done after + * loading providers, and we need to control the + * exit handler as it needs to be executed last + * after all the EVP_MD ctx have been called, as + * they may unload elements of providers once all + * the contexts have been cleaned up. + */ + OPENSSL_init_crypto(OPENSSL_INIT_NO_ATEXIT | OPENSSL_INIT_LOAD_CONFIG, NULL); + #if OPENSSL_VERSION_NUMBER >= 0x30000000L /* * Load the default provider for most algorithms @@ -471,7 +491,18 @@ int fr_openssl_init(void) } #endif - OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL); + /* + * It's best to use OpenSSL's cleanup stack + * as then everything is cleaned up relative + * to the OPENSSL_cleanup() call. + */ +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + OPENSSL_atexit(_openssl_provider_free); +#endif + +#if OPENSSL_VERSION_NUMBER < 0x30000000L + OPENSSL_atexit(_openssl_engine_free); +#endif /* * SHA256 is in all versions of OpenSSL, but isn't @@ -492,6 +523,16 @@ int fr_openssl_init(void) fr_tls_bio_init(); + /* + * Use an atexit handler to try and ensure + * that OpenSSL gets freed last. + * + * All EVP_*ctxs need to be freed before we + * de-initialise the libraries else we get + * crashes (at least with OpenSSL 3.0.1). + */ + fr_atexit_global(fr_openssl_cleanup, NULL); + instance_count++; return 0; diff --git a/src/lib/tls/ctx.c b/src/lib/tls/ctx.c index ea1d44dd69..13eea42e88 100644 --- a/src/lib/tls/ctx.c +++ b/src/lib/tls/ctx.c @@ -946,7 +946,7 @@ post_ca: * message. For every new session, there can be a * different callback argument. */ - SSL_CTX_set_msg_callback(ctx, fr_tls_session_msg_cb); + SSL_CTX_set_msg_callback(ctx, fr_tls_session_msg_cb); /* Set Info callback */ SSL_CTX_set_info_callback(ctx, fr_tls_session_info_cb); diff --git a/src/lib/util/atexit.c b/src/lib/util/atexit.c index 74976e8704..9d2361f761 100644 --- a/src/lib/util/atexit.c +++ b/src/lib/util/atexit.c @@ -69,6 +69,7 @@ struct fr_exit_handler_list_s { }; static _Thread_local fr_atexit_list_t *fr_atexit_thread_local = NULL; +static fr_atexit_list_t *fr_atexit_threads = NULL; static fr_atexit_list_t *fr_atexit_global = NULL; static pthread_mutex_t fr_atexit_global_mutex = PTHREAD_MUTEX_INITIALIZER; static bool is_exiting; @@ -168,6 +169,7 @@ static void _global_free(void) is_exiting = true; pthread_mutex_unlock(&fr_atexit_global_mutex); + TALLOC_FREE(fr_atexit_threads); /* Forcefully cleanup any thread-specific memory */ TALLOC_FREE(fr_atexit_global); } @@ -185,6 +187,15 @@ int fr_atexit_global_setup(void) fr_dlist_talloc_init(&fr_atexit_global->head, fr_atexit_entry_t, entry); talloc_set_destructor(fr_atexit_global, _global_list_free); + + fr_atexit_threads = talloc_zero(NULL, fr_atexit_list_t); + if (unlikely(!fr_atexit_threads)) return -1; + + THREAD_LOCAL_DEBUG("%s - Alloced threads destructor list %p", __FUNCTION__, fr_atexit_threads); + + fr_dlist_talloc_init(&fr_atexit_threads->head, fr_atexit_entry_t, entry); + talloc_set_destructor(fr_atexit_threads, _global_list_free); + atexit(_global_free); /* Call all remaining destructors at process exit */ return 0; @@ -262,7 +273,7 @@ int _fr_atexit_thread_local(NDEBUG_LOCATION_ARGS */ pthread_mutex_lock(&fr_atexit_global_mutex); list->e = atexit_entry_alloc(NDEBUG_LOCATION_VALS - fr_atexit_global, + fr_atexit_threads, _thread_local_free, list); -- 2.47.2