From 99db501ed95a232411a753ada319faac4619daf4 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Fri, 14 Apr 2023 21:42:56 +0900 Subject: [PATCH 1/2] Fix races around pthread exit and join --- Makefile | 1 + .../musl/src/thread/pthread_create.c | 22 +++++++------------ .../src/thread/wasm32/wasi_thread_start.s | 16 ++++++++++++++ 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/Makefile b/Makefile index 9160860bc..31fa76d4f 100644 --- a/Makefile +++ b/Makefile @@ -325,6 +325,7 @@ ifeq ($(THREAD_MODEL), posix) # Specify the tls-model until LLVM 15 is released (which should contain # https://reviews.llvm.org/D130053). CFLAGS += -mthread-model posix -pthread -ftls-model=local-exec +ASMFLAGS += -matomics # Include cloudlib's directory to access the structure definition of clockid_t CFLAGS += -I$(LIBC_BOTTOM_HALF_CLOUDLIBC_SRC) diff --git a/libc-top-half/musl/src/thread/pthread_create.c b/libc-top-half/musl/src/thread/pthread_create.c index 676e2ccf9..12094d634 100644 --- a/libc-top-half/musl/src/thread/pthread_create.c +++ b/libc-top-half/musl/src/thread/pthread_create.c @@ -164,14 +164,6 @@ static void __pthread_exit(void *result) self->prev->next = self->next; self->prev = self->next = self; -#ifndef __wasilibc_unmodified_upstream - /* On Linux, the thread is created with CLONE_CHILD_CLEARTID, - * and this lock will unlock by kernel when this thread terminates. - * So we should unlock it here in WebAssembly. - * See also set_tid_address(2) */ - __tl_unlock(); -#endif - #ifdef __wasilibc_unmodified_upstream if (state==DT_DETACHED && self->map_base) { /* Detached threads must block even implementation-internal @@ -190,9 +182,6 @@ static void __pthread_exit(void *result) } #else if (state==DT_DETACHED && self->map_base) { - // __syscall(SYS_exit) would unlock the thread, list - // do it manually here - __tl_unlock(); free(self->map_base); // Can't use `exit()` here, because it is too high level return; @@ -212,10 +201,15 @@ static void __pthread_exit(void *result) #ifdef __wasilibc_unmodified_upstream for (;;) __syscall(SYS_exit, 0); #else - // __syscall(SYS_exit) would unlock the thread, list - // do it manually here - __tl_unlock(); // Can't use `exit()` here, because it is too high level + + /* On Linux, the thread is created with CLONE_CHILD_CLEARTID, + * and the lock (__thread_list_lock) will be unlocked by kernel when + * this thread terminates. + * See also set_tid_address(2) + * + * In WebAssembly, we leave it to wasi_thread_start instead. + */ #endif } diff --git a/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s b/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s index 0fe9854e9..26daab0ae 100644 --- a/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s +++ b/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s @@ -28,4 +28,20 @@ wasi_thread_start: local.get 1 # start_arg call __wasi_thread_start_C + # Unlock thread list. (as CLONE_CHILD_CLEARTID would do for Linux) + # + # Note: once we unlock the thread list, our "map_base" can be freed + # by a joining thread. It's safe as we are in ASM and no longer use + # our C stack or pthread_t. + i32.const __thread_list_lock + i32.const 0 + i32.atomic.store 0 + # As an optimization, we can check tl_lock_waiters here. + # But for now, simply wake up unconditionally as + # CLONE_CHILD_CLEARTID does. + i32.const __thread_list_lock + i32.const 1 + memory.atomic.notify 0 + drop + end_function From 7262ca443802acd6e37d2fc380f31f4a74f51f78 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Wed, 31 May 2023 09:16:16 +0900 Subject: [PATCH 2/2] wasi_thread_start: add a comment --- libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s b/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s index 26daab0ae..7a480b837 100644 --- a/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s +++ b/libc-top-half/musl/src/thread/wasm32/wasi_thread_start.s @@ -32,7 +32,8 @@ wasi_thread_start: # # Note: once we unlock the thread list, our "map_base" can be freed # by a joining thread. It's safe as we are in ASM and no longer use - # our C stack or pthread_t. + # our C stack or pthread_t. It's impossible to do this safely in C + # because there is no way to tell the C compiler not to use C stack. i32.const __thread_list_lock i32.const 0 i32.atomic.store 0