From 493b77167dc367c394de23cfe78a029298e2a254 Mon Sep 17 00:00:00 2001 From: maddeleine <59030281+maddeleine@users.noreply.github.com> Date: Tue, 12 Nov 2024 12:32:04 -0800 Subject: [PATCH] feat: Reworking cleanup behavior (#4871) --- api/s2n.h | 10 ++---- codebuild/bin/s2n_dynamic_load_test.c | 15 +++++--- codebuild/bin/test_exec_leak.sh | 2 +- .../usage-guide/topics/ch02-initialization.md | 13 +++++-- tests/s2n_test.h | 27 +++++++------- tests/unit/s2n_init_test.c | 36 ++++++++----------- tests/unit/s2n_random_test.c | 7 ++-- utils/s2n_init.c | 10 ++---- utils/s2n_init.h | 1 + 9 files changed, 56 insertions(+), 65 deletions(-) diff --git a/api/s2n.h b/api/s2n.h index f104a5ee2af..e7e2a634716 100644 --- a/api/s2n.h +++ b/api/s2n.h @@ -229,8 +229,8 @@ S2N_API extern unsigned long s2n_get_openssl_version(void); S2N_API extern int s2n_init(void); /** - * Cleans up any internal resources used by s2n-tls. This function should be called from each thread or process - * that is created subsequent to calling `s2n_init` when that thread or process is done calling other s2n-tls functions. + * Cleans up thread-local resources used by s2n-tls. Does not perform a full library cleanup. To fully + * clean up the library use s2n_cleanup_final(). * * @returns S2N_SUCCESS on success. S2N_FAILURE on failure */ @@ -239,12 +239,6 @@ S2N_API extern int s2n_cleanup(void); /* * Performs a complete deinitialization and cleanup of the s2n-tls library. * - * s2n_cleanup_final will always perform a complete cleanup. In contrast, - * s2n_cleanup will only perform a complete cleanup if the atexit handler - * is disabled and s2n_cleanup is called by the thread that called s2n_init. - * Therefore s2n_cleanup_final should be used instead of s2n_cleanup in cases - * where the user needs full control over when the complete cleanup executes. - * * @returns S2N_SUCCESS on success. S2N_FAILURE on failure */ S2N_API extern int s2n_cleanup_final(void); diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index bab76c8aa16..d8bc929982a 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -37,10 +37,10 @@ static void *s2n_load_dynamic_lib(void *ctx) exit(1); } - int (*s2n_cleanup_dl)(void) = NULL; - *(void **) (&s2n_cleanup_dl) = dlsym(s2n_so, "s2n_cleanup"); + int (*s2n_cleanup_final_dl)(void) = NULL; + *(void **) (&s2n_cleanup_final_dl) = dlsym(s2n_so, "s2n_cleanup_final"); if (dlerror()) { - printf("Error dynamically loading s2n_cleanup\n"); + printf("Error dynamically loading s2n_cleanup_final\n"); exit(1); } @@ -63,17 +63,22 @@ static void *s2n_load_dynamic_lib(void *ctx) fprintf(stderr, "Error calling s2n_init: '%s'\n", (*s2n_strerror_debug_dl)(s2n_errno, "EN")); exit(1); } - if ((*s2n_cleanup_dl)()) { + if ((*s2n_cleanup_final_dl)()) { int s2n_errno = (*s2n_errno_location_dl)(); - fprintf(stderr, "Error calling s2n_cleanup: '%s'\n", (*s2n_strerror_debug_dl)(s2n_errno, "EN")); + fprintf(stderr, "Error calling s2n_cleanup_final: '%s'\n", (*s2n_strerror_debug_dl)(s2n_errno, "EN")); exit(1); } + /* TODO: https://github.com/aws/s2n-tls/issues/4827 + * This dlclose call invokes the pthread key destructor that + * asserts that the s2n-tls library is initialized, which at this point + * is not, due to the s2n_cleanup_final call. This is a bug. if (dlclose(s2n_so)) { printf("Error closing libs2n\n"); printf("%s\n", dlerror()); exit(1); } + */ return NULL; } diff --git a/codebuild/bin/test_exec_leak.sh b/codebuild/bin/test_exec_leak.sh index 6dc50416438..367652546a8 100755 --- a/codebuild/bin/test_exec_leak.sh +++ b/codebuild/bin/test_exec_leak.sh @@ -51,7 +51,7 @@ cat < build/detect_exec_leak_finish.c int main() { s2n_init(); - s2n_cleanup(); + s2n_cleanup_final(); /* close std* file descriptors so valgrind output is less noisy */ fclose(stdin); diff --git a/docs/usage-guide/topics/ch02-initialization.md b/docs/usage-guide/topics/ch02-initialization.md index 467c0f7567c..f4f60e6247b 100644 --- a/docs/usage-guide/topics/ch02-initialization.md +++ b/docs/usage-guide/topics/ch02-initialization.md @@ -1,10 +1,17 @@ # Initialization and Teardown -The s2n-tls library must be initialized with `s2n_init()` before calling most library functions. `s2n_init()` MUST NOT be called more than once, even when an application uses multiple threads or processes. s2n attempts to clean up its thread-local memory at thread-exit and all other memory at process-exit. However, this may not work if you are using a thread library other than pthreads or other threads using s2n outlive the thread that called `s2n_init()`. In that case you should call `s2n_cleanup_thread()` from every thread or process created after `s2n_init()`. -> Note: `s2n_cleanup_thread()` is currently considered unstable, meaning the API is subject to change in a future release. To access this API, include `api/unstable/cleanup.h`. +## Initialization +The s2n-tls library must be initialized with `s2n_init()` before calling most library functions. `s2n_init()` will error if it is called more than once, even when an application uses multiple threads. Initialization can be modified by calling `s2n_crypto_disable_init()` or `s2n_disable_atexit()` before `s2n_init()`. -An application can override s2n-tls’s internal memory management by calling `s2n_mem_set_callbacks` before calling s2n_init. +An application can override s2n-tls’s internal memory management by calling `s2n_mem_set_callbacks()` before calling `s2n_init()`. If you are trying to use FIPS mode, you must enable FIPS in your libcrypto library (probably by calling `FIPS_mode_set(1)`) before calling `s2n_init()`. + +## Teardown +### Thread-local Memory +We recommend calling `s2n_cleanup()` from every thread created after `s2n_init()` to ensure there are no memory leaks. s2n-tls has thread-local memory that it attempts to clean up automatically at thread-exit. However, this is done using pthread destructors and may not work if you are using a threads library other than pthreads. + +### Library Cleanup +A full cleanup and de-initialization of the library can be done by calling `s2n_cleanup_final()`. s2n-tls allocates some memory at initialization that is intended to live for the duration of the process, but can be cleaned up earlier with `s2n_cleanup_final()`. Not calling this method may cause tools like ASAN or valgrind to detect memory leaks, as the memory will still be allocated when the process exits. diff --git a/tests/s2n_test.h b/tests/s2n_test.h index 3ad2c168783..e03b21ff7ba 100644 --- a/tests/s2n_test.h +++ b/tests/s2n_test.h @@ -15,18 +15,18 @@ #pragma once #include +#include #include #include #include #include -#include - #include "error/s2n_errno.h" -#include "utils/s2n_safety.h" -#include "utils/s2n_result.h" #include "tls/s2n_alerts.h" #include "tls/s2n_tls13.h" +#include "utils/s2n_init.h" +#include "utils/s2n_result.h" +#include "utils/s2n_safety.h" int test_count; @@ -64,14 +64,15 @@ bool s2n_use_color_in_output = true; * number of independent childs at the start of a unit test and where you want * each child to have its own independently initialised s2n. */ -#define BEGIN_TEST_NO_INIT() \ - do { \ - test_count = 0; \ - fprintf(stdout, "Running %-50s ... ", __FILE__); \ - fflush(stdout); \ - EXPECT_SUCCESS_WITHOUT_COUNT(s2n_in_unit_test_set(true)); \ - S2N_TEST_OPTIONALLY_ENABLE_FIPS_MODE(); \ - } while(0) +#define BEGIN_TEST_NO_INIT() \ + do { \ + test_count = 0; \ + fprintf(stdout, "Running %-50s ... ", __FILE__); \ + fflush(stdout); \ + EXPECT_SUCCESS_WITHOUT_COUNT(s2n_in_unit_test_set(true)); \ + S2N_TEST_OPTIONALLY_ENABLE_FIPS_MODE(); \ + EXPECT_SUCCESS(s2n_enable_atexit()); \ + } while (0) #define END_TEST_NO_INIT() \ do { \ @@ -261,7 +262,7 @@ void s2n_test__fuzz_cleanup() \ if (fuzz_cleanup) { \ ((void (*)()) fuzz_cleanup)(); \ } \ - s2n_cleanup(); \ + s2n_cleanup_final(); \ } \ int LLVMFuzzerInitialize(int *argc, char **argv[]) \ { \ diff --git a/tests/unit/s2n_init_test.c b/tests/unit/s2n_init_test.c index 091427c86bc..27a7b9f8095 100644 --- a/tests/unit/s2n_init_test.c +++ b/tests/unit/s2n_init_test.c @@ -19,7 +19,6 @@ #include "s2n_test.h" bool s2n_is_initialized(void); -int s2n_enable_atexit(void); static void *s2n_init_fail_cb(void *_unused_arg) { @@ -49,24 +48,18 @@ int main(int argc, char **argv) { BEGIN_TEST_NO_INIT(); - /* Disabling the atexit handler makes it easier for us to test s2n_init and s2n_cleanup - * behavior. Otherwise we'd have to create and exit a bunch of processes to test this - * interaction. */ - EXPECT_SUCCESS(s2n_disable_atexit()); - /* Calling s2n_init twice in a row will cause an error */ EXPECT_SUCCESS(s2n_init()); EXPECT_FAILURE_WITH_ERRNO(s2n_init(), S2N_ERR_INITIALIZED); - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); - /* Second call to s2n_cleanup will fail, since the full cleanup is not idempotent. - * This behavior only exists when atexit is disabled. */ - EXPECT_FAILURE_WITH_ERRNO(s2n_cleanup(), S2N_ERR_NOT_INITIALIZED); + /* Second call to s2n_cleanup_final will fail, since the full cleanup is not idempotent. */ + EXPECT_FAILURE_WITH_ERRNO(s2n_cleanup_final(), S2N_ERR_NOT_INITIALIZED); /* Clean up and init multiple times */ for (size_t i = 0; i < 10; i++) { EXPECT_SUCCESS(s2n_init()); - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); } /* Calling s2n_init again after creating a process will cause an error */ @@ -78,16 +71,16 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_cleanup()); return 0; } - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); /* Calling s2n_init again after creating a thread will cause an error */ EXPECT_SUCCESS(s2n_init()); pthread_t init_thread = { 0 }; EXPECT_EQUAL(pthread_create(&init_thread, NULL, s2n_init_fail_cb, NULL), 0); EXPECT_EQUAL(pthread_join(init_thread, NULL), 0); - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); - /* Calling s2n_init/s2n_cleanup in a different thread than s2n_cleanup_thread is called cleans up properly */ + /* Calling s2n_init/s2n_cleanup_final in a different thread than s2n_cleanup_thread is called cleans up properly */ { EXPECT_SUCCESS(s2n_init()); EXPECT_TRUE(s2n_is_initialized()); @@ -98,10 +91,10 @@ int main(int argc, char **argv) /* Calling s2n_cleanup_thread in the main thread leaves s2n initialized. */ EXPECT_SUCCESS(s2n_cleanup_thread()); EXPECT_TRUE(s2n_is_initialized()); - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); EXPECT_FALSE(s2n_is_initialized()); - /* Second call to s2n_cleanup will fail, since the full cleanup is not idempotent. */ - EXPECT_FAILURE_WITH_ERRNO(s2n_cleanup(), S2N_ERR_NOT_INITIALIZED); + /* Second call to s2n_cleanup_final will fail, since the full cleanup is not idempotent. */ + EXPECT_FAILURE_WITH_ERRNO(s2n_cleanup_final(), S2N_ERR_NOT_INITIALIZED); } /* s2n_cleanup_final */ @@ -112,11 +105,11 @@ int main(int argc, char **argv) EXPECT_SUCCESS(s2n_cleanup_final()); EXPECT_FALSE(s2n_is_initialized()); - /* s2n_cleanup fully cleans up the library when the atexit handler is disabled. - * Therefore, calling s2n_cleanup_final after s2n_cleanup will error */ + /* s2n_cleanup only cleans up thread-local storage. + * Therefore, calling s2n_cleanup_final after s2n_cleanup will succeed */ EXPECT_SUCCESS(s2n_init()); EXPECT_SUCCESS(s2n_cleanup()); - EXPECT_FAILURE_WITH_ERRNO(s2n_cleanup_final(), S2N_ERR_NOT_INITIALIZED); + EXPECT_SUCCESS(s2n_cleanup_final()); /* s2n_cleanup_thread only cleans up thread-local storage. * Therefore calling s2n_cleanup_final after s2n_cleanup_thread will succeed */ @@ -130,8 +123,7 @@ int main(int argc, char **argv) /* Initializing s2n on a child thread without calling s2n_cleanup on that * thread will not result in a memory leak. This is because we register - * thread-local memory to be cleaned up at thread-exit - * and then our atexit handler cleans up the rest at proccess-exit. */ + * thread-local memory to be cleaned up at thread-exit. */ pthread_t init_success_thread = { 0 }; EXPECT_EQUAL(pthread_create(&init_success_thread, NULL, s2n_init_success_cb, NULL), 0); EXPECT_EQUAL(pthread_join(init_success_thread, NULL), 0); diff --git a/tests/unit/s2n_random_test.c b/tests/unit/s2n_random_test.c index 5c031c87be6..802693d6f81 100644 --- a/tests/unit/s2n_random_test.c +++ b/tests/unit/s2n_random_test.c @@ -782,9 +782,8 @@ static int s2n_random_noop_destructor_test_cb(struct random_test_case *test_case static int s2n_random_rand_bytes_after_cleanup_cb(struct random_test_case *test_case) { - s2n_disable_atexit(); EXPECT_SUCCESS(s2n_init()); - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); unsigned char rndbytes[16]; EXPECT_EQUAL(RAND_bytes(rndbytes, sizeof(rndbytes)), 1); @@ -818,8 +817,6 @@ static int s2n_random_rand_bytes_before_init(struct random_test_case *test_case) static int s2n_random_invalid_urandom_fd_cb(struct random_test_case *test_case) { - EXPECT_SUCCESS(s2n_disable_atexit()); - struct s2n_rand_device *dev_urandom = NULL; EXPECT_OK(s2n_rand_get_urandom_for_test(&dev_urandom)); EXPECT_NOT_NULL(dev_urandom); @@ -871,7 +868,7 @@ static int s2n_random_invalid_urandom_fd_cb(struct random_test_case *test_case) EXPECT_TRUE(public_bytes_used > 0); } - EXPECT_SUCCESS(s2n_cleanup()); + EXPECT_SUCCESS(s2n_cleanup_final()); } return S2N_SUCCESS; diff --git a/utils/s2n_init.c b/utils/s2n_init.c index 82608d78e4e..1a94fa2e684 100644 --- a/utils/s2n_init.c +++ b/utils/s2n_init.c @@ -37,7 +37,7 @@ static void s2n_cleanup_atexit(void); static pthread_t main_thread = 0; static bool initialized = false; -static bool atexit_cleanup = true; +static bool atexit_cleanup = false; int s2n_disable_atexit(void) { POSIX_ENSURE(!initialized, S2N_ERR_INITIALIZED); @@ -139,13 +139,7 @@ int s2n_cleanup(void) { POSIX_GUARD(s2n_cleanup_thread()); - /* If this is the main thread and atexit cleanup is disabled, - * perform final cleanup now */ - if (pthread_equal(pthread_self(), main_thread) && !atexit_cleanup) { - POSIX_GUARD(s2n_cleanup_final()); - } - - return 0; + return S2N_SUCCESS; } static void s2n_cleanup_atexit(void) diff --git a/utils/s2n_init.h b/utils/s2n_init.h index 7936b85a76e..6a9deed372c 100644 --- a/utils/s2n_init.h +++ b/utils/s2n_init.h @@ -20,3 +20,4 @@ int s2n_init(void); int s2n_cleanup(void); bool s2n_is_initialized(void); +int s2n_enable_atexit(void);