Changing cleanup behavior

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

tests/unit/s2n_init_test.c

@@ -13,13 +13,14 @@
  * permissions and limitations under the License.
  */
 
+#include "utils/s2n_init.h"
+
 #include <pthread.h>
 
 #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();
 }

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;

utils/s2n_init.c

@@ -37,20 +37,14 @@ 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);
  atexit_cleanup = false;
  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)

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