Fixing thread exit and thread cancellation (#12985) (#10524)

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]>

Author: abujalski

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);
     }

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())) {

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;
 	}
 

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 <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;
+}

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

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')

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',