Skip to content

Commit

Permalink
Fixing thread exit and thread cancellation (#12985)
Browse files Browse the repository at this point in the history
This patch fixes thread cancellation/exit method and also unifies why
how thread structures are updated when thread is canceled or exits.

Following problems are addressed:
1. heap-use-after-free on std::thread #12985
   Function `cleanupThread()` from `src/library_pthread.js`
   unconditionally set `pthread.self` member of pthread structure to 0.
   Problem was that `cleanupThread()` function might be called after
   that pthread structure has been deleted. To fix this problem, setting
   `pthread.self` field is now guarded by check if thread data hasn't
   been already freed.
2. pthread_cond_wait/2-3 test hang
  - Disabling recursive thread cancellation.
  - Allowing __timedwait_cp to be true cancellation point as _cp
    suffix suggests
3. pthread_getschedparam/1-3 test hangs sometimes:
   In pthread_barrier_wait adding check if lock is held by main thread
   and waiting on futex in small slices of time there, to check if
   there is some work to do on behalf of Worker Threads.

Signed-off-by: Adam Bujalski <[email protected]>
  • Loading branch information
abujalski committed Feb 16, 2021
1 parent a38a72a commit 610f958
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 33 deletions.
66 changes: 41 additions & 25 deletions src/library_pthread.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,30 @@ var LibraryPThread = {
if (ENVIRONMENT_IS_PTHREAD && _pthread_self()) ___pthread_tsd_run_dtors();
},

runExitHandlersAndDeinitThread: function(tb, exitCode) {
#if PTHREADS_PROFILING
var profilerBlock = Atomics.load(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2);
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2, 0);
_free(profilerBlock);
#endif

// Disable all cancellation so that executing the cleanup handlers won't trigger another JS
// canceled exception to be thrown.
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.canceldisable }}} ) >> 2, 1/*PTHREAD_CANCEL_DISABLE*/);
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.cancelasync }}} ) >> 2, 0/*PTHREAD_CANCEL_DEFERRED*/);
PThread.runExitHandlers();

Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, exitCode);
// When we publish this, the main thread is free to deallocate the thread object and we are done.
// Therefore set _pthread_self = 0; above to 'release' the object in this worker thread.
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1); // Mark the thread as no longer running.

_emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}}); // wake all threads

// Not hosting a pthread anymore in this worker, reset the info structures to null.
__emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
},

// Called when we are performing a pthread_exit(), either explicitly called
// by programmer, or implicitly when leaving the thread main function.
threadExit: function(exitCode) {
Expand All @@ -181,24 +205,8 @@ var LibraryPThread = {
#if ASSERTIONS
err('Pthread 0x' + tb.toString(16) + ' exited.');
#endif
#if PTHREADS_PROFILING
var profilerBlock = Atomics.load(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2);
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2, 0);
_free(profilerBlock);
#endif
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, exitCode);
// When we publish this, the main thread is free to deallocate the thread object and we are done.
// Therefore set _pthread_self = 0; above to 'release' the object in this worker thread.
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1);

// Disable all cancellation so that executing the cleanup handlers won't trigger another JS
// canceled exception to be thrown.
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.canceldisable }}} ) >> 2, 1/*PTHREAD_CANCEL_DISABLE*/);
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.cancelasync }}} ) >> 2, 0/*PTHREAD_CANCEL_DEFERRED*/);
PThread.runExitHandlers();
PThread.runExitHandlersAndDeinitThread(tb, exitCode);

_emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}});
__emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
if (ENVIRONMENT_IS_PTHREAD) {
// Note: in theory we would like to return any offscreen canvases back to the main thread,
// but if we ever fetched a rendering context for them that would not be valid, so we don't try.
Expand All @@ -208,13 +216,7 @@ var LibraryPThread = {
},

threadCancel: function() {
PThread.runExitHandlers();
var tb = _pthread_self();
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, -1/*PTHREAD_CANCELED*/);
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1); // Mark the thread as no longer running.
_emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}}); // wake all threads
// Not hosting a pthread anymore in this worker, reset the info structures to null.
__emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
PThread.runExitHandlersAndDeinitThread(_pthread_self(), -1/*PTHREAD_CANCELED*/);
postMessage({ 'cmd': 'cancelDone' });
},

