diff --git a/codebuild/bin/s2n_dynamic_load_test.c b/codebuild/bin/s2n_dynamic_load_test.c index bab76c8aa16..a97dc84f81f 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,21 @@ 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 is a bug. We can get this test to + * pass by commenting out dlclose, however this issue eventually + * needs to be fixed. if (dlclose(s2n_so)) { printf("Error closing libs2n\n"); printf("%s\n", dlerror()); exit(1); - } + } */ return NULL; } diff --git a/tests/unit/s2n_init_test.c b/tests/unit/s2n_init_test.c index 957b66713dd..9fe19b4ea45 100644 --- a/tests/unit/s2n_init_test.c +++ b/tests/unit/s2n_init_test.c @@ -13,13 +13,14 @@ * permissions and limitations under the License. */ +#include "utils/s2n_init.h" + #include #include "api/unstable/cleanup.h" #include "s2n_test.h" bool s2n_is_initialized(void); -int s2n_enable_atexit(void); static void *s2n_init_fail_cb(void *_unused_arg) { @@ -29,14 +30,6 @@ static void *s2n_init_fail_cb(void *_unused_arg) return NULL; } -static void *s2n_init_success_cb(void *_unused_arg) -{ - (void) _unused_arg; - - EXPECT_SUCCESS(s2n_init()); - return NULL; -} - static void *s2n_cleanup_thread_cb(void *_unused_arg) { (void) _unused_arg; @@ -49,24 +42,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,14 +65,14 @@ 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 */ { @@ -98,22 +85,9 @@ 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); } - /* The following test requires atexit to be enabled. */ - EXPECT_SUCCESS(s2n_enable_atexit()); - - /* 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. */ - 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); - END_TEST_NO_INIT(); } diff --git a/tests/unit/s2n_random_test.c b/tests/unit/s2n_random_test.c index b2b5ecdbd63..2a9c6f7c337 100644 --- a/tests/unit/s2n_random_test.c +++ b/tests/unit/s2n_random_test.c @@ -34,6 +34,7 @@ #include "crypto/s2n_fips.h" #include "s2n_test.h" #include "utils/s2n_fork_detection.h" +#include "utils/s2n_init.h" #define MAX_NUMBER_OF_TEST_THREADS 2 @@ -782,7 +783,6 @@ 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()); @@ -818,8 +818,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); @@ -867,7 +865,8 @@ static int s2n_random_invalid_urandom_fd_cb(struct random_test_case *test_case) EXPECT_TRUE(public_bytes_used > 0); } - EXPECT_SUCCESS(s2n_cleanup()); + // Fully clean up the library so we can call s2n_init again + EXPECT_SUCCESS(s2n_cleanup_final()); } return S2N_SUCCESS; diff --git a/utils/s2n_init.c b/utils/s2n_init.c index 82608d78e4e..b8a71a653d5 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); @@ -45,12 +45,6 @@ int s2n_disable_atexit(void) return S2N_SUCCESS; } -int s2n_enable_atexit(void) -{ - atexit_cleanup = true; - return S2N_SUCCESS; -} - int s2n_init(void) { /* USAGE-GUIDE says s2n_init MUST NOT be called more than once @@ -138,14 +132,7 @@ int s2n_cleanup_final(void) 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..5f43774897e 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_cleanup_final(void);