Skip to content

Commit 5c6180b

Browse files
committed
global: convert to fiber-local storage to fix exit races
On Windows platforms, we automatically clean up the thread-local storage upon detaching a thread via `DllMain()`. The thing is that this happens for every thread of applications that link against the libgit2 DLL, even those that don't have anything to do with libgit2 itself. As a result, we cannot assume that these unsuspecting threads make use of our `git_libgit2_init()` and `git_libgit2_shutdow()` reference counting, which may lead to racy situations: Thread 1 Thread 2 git_libgit2_shutdown() DllMain(DETACH_THREAD) git__free_tls_data() git_atomic_dec() == 0 git__free_tls_data() TlsFree(_tls_index) TlsGetValue(_tls_index) Due to the second thread never having executed `git_libgit2_init()`, the first thread will clean up TLS data and as a result also free the `_tls_index` variable. When detaching the second thread, we unconditionally access the now-free'd `_tls_index` variable, which is obviously not going to work out well. Fix the issue by converting the code to use fiber-local storage instead of thread-local storage. While FLS will behave the exact same as TLS if no fibers are in use, it does allow us to specify a destructor similar to the one that is accepted by pthread_key_create(3P). Like this, we do not have to manually free indices anymore, but will let the FLS handle calling the destructor. This allows us to get rid of `DllMain()` completely, as we only used it to keep track of when threads were exiting and results in an overall simplification of TLS cleanup.
1 parent 7f20778 commit 5c6180b

File tree

3 files changed

+12
-47
lines changed

3 files changed

+12
-47
lines changed

src/global.c

Lines changed: 12 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -141,14 +141,21 @@ static void shutdown_common(void)
141141
*/
142142
#if defined(GIT_THREADS) && defined(GIT_WIN32)
143143

144-
static DWORD _tls_index;
144+
static DWORD _fls_index;
145145
static volatile LONG _mutex = 0;
146146

147+
static void WINAPI fls_free(void *st)
148+
{
149+
git__global_state_cleanup(st);
150+
git__free(st);
151+
}
152+
147153
static int synchronized_threads_init(void)
148154
{
149155
int error;
150156

151-
_tls_index = TlsAlloc();
157+
if ((_fls_index = FlsAlloc(fls_free)) == FLS_OUT_OF_INDEXES)
158+
return -1;
152159

153160
git_threads_init();
154161

@@ -190,9 +197,7 @@ int git_libgit2_shutdown(void)
190197
if ((ret = git_atomic_dec(&git__n_inits)) == 0) {
191198
shutdown_common();
192199

193-
git__free_tls_data();
194-
195-
TlsFree(_tls_index);
200+
FlsFree(_fls_index);
196201
git_mutex_free(&git__mwindow_mutex);
197202

198203
#if defined(GIT_MSVC_CRTDBG)
@@ -213,7 +218,7 @@ git_global_st *git__global_state(void)
213218

214219
assert(git_atomic_get(&git__n_inits) > 0);
215220

216-
if ((ptr = TlsGetValue(_tls_index)) != NULL)
221+
if ((ptr = FlsGetValue(_fls_index)) != NULL)
217222
return ptr;
218223

219224
ptr = git__calloc(1, sizeof(git_global_st));
@@ -222,43 +227,10 @@ git_global_st *git__global_state(void)
222227

223228
git_buf_init(&ptr->error_buf, 0);
224229

225-
TlsSetValue(_tls_index, ptr);
230+
FlsSetValue(_fls_index, ptr);
226231
return ptr;
227232
}
228233

229-
/**
230-
* Free the TLS data associated with this thread.
231-
* This should only be used by the thread as it
232-
* is exiting.
233-
*/
234-
void git__free_tls_data(void)
235-
{
236-
void *ptr = TlsGetValue(_tls_index);
237-
if (!ptr)
238-
return;
239-
240-
git__global_state_cleanup(ptr);
241-
git__free(ptr);
242-
TlsSetValue(_tls_index, NULL);
243-
}
244-
245-
BOOL WINAPI DllMain(HINSTANCE hInstDll, DWORD fdwReason, LPVOID lpvReserved)
246-
{
247-
GIT_UNUSED(hInstDll);
248-
GIT_UNUSED(lpvReserved);
249-
250-
/* This is how Windows lets us know our thread is being shut down */
251-
if (fdwReason == DLL_THREAD_DETACH) {
252-
git__free_tls_data();
253-
}
254-
255-
/*
256-
* Windows pays attention to this during library loading. We don't do anything
257-
* so we trivially succeed.
258-
*/
259-
return TRUE;
260-
}
261-
262234
#elif defined(GIT_THREADS) && defined(_POSIX_THREADS)
263235

264236
static pthread_key_t _tls_key;

src/global.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@ typedef void (*git_global_shutdown_fn)(void);
3535

3636
extern void git__on_shutdown(git_global_shutdown_fn callback);
3737

38-
extern void git__free_tls_data(void);
39-
4038
extern const char *git_libgit2__user_agent(void);
4139
extern const char *git_libgit2__ssl_ciphers(void);
4240

src/win32/thread.c

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,6 @@ static DWORD WINAPI git_win32__threadproc(LPVOID lpParameter)
3232

3333
thread->result = thread->proc(thread->param);
3434

35-
git__free_tls_data();
36-
3735
return CLEAN_THREAD_EXIT;
3836
}
3937

@@ -103,9 +101,6 @@ void git_thread_exit(void *value)
103101
{
104102
assert(GIT_GLOBAL->current_thread);
105103
GIT_GLOBAL->current_thread->result = value;
106-
107-
git__free_tls_data();
108-
109104
ExitThread(CLEAN_THREAD_EXIT);
110105
}
111106

0 commit comments

Comments
 (0)