Expand Down Expand Up @@ -494,9 +496,23 @@ var LibraryPThread = {
$cleanupThread: function(pthread_ptr) {
if (ENVIRONMENT_IS_PTHREAD) throw 'Internal Error! cleanupThread() can only ever be called from main application thread!';
if (!pthread_ptr) throw 'Internal Error! Null pthread_ptr in cleanupThread!';
{{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
var pthread = PThread.pthreads[pthread_ptr];
// If pthread has been removed from this map this also means that pthread_ptr points
// to already freed data. Such situation may occur in following circumstances:
// 1. Joining cancelled thread - in such situation it may happen that pthread data will
// already be removed by handling 'cancelDone' message.
// 2. Joining thread from non-main browser thread (this also includes thread running main()
// when compiled with `PROXY_TO_PTHREAD`) - in such situation it may happen that following
// code flow occur (MB - Main Browser Thread, S1, S2 - Worker Threads):
// S2: thread ends, 'exit' message is sent to MB
// S1: calls pthread_join(S2), this causes:
// a. S2 is marked as detached,
// b. 'cleanupThread' message is sent to MB.
// MB: handles 'exit' message, as thread is detached, so returnWorkerToPool()
// is called and all thread related structs are freed/released.
// MB: handles 'cleanupThread' message which calls this function.
if (pthread) {
{{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
var worker = pthread.worker;
PThread.returnWorkerToPool(worker);
}
Expand Down
7 changes: 6 additions & 1 deletion system/lib/libc/musl/src/thread/__timedwait.c
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ int __timedwait_cp(volatile int *addr, int val,
#ifdef __EMSCRIPTEN__
double msecsToSleep = top ? (top->tv_sec * 1000 + top->tv_nsec / 1000000.0) : INFINITY;
int is_main_thread = emscripten_is_main_browser_thread();
if (is_main_thread || pthread_self()->cancelasync == PTHREAD_CANCEL_ASYNCHRONOUS) {
// cp suffix in the function name means "cancellation point", so this wait can be cancelled
// by the users unless current threads cancelability is set to PTHREAD_CANCEL_DISABLE
// which may be either done by the user of __timedwait() function.
if (is_main_thread ||
pthread_self()->canceldisable != PTHREAD_CANCEL_DISABLE ||
pthread_self()->cancelasync == PTHREAD_CANCEL_ASYNCHRONOUS) {
double sleepUntilTime = emscripten_get_now() + msecsToSleep;
do {
if (_pthread_isduecanceled(pthread_self())) {
Expand Down
21 changes: 18 additions & 3 deletions system/lib/libc/musl/src/thread/pthread_barrier_wait.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,29 @@ int pthread_barrier_wait(pthread_barrier_t *b)
while (spins-- && !inst->finished)
a_spin();
a_inc(&inst->finished);
while (inst->finished == 1) {
#ifdef __EMSCRIPTEN__
emscripten_futex_wait(&inst->finished, 1, INFINITY);
int is_main_thread = emscripten_is_main_browser_thread();
while (inst->finished == 1) {
if (is_main_thread) {
int e;
do {
// Main thread waits in _very_ small slices so that it stays responsive to assist proxied
// pthread calls.
e = emscripten_futex_wait(&inst->finished, 1, 1);
// Assist other threads by executing proxied operations that are effectively singlethreaded.
emscripten_main_thread_process_queued_calls();
} while(e == -ETIMEDOUT);
} else {
// Can wait in one go.
emscripten_futex_wait(&inst->finished, 1, INFINITY);
}
}
#else
while (inst->finished == 1) {
__syscall(SYS_futex,&inst->finished,FUTEX_WAIT|128,1,0) != -ENOSYS
|| __syscall(SYS_futex,&inst->finished,FUTEX_WAIT,1,0);
#endif
}
#endif
return PTHREAD_BARRIER_SERIAL_THREAD;
}

Expand Down
87 changes: 87 additions & 0 deletions tests/pthread/test_pthread_cancel_cond_wait.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Copyright 2021 The Emscripten Authors. All rights reserved.
// Emscripten is available under two separate licenses, the MIT license and the
// University of Illinois/NCSA Open Source License. Both these licenses can be
// found in the LICENSE file.

#include <pthread.h>
#include <sys/types.h>
#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
#include <unistd.h>
#include <errno.h>
#include <emscripten.h>
#include <emscripten/threading.h>

pthread_barrier_t barrier;
pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
pthread_cond_t condvar = PTHREAD_COND_INITIALIZER;

int th_cancelled = 0;

volatile int res = 43;

static void cleanup_handler(void *arg) {
emscripten_log(EM_LOG_CONSOLE, "Called clean-up handler with arg %p", arg);
int a = reinterpret_cast<int>(arg);
res -= a;

pthread_mutex_unlock(&mutex);
pthread_barrier_wait(&barrier);
}

static void *thread_start(void *arg) {
pthread_cleanup_push(cleanup_handler, (void*)42);
emscripten_log(EM_LOG_CONSOLE, "Thread started!");
pthread_mutex_lock(&mutex);
pthread_barrier_wait(&barrier);

int ret = 0;
do {
emscripten_log(EM_LOG_CONSOLE, "Waiting on conditional variable");
ret = pthread_cond_wait(&condvar, &mutex);
} while (ret == 0 && th_cancelled == 0);

if (ret != 0) {
emscripten_log(EM_LOG_CONSOLE, "Cond wait failed ret: %d", ret);
}

res = 1000; // Shouldn't ever reach here.
pthread_cleanup_pop(0);

pthread_mutex_unlock(&mutex);
pthread_barrier_wait(&barrier);
return NULL;
}

int main() {
if (!emscripten_has_threading_support()) {
printf("Skipped: Threading is not supported.\n");
return 1;
}

pthread_barrier_init(&barrier, nullptr, 2);

pthread_t thr;
int s = pthread_create(&thr, NULL, thread_start, (void*)0);
assert(s == 0);
emscripten_log(EM_LOG_CONSOLE, "Thread created");

pthread_barrier_wait(&barrier);

// Lock mutex to ensure that thread is waiting
pthread_mutex_lock(&mutex);

emscripten_log(EM_LOG_CONSOLE, "Canceling thread..");
s = pthread_cancel(thr);
assert(s == 0);
th_cancelled = 1;
pthread_mutex_unlock(&mutex);

emscripten_log(EM_LOG_CONSOLE, "Main thread waitnig for side-thread");
pthread_barrier_wait(&barrier);
pthread_barrier_destroy(&barrier);

emscripten_log(EM_LOG_CONSOLE, "Test finished result: %d", res);
return res;
}
5 changes: 5 additions & 0 deletions tests/test_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -3771,6 +3771,11 @@ def test_std_thread_detach(self):
def test_pthread_cancel(self):
self.btest(path_from_root('tests', 'pthread', 'test_pthread_cancel.cpp'), expected='1', args=['-O3', '-s', 'USE_PTHREADS', '-s', 'PTHREAD_POOL_SIZE=8'])

# Test that pthread_cancel() cancels pthread_cond_wait() operation
@requires_threads
def test_pthread_cancel_cond_wait(self):
self.btest_exit(path_from_root('tests', 'pthread', 'test_pthread_cancel_cond_wait.cpp'), expected='1', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=8'])

# Test pthread_kill() operation
@no_chrome('pthread_kill hangs chrome renderer, and keep subsequent tests from passing')
@requires_threads
Expand Down
1 change: 0 additions & 1 deletion tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -8148,7 +8148,6 @@ def test_pthread_c11_threads(self):
self.set_setting('INITIAL_MEMORY', '64mb')
self.do_run_in_out_file_test('tests', 'pthread', 'test_pthread_c11_threads.c')

@no_asan('flakey errors that must be fixed, https://github.com/emscripten-core/emscripten/issues/12985')
@node_pthreads
def test_pthread_cxx_threads(self):
self.set_setting('PROXY_TO_PTHREAD')
Expand Down
3 changes: 0 additions & 3 deletions tests/test_posixtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,7 @@ def get_pthread_tests():
**flaky,
'test_pthread_create_11_1': 'never returns',
'test_pthread_barrier_wait_2_1': 'never returns',
'test_pthread_cond_timedwait_2_6': 'never returns',
'test_pthread_cond_timedwait_4_3': 'never returns',
'test_pthread_attr_setscope_5_1': 'internally skipped (PTS_UNTESTED)',
'test_pthread_cond_wait_2_3': 'never returns',
'test_pthread_create_5_1': 'never returns',
'test_pthread_exit_1_2': 'never returns',
'test_pthread_exit_2_2': 'never returns',
Expand Down

0 comments on commit 610f958

Please sign in to comment.