From 7e2df1aa575146790e1e814e597f93fff0d10cc4 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Thu, 28 Sep 2023 18:34:28 -0400 Subject: [PATCH] Free thread-local OpenSSL state on thread exit OpenSSL 1.0.x automatically allocates a per-thread error queue as needed, but requires that the application call ERR_remove_thread_state() when terminating a thread to prevent the memory from leaking. As the Go runtime may create and terminate threads arbitrarily, the amount of memory leaked has the potential to grow unbounded. Clean up the OpenSSL error queue for a thread upon thread exit. Use the native threading APIs to arrange a callback on thread exit as the Go runtime does not provide any similar facilities itself. No explicit thread-exit handling is needed for OpenSSL 1.1.0 and above as it automatically deallocates thread-local resources without application assistance. See the OPENSSL_thread_stop(3) man page for more information. Signed-off-by: Cory Snider --- goopenssl.h | 2 ++ shims.h | 1 + thread_setup.go | 12 ++++++++++++ thread_setup.h | 4 ++++ thread_setup_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ thread_setup_unix.c | 32 +++++++++++++++++++++++++++++--- thread_setup_windows.c | 37 ++++++++++++++++++++++++++++++++++--- 7 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 thread_setup.go create mode 100644 thread_setup.h create mode 100644 thread_setup_test.go diff --git a/goopenssl.h b/goopenssl.h index d0ffe6cb..a5f390a8 100644 --- a/goopenssl.h +++ b/goopenssl.h @@ -4,6 +4,8 @@ #include "shims.h" +// Suppress warnings about unused parameters. +#define UNUSED(x) (void)(x) static inline void go_openssl_do_leak_check(void) diff --git a/shims.h b/shims.h index 858c47e7..4593aa12 100644 --- a/shims.h +++ b/shims.h @@ -170,6 +170,7 @@ DEFINEFUNC_3_0(unsigned long, ERR_get_error_all, (const char **file, int *line, DEFINEFUNC_RENAMED_1_1(const char *, OpenSSL_version, SSLeay_version, (int type), (type)) \ DEFINEFUNC(void, OPENSSL_init, (void), ()) \ DEFINEFUNC_LEGACY_1_0(void, ERR_load_crypto_strings, (void), ()) \ +DEFINEFUNC_LEGACY_1_0(void, ERR_remove_thread_state, (const GO_CRYPTO_THREADID_PTR tid), (tid)) \ DEFINEFUNC_LEGACY_1_0(int, CRYPTO_num_locks, (void), ()) \ DEFINEFUNC_LEGACY_1_0(int, CRYPTO_THREADID_set_callback, (void (*threadid_func) (GO_CRYPTO_THREADID_PTR)), (threadid_func)) \ DEFINEFUNC_LEGACY_1_0(void, CRYPTO_THREADID_set_numeric, (GO_CRYPTO_THREADID_PTR id, unsigned long val), (id, val)) \ diff --git a/thread_setup.go b/thread_setup.go new file mode 100644 index 00000000..314bf1ef --- /dev/null +++ b/thread_setup.go @@ -0,0 +1,12 @@ +package openssl + +// Go wrappers for testing the thread setup code as _test.go files cannot import "C". + +// #include "thread_setup.h" +import "C" + +// opensslThreadsCleanedUp returns the number of times the thread-local OpenSSL +// state has been cleaned up since the process started. +func opensslThreadsCleanedUp() uint { + return uint(C.go_openssl_threads_cleaned_up) +} diff --git a/thread_setup.h b/thread_setup.h new file mode 100644 index 00000000..98d12f82 --- /dev/null +++ b/thread_setup.h @@ -0,0 +1,4 @@ +#define CRYPTO_LOCK 0x01 + +/* Used by unit tests. */ +extern volatile unsigned int go_openssl_threads_cleaned_up; diff --git a/thread_setup_test.go b/thread_setup_test.go new file mode 100644 index 00000000..57e41aaa --- /dev/null +++ b/thread_setup_test.go @@ -0,0 +1,40 @@ +package openssl + +import ( + "runtime" + "testing" + "time" +) + +func init() { + // The runtime parks the "main" thread of the process on Linux rather of + // terminating it. Lock the main thread to the initial goroutine to ensure + // that the test goroutines will always be scheduled onto non-main threads + // that can be consistently made to terminate on demand. + runtime.LockOSThread() +} + +func TestThreadCleanup(t *testing.T) { + if vMajor > 1 || vMinor > 0 { + t.Skip("explicit thread cleanup is only needed for OpenSSL 1.0.x") + } + + before := opensslThreadsCleanedUp() + done := make(chan struct{}) + go func() { + defer close(done) + // The thread this goroutine is running on will be terminated by the + // runtime when the goroutine exits. + runtime.LockOSThread() + // Checking for errors has the side effect of initializing + // the thread-local OpenSSL error queue. + _ = newOpenSSLError("") + }() + <-done + time.Sleep(100 * time.Millisecond) // Give some time for the thread to terminate. + after := opensslThreadsCleanedUp() + + if n := after - before; n != 1 { + t.Errorf("expected thread cleanup to have run once, but it ran %d times", n) + } +} diff --git a/thread_setup_unix.c b/thread_setup_unix.c index dc2df609..53ea9d03 100644 --- a/thread_setup_unix.c +++ b/thread_setup_unix.c @@ -1,13 +1,17 @@ //go:build unix #include "goopenssl.h" +#include "thread_setup.h" #include - -#define CRYPTO_LOCK 0x01 /* This array will store all of the mutexes available to OpenSSL. */ static pthread_mutex_t *mutex_buf = NULL; - + +static pthread_key_t destructor_key; + +/* Used by unit tests. */ +volatile unsigned int go_openssl_threads_cleaned_up = 0; + static void locking_function(int mode, int n, const char *file, int line) { if (mode & CRYPTO_LOCK) @@ -19,10 +23,32 @@ static void locking_function(int mode, int n, const char *file, int line) static void thread_id(GO_CRYPTO_THREADID_PTR tid) { go_openssl_CRYPTO_THREADID_set_numeric(tid, (unsigned long)pthread_self()); + + // OpenSSL fetches the current thread ID whenever it does anything with the + // per-thread error state, so this function is guaranteed to be executed at + // least once on any thread with associated error state. The thread-local + // variable needs to be set to a non-NULL value so that the destructor will + // be called when the thread exits. The actual value does not matter. + (void) pthread_setspecific(destructor_key, (void*)1); +} + +static void cleanup_thread_state(void *ignored) +{ + UNUSED(ignored); + go_openssl_ERR_remove_thread_state(NULL); + // ERR_remove_thread_state(NULL) in turn calls our registered thread_id + // callback via CRYPTO_THREADID_current(), which sets the thread-local + // variable associated with this destructor to a non-NULL value. We have to + // clear the variable ourselves to prevent pthreads from calling the + // destructor again for the same thread. + (void) pthread_setspecific(destructor_key, NULL); + go_openssl_threads_cleaned_up++; } int go_openssl_thread_setup(void) { + if (pthread_key_create(&destructor_key, cleanup_thread_state) != 0) + return 0; mutex_buf = malloc(go_openssl_CRYPTO_num_locks()*sizeof(pthread_mutex_t)); if (!mutex_buf) return 0; diff --git a/thread_setup_windows.c b/thread_setup_windows.c index 7bc66d80..93281d6c 100644 --- a/thread_setup_windows.c +++ b/thread_setup_windows.c @@ -1,15 +1,19 @@ //go:build windows #include "goopenssl.h" +#include "thread_setup.h" #include #include -#define CRYPTO_LOCK 0x01 - /* This array will store all of the mutexes available to OpenSSL. */ static HANDLE *mutex_buf = NULL; +static DWORD fls_index = FLS_OUT_OF_INDEXES; + +/* Used by unit tests. */ +volatile unsigned int go_openssl_threads_cleaned_up = 0; + static void locking_function(int mode, int n, const char *file, int line) { if (mode & CRYPTO_LOCK) @@ -18,8 +22,33 @@ static void locking_function(int mode, int n, const char *file, int line) ReleaseMutex(mutex_buf[n]); } +static void thread_id(GO_CRYPTO_THREADID_PTR tid) +{ + go_openssl_CRYPTO_THREADID_set_numeric(tid, (unsigned long)GetCurrentThreadId()); + + // OpenSSL fetches the current thread ID whenever it does anything with the + // per-thread error state, so this function is guaranteed to be executed at + // least once on any thread with associated error state. As the Win32 API + // reference documentation is unclear on whether the fiber-local storage + // slot needs to be set to trigger the destructor on thread exit, set it to + // a non-NULL value just in case. + (void) FlsSetValue(fls_index, (void*)1); + go_openssl_threads_cleaned_up++; +} + +static void cleanup_thread_state(void *ignored) +{ + UNUSED(ignored); + go_openssl_ERR_remove_thread_state(NULL); +} + int go_openssl_thread_setup(void) { + // Use the fiber-local storage API to hook a callback on thread exit. + // https://devblogs.microsoft.com/oldnewthing/20191011-00/?p=102989 + fls_index = FlsAlloc(cleanup_thread_state); + if (fls_index == FLS_OUT_OF_INDEXES) + return 0; mutex_buf = malloc(go_openssl_CRYPTO_num_locks()*sizeof(HANDLE)); if (!mutex_buf) return 0; @@ -27,7 +56,9 @@ int go_openssl_thread_setup(void) for (i = 0; i < go_openssl_CRYPTO_num_locks(); i++) mutex_buf[i] = CreateMutex(NULL, FALSE, NULL); go_openssl_CRYPTO_set_locking_callback(locking_function); - // go_openssl_CRYPTO_set_id_callback is not needed on Windows + // go_openssl_CRYPTO_set_id_callback is not strictly needed on Windows // as OpenSSL uses GetCurrentThreadId() by default. + // But we need to piggyback off the callback for our own purposes. + go_openssl_CRYPTO_THREADID_set_callback(thread_id); return 1; }