Skip to content

Commit

Permalink
feat: Reworking cleanup behavior (#4871)
Browse files Browse the repository at this point in the history
  • Loading branch information
maddeleine authored Nov 12, 2024
1 parent 0f96548 commit 493b771
Show file tree
Hide file tree
Showing 9 changed files with 56 additions and 65 deletions.
10 changes: 2 additions & 8 deletions api/s2n.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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);
Expand Down
15 changes: 10 additions & 5 deletions codebuild/bin/s2n_dynamic_load_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion codebuild/bin/test_exec_leak.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 10 additions & 3 deletions docs/usage-guide/topics/ch02-initialization.md
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 14 additions & 13 deletions tests/s2n_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 { \
Expand Down Expand Up @@ -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[]) \
{ \
Expand Down
36 changes: 14 additions & 22 deletions tests/unit/s2n_init_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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 */
Expand All @@ -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());
Expand All @@ -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 */
Expand All @@ -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 */
Expand All @@ -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);
Expand Down
7 changes: 2 additions & 5 deletions tests/unit/s2n_random_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 2 additions & 8 deletions utils/s2n_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions utils/s2n_init.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@
int s2n_init(void);
int s2n_cleanup(void);
bool s2n_is_initialized(void);
int s2n_enable_atexit(void);

0 comments on commit 493b771

Please sign in to comment.