feat: Reworking cleanup behavior (#4871)

Author: maddeleine

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);

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;
 }

codebuild/bin/test_exec_leak.sh

@@ -51,7 +51,7 @@ cat <<EOF > 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);

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.

tests/s2n_test.h

@@ -15,18 +15,18 @@
 
 #pragma once
 #include <errno.h>
+#include <openssl/crypto.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 
-#include <openssl/crypto.h>
-
 #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[]) \
 { \

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);

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;

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)

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);