From a10cfae3ad9dc430032e39ed780cc529168129f4 Mon Sep 17 00:00:00 2001 From: Appelmans Date: Mon, 5 Jun 2023 16:59:54 -0700 Subject: [PATCH 1/6] Fixed pthread leak --- codebuild/bin/s2n_dynamic_load_test.c | 8 +++++--- utils/s2n_random.c | 8 +++++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index 3cac2d33c54..efe5ae5d1cf 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -17,6 +17,7 @@ #include #include #include +#include static void *s2n_load_dynamic_lib(void *ctx) { @@ -62,10 +63,11 @@ int main(int argc, char *argv[]) /* s2n-tls library can be dynamically loaded and cleaned up safely * - * We can't use any s2n test macros because this test doesn't get linked to - * s2n during compile-time. + * We can't use any s2n test macros because this test doesn't get linked to + * s2n during compile-time. This test is in a loop to make sure that we are + * cleaning up pthread keys properly. */ - { + for (size_t i = 0; i <= PTHREAD_KEYS_MAX; i++) { pthread_t thread_id = { 0 }; if (pthread_create(&thread_id, NULL, &s2n_load_dynamic_lib, argv[1])) { exit(1); diff --git a/utils/s2n_random.c b/utils/s2n_random.c index 98ba8098610..21950ee93b1 100644 --- a/utils/s2n_random.c +++ b/utils/s2n_random.c @@ -169,6 +169,12 @@ S2N_RESULT s2n_get_mix_entropy(struct s2n_blob *blob) return S2N_RESULT_OK; } +/* Deletes pthread key at process-exit */ +static void __attribute__((destructor)) s2n_drbg_rand_state_key_cleanup(void) +{ + (void) pthread_key_delete(s2n_per_thread_rand_state_key); +} + static void s2n_drbg_destructor(void *_unused_argument) { (void) _unused_argument; @@ -178,7 +184,7 @@ static void s2n_drbg_destructor(void *_unused_argument) static void s2n_drbg_make_rand_state_key(void) { - (void) pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor); + RESULT_ENSURE(pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor) == 0, S2N_ERR_DRBG); } static S2N_RESULT s2n_init_drbgs(void) From 2a9dbc8c8a0a491cc359c6ace11139350cfbffe2 Mon Sep 17 00:00:00 2001 From: Appelmans Date: Mon, 5 Jun 2023 17:51:58 -0700 Subject: [PATCH 2/6] Fixed return value --- utils/s2n_random.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utils/s2n_random.c b/utils/s2n_random.c index 21950ee93b1..41832520dea 100644 --- a/utils/s2n_random.c +++ b/utils/s2n_random.c @@ -182,7 +182,7 @@ static void s2n_drbg_destructor(void *_unused_argument) s2n_result_ignore(s2n_rand_cleanup_thread()); } -static void s2n_drbg_make_rand_state_key(void) +static S2N_RESULT s2n_drbg_make_rand_state_key(void) { RESULT_ENSURE(pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor) == 0, S2N_ERR_DRBG); } From 86ac48c80c13f150934715d5b078f9094ff74dd3 Mon Sep 17 00:00:00 2001 From: Appelmans Date: Tue, 6 Jun 2023 12:50:38 -0700 Subject: [PATCH 3/6] Removes test for this issue --- codebuild/bin/s2n_dynamic_load_test.c | 6 ++---- utils/s2n_random.c | 6 ++++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index efe5ae5d1cf..59283e0ddfa 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -17,7 +17,6 @@ #include #include #include -#include static void *s2n_load_dynamic_lib(void *ctx) { @@ -64,10 +63,9 @@ int main(int argc, char *argv[]) /* s2n-tls library can be dynamically loaded and cleaned up safely * * We can't use any s2n test macros because this test doesn't get linked to - * s2n during compile-time. This test is in a loop to make sure that we are - * cleaning up pthread keys properly. + * s2n during compile-time. */ - for (size_t i = 0; i <= PTHREAD_KEYS_MAX; i++) { + { pthread_t thread_id = { 0 }; if (pthread_create(&thread_id, NULL, &s2n_load_dynamic_lib, argv[1])) { exit(1); diff --git a/utils/s2n_random.c b/utils/s2n_random.c index 41832520dea..d0c197ac9e3 100644 --- a/utils/s2n_random.c +++ b/utils/s2n_random.c @@ -182,9 +182,11 @@ static void s2n_drbg_destructor(void *_unused_argument) s2n_result_ignore(s2n_rand_cleanup_thread()); } -static S2N_RESULT s2n_drbg_make_rand_state_key(void) +static void s2n_drbg_make_rand_state_key(void) { - RESULT_ENSURE(pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor) == 0, S2N_ERR_DRBG); + /* We can't error check the output of pthread_key_create since s2n_drbg_make_rand_state_key + * isn't allowed to return a value due to the parameter constraints of pthread_once. */ + (void) pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor); } static S2N_RESULT s2n_init_drbgs(void) From d43080790d5e0d0059a13e1fd3208b01fda0aa97 Mon Sep 17 00:00:00 2001 From: Appelmans Date: Tue, 6 Jun 2023 15:34:21 -0700 Subject: [PATCH 4/6] Added dev guide explaination --- codebuild/bin/s2n_dynamic_load_test.c | 2 +- docs/DEVELOPMENT-GUIDE.md | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index 59283e0ddfa..3cac2d33c54 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -62,7 +62,7 @@ int main(int argc, char *argv[]) /* s2n-tls library can be dynamically loaded and cleaned up safely * - * We can't use any s2n test macros because this test doesn't get linked to + * We can't use any s2n test macros because this test doesn't get linked to * s2n during compile-time. */ { diff --git a/docs/DEVELOPMENT-GUIDE.md b/docs/DEVELOPMENT-GUIDE.md index 7d88095d2c6..09ee50f4678 100644 --- a/docs/DEVELOPMENT-GUIDE.md +++ b/docs/DEVELOPMENT-GUIDE.md @@ -156,6 +156,14 @@ As discussed below, s2n-tls rarely allocates resources, and so has nothing to cl `DEFER_CLEANUP(_thealloc, _thecleanup)` is a failsafe way of ensuring that resources are cleaned up, using the ` __attribute__((cleanup())` destructor mechanism available in modern C compilers. When the variable declared in `_thealloc` goes out of scope, the cleanup function `_thecleanup` is automatically called. This guarantees that resources will be cleaned up, no matter how the function exits. +### Lifecycle of s2n memory +s2n states publicly that every `s2n_init()` call should be paired with an `s2n_cleanup()` call, but we also attempt to do some auto-cleanup behind the scenes because we know not every s2n-user can actually follow those steps. Unfortunately, that auto-cleanup has also caused issues because it’s not very well documented and is not guaranteed to work. Here is our general philosophy behind the auto-clean behavior. + +For every thread that s2n functions are called in, a small amount of thread-local memory also gets initialized. This is to ensure that our random number generator will output different numbers in different threads. This memory needs to be cleaned up per thread and users can do this themselves if they call `s2n_cleanup()` per thread. But if they forget, we utilize a pthread key that calls a destructor function that cleans up our thread-local memory when the thread closes. + +An important thing to note is that a call to `s2n_cleanup()` usually does not fully clean up s2n. It only cleans up the thread-local memory. This is because we have an atexit handler that does fully clean up s2n at process-exit. +The behavior is different if the atexit handler is disabled by calling `s2n_disable_atexit()`. Then s2n is actually fully cleaned up if `s2n_cleanup()` is called on the thread that called `s2n_init()`. + ### Control flow and the state machine Branches can be a source of cognitive load, as they ask the reader to follow a path of thinking, while also remembering that there is another path to be explored. When branches are nested, they can often lead to impossible to grasp combinatorial explosions. s2n-tls tries to systematically reduce the number of branches used in the code in several ways. From 7e23bc373542ddd7f3b6fabff9f570d6fa53369b Mon Sep 17 00:00:00 2001 From: Appelmans Date: Wed, 7 Jun 2023 14:37:04 -0700 Subject: [PATCH 5/6] Added test back in --- codebuild/bin/s2n_dynamic_load_test.c | 6 ++++-- utils/s2n_random.c | 9 ++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index 3cac2d33c54..d879ed78ecf 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -17,6 +17,7 @@ #include #include #include +#include static void *s2n_load_dynamic_lib(void *ctx) { @@ -63,9 +64,10 @@ int main(int argc, char *argv[]) /* s2n-tls library can be dynamically loaded and cleaned up safely * * We can't use any s2n test macros because this test doesn't get linked to - * s2n during compile-time. + * s2n during compile-time. This test is in a loop to make sure that we are + * cleaning up pthread keys properly. */ - { + for (size_t i = 0; i <= PTHREAD_KEYS_MAX; i++) { pthread_t thread_id = { 0 }; if (pthread_create(&thread_id, NULL, &s2n_load_dynamic_lib, argv[1])) { exit(1); diff --git a/utils/s2n_random.c b/utils/s2n_random.c index d0c197ac9e3..ef4e34ec8c4 100644 --- a/utils/s2n_random.c +++ b/utils/s2n_random.c @@ -100,6 +100,8 @@ struct s2n_rand_state { static pthread_key_t s2n_per_thread_rand_state_key; /* Needed to ensure key is initialized only once */ static pthread_once_t s2n_per_thread_rand_state_key_once = PTHREAD_ONCE_INIT; +/* Tracks if call to pthread_key_create failed */ +static int pthread_key_create_result; static __thread struct s2n_rand_state s2n_per_thread_rand_state = { .cached_fork_generation_number = 0, @@ -184,9 +186,9 @@ static void s2n_drbg_destructor(void *_unused_argument) static void s2n_drbg_make_rand_state_key(void) { - /* We can't error check the output of pthread_key_create since s2n_drbg_make_rand_state_key - * isn't allowed to return a value due to the parameter constraints of pthread_once. */ - (void) pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor); + /* We can't return the output of pthread_key_create due to the parameter constraints of pthread_once. + * Here we store the result in a global variable that will be error checked later. */ + pthread_key_create_result = pthread_key_create(&s2n_per_thread_rand_state_key, s2n_drbg_destructor); } static S2N_RESULT s2n_init_drbgs(void) @@ -199,6 +201,7 @@ static S2N_RESULT s2n_init_drbgs(void) RESULT_GUARD_POSIX(s2n_blob_init(&private, s2n_private_drbg, sizeof(s2n_private_drbg))); RESULT_ENSURE(pthread_once(&s2n_per_thread_rand_state_key_once, s2n_drbg_make_rand_state_key) == 0, S2N_ERR_DRBG); + RESULT_ENSURE_EQ(pthread_key_create_result, 0); RESULT_GUARD(s2n_drbg_instantiate(&s2n_per_thread_rand_state.public_drbg, &public, S2N_AES_128_CTR_NO_DF_PR)); RESULT_GUARD(s2n_drbg_instantiate(&s2n_per_thread_rand_state.private_drbg, &private, S2N_AES_256_CTR_NO_DF_PR)); From 4c63139cd04777e0a286512d744300f4d8398ff0 Mon Sep 17 00:00:00 2001 From: Appelmans Date: Thu, 8 Jun 2023 12:45:08 -0700 Subject: [PATCH 6/6] Tweaked loop limit --- codebuild/bin/s2n_dynamic_load_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index d879ed78ecf..10288487e3e 100644 --- a/codebuild/bin/s2n_dynamic_load_test.c +++ b/codebuild/bin/s2n_dynamic_load_test.c @@ -67,7 +67,7 @@ int main(int argc, char *argv[]) * s2n during compile-time. This test is in a loop to make sure that we are * cleaning up pthread keys properly. */ - for (size_t i = 0; i <= PTHREAD_KEYS_MAX; i++) { + for (size_t i = 0; i <= PTHREAD_KEYS_MAX + 1; i++) { pthread_t thread_id = { 0 }; if (pthread_create(&thread_id, NULL, &s2n_load_dynamic_lib, argv[1])) { exit(1);