From 59c6c2860a573521a96a402f8232127ceb27b0f6 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 27 Oct 2016 12:31:17 +0200 Subject: [PATCH 1/3] global: synchronize initialization and shutdown with pthreads When trying to initialize and tear down global data structures from different threads at once with `git_libgit2_init` and `git_libgit2_shutdown`, we race around initializing data. While we use `pthread_once` to assert that we only initilize data a single time, we actually reset the `pthread_once_t` on the last call to `git_libgit2_shutdown`. As resetting this variable is not synchronized with other threads trying to access it, this is actually racy when one thread tries to do a complete shutdown of libgit2 while another thread tries to initialize it. Fix the issue by creating a mutex which synchronizes `init_once` and the library shutdown. --- src/global.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/global.c b/src/global.c index 45b1ab8f6..de722c7e8 100644 --- a/src/global.c +++ b/src/global.c @@ -247,6 +247,7 @@ BOOL WINAPI DllMain(HINSTANCE hInstDll, DWORD fdwReason, LPVOID lpvReserved) #elif defined(GIT_THREADS) && defined(_POSIX_THREADS) static pthread_key_t _tls_key; +static pthread_mutex_t _init_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_once_t _once_init = PTHREAD_ONCE_INIT; int init_error = 0; @@ -268,12 +269,19 @@ static void init_once(void) int git_libgit2_init(void) { - int ret; + int ret, err; ret = git_atomic_inc(&git__n_inits); - pthread_once(&_once_init, init_once); - return init_error ? init_error : ret; + if ((err = pthread_mutex_lock(&_init_mutex)) != 0) + return err; + err = pthread_once(&_once_init, init_once); + err |= pthread_mutex_unlock(&_init_mutex); + + if (err || init_error) + return err | init_error; + + return ret; } int git_libgit2_shutdown(void) @@ -285,6 +293,9 @@ int git_libgit2_shutdown(void) if ((ret = git_atomic_dec(&git__n_inits)) != 0) return ret; + if ((ret = pthread_mutex_lock(&_init_mutex)) != 0) + return ret; + /* Shut down any subsystems that have global state */ shutdown_common(); @@ -298,6 +309,9 @@ int git_libgit2_shutdown(void) git_mutex_free(&git__mwindow_mutex); _once_init = new_once; + if ((ret = pthread_mutex_unlock(&_init_mutex)) != 0) + return ret; + return 0; } From 038f0e1b4cf6333d9835776b72e6bf8fe4975de5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 2 Nov 2016 08:49:24 +0100 Subject: [PATCH 2/3] global: reset global state on shutdown without threading When threading is not enabled for libgit2, we keep global state in a simple static variable. When libgit2 is shut down, we clean up the global state by freeing the global state's dynamically allocated memory. When libgit2 is built with threading, we additionally free the thread-local storage and thus completely remove the global state. In a non-threaded build, though, we simply leave the global state as-is, which may result in an error upon reinitializing libgit2. Fix the issue by zeroing out the variable on a shutdown, thus returning it to its initial state. --- src/global.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/global.c b/src/global.c index de722c7e8..e2ad8fe71 100644 --- a/src/global.c +++ b/src/global.c @@ -341,7 +341,7 @@ int git_libgit2_init(void) { int ret; - /* Only init SSL the first time */ + /* Only init subsystems the first time */ if ((ret = git_atomic_inc(&git__n_inits)) != 1) return ret; @@ -359,6 +359,7 @@ int git_libgit2_shutdown(void) if ((ret = git_atomic_dec(&git__n_inits)) == 0) { shutdown_common(); git__global_state_cleanup(&__state); + memset(&__state, 0, sizeof(__state)); } return ret; From 1c33ecc4456186f4cc6570876ed5c47031b7ef1b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Tue, 1 Nov 2016 14:30:38 +0100 Subject: [PATCH 3/3] tests: core: test deinitialization and concurrent initialization Exercise the logic surrounding deinitialization of the libgit2 library as well as repeated concurrent de- and reinitialization. This tries to catch races and makes sure that it is possible to reinitialize libgit2 multiple times. After deinitializing libgit2, we have to make sure to setup options required for testing. Currently, this only includes setting up the configuration search path again. Before, this has been set up once in `tests/main.c`. --- tests/core/init.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/tests/core/init.c b/tests/core/init.c index e17b7845f..cd90b378d 100644 --- a/tests/core/init.c +++ b/tests/core/init.c @@ -12,3 +12,43 @@ void test_core_init__returns_count(void) cl_assert_equal_i(1, git_libgit2_shutdown()); } +void test_core_init__reinit_succeeds(void) +{ + cl_assert_equal_i(0, git_libgit2_shutdown()); + cl_assert_equal_i(1, git_libgit2_init()); + cl_sandbox_set_search_path_defaults(); +} + +#ifdef GIT_THREADS +static void *reinit(void *unused) +{ + unsigned i; + + for (i = 0; i < 20; i++) { + cl_assert(git_libgit2_init() > 0); + cl_assert(git_libgit2_shutdown() >= 0); + } + + return unused; +} +#endif + +void test_core_init__concurrent_init_succeeds(void) +{ +#ifdef GIT_THREADS + git_thread threads[10]; + unsigned i; + + cl_assert_equal_i(0, git_libgit2_shutdown()); + + for (i = 0; i < ARRAY_SIZE(threads); i++) + git_thread_create(&threads[i], reinit, NULL); + for (i = 0; i < ARRAY_SIZE(threads); i++) + git_thread_join(&threads[i], NULL); + + cl_assert_equal_i(1, git_libgit2_init()); + cl_sandbox_set_search_path_defaults(); +#else + cl_skip(); +#endif +}