From a342a1dba1b28d6bd230455a667ab96c50c4c845 Mon Sep 17 00:00:00 2001 From: Adam Bujalski Date: Fri, 26 Apr 2019 13:34:26 +0200 Subject: [PATCH] Fixing thread exit and thread cancellation (#12985) 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 --- src/library_pthread.js | 68 +++++++++------ system/lib/libc/musl/src/thread/__timedwait.c | 7 +- .../musl/src/thread/pthread_barrier_wait.c | 21 ++++- .../pthread/test_pthread_cancel_cond_wait.cpp | 87 +++++++++++++++++++ tests/test_browser.py | 5 ++ tests/test_core.py | 1 - tests/test_posixtest.py | 3 - 7 files changed, 158 insertions(+), 34 deletions(-) create mode 100644 tests/pthread/test_pthread_cancel_cond_wait.cpp diff --git a/src/library_pthread.js b/src/library_pthread.js index 81470bc17d29..e49372a4cd99 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js @@ -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) { @@ -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(); - - _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. + PThread.runExitHandlersAndDeinitThread(tb, exitCode); + 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. @@ -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' }); }, @@ -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); } diff --git a/system/lib/libc/musl/src/thread/__timedwait.c b/system/lib/libc/musl/src/thread/__timedwait.c index 71abe8e775b6..433f1d00c630 100644 --- a/system/lib/libc/musl/src/thread/__timedwait.c +++ b/system/lib/libc/musl/src/thread/__timedwait.c @@ -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())) { diff --git a/system/lib/libc/musl/src/thread/pthread_barrier_wait.c b/system/lib/libc/musl/src/thread/pthread_barrier_wait.c index 46b856619829..8e212f009080 100644 --- a/system/lib/libc/musl/src/thread/pthread_barrier_wait.c +++ b/system/lib/libc/musl/src/thread/pthread_barrier_wait.c @@ -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; } diff --git a/tests/pthread/test_pthread_cancel_cond_wait.cpp b/tests/pthread/test_pthread_cancel_cond_wait.cpp new file mode 100644 index 000000000000..a53ffa9459d4 --- /dev/null +++ b/tests/pthread/test_pthread_cancel_cond_wait.cpp @@ -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 +#include +#include +#include +#include +#include +#include +#include +#include + +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(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; +} diff --git a/tests/test_browser.py b/tests/test_browser.py index 74322a969ed3..eae0b660d4ee 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py @@ -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 diff --git a/tests/test_core.py b/tests/test_core.py index e569327f6707..d79077879164 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -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') diff --git a/tests/test_posixtest.py b/tests/test_posixtest.py index 1820eb3f9912..6e1cc45e9c69 100644 --- a/tests/test_posixtest.py +++ b/tests/test_posixtest.py @@ -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',