From de865048c57f37abf12d31f1b66d74310d55aff8 Mon Sep 17 00:00:00 2001 From: eloparco Date: Sun, 26 Feb 2023 00:08:13 +0000 Subject: [PATCH 1/2] fix(threads): protect access to exec env --- core/iwasm/common/wasm_exec_env.c | 10 +++++++ core/iwasm/interpreter/wasm_interp_classic.c | 18 +++++++++++- core/iwasm/interpreter/wasm_interp_fast.c | 29 +++++++++++++------ .../libraries/libc-wasi/libc_wasi_wrapper.c | 13 ++++++++- .../libraries/thread-mgr/thread_manager.c | 12 ++++++-- 5 files changed, 69 insertions(+), 13 deletions(-) diff --git a/core/iwasm/common/wasm_exec_env.c b/core/iwasm/common/wasm_exec_env.c index 9bd74d7d55..3628c9b092 100644 --- a/core/iwasm/common/wasm_exec_env.c +++ b/core/iwasm/common/wasm_exec_env.c @@ -208,10 +208,20 @@ void wasm_exec_env_set_thread_info(WASMExecEnv *exec_env) { uint8 *stack_boundary = os_thread_get_stack_boundary(); + +#if WASM_ENABLE_THREAD_MGR != 0 + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); + if (cluster) + os_mutex_lock(&cluster->lock); +#endif exec_env->handle = os_self_thread(); exec_env->native_stack_boundary = stack_boundary ? stack_boundary + WASM_STACK_GUARD_SIZE : NULL; exec_env->native_stack_top_min = (void *)UINTPTR_MAX; +#if WASM_ENABLE_THREAD_MGR != 0 + if (cluster) + os_mutex_unlock(&cluster->lock); +#endif } #if WASM_ENABLE_THREAD_MGR != 0 diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index ac6763c697..a82607f19b 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -12,8 +12,10 @@ #if WASM_ENABLE_SHARED_MEMORY != 0 #include "../common/wasm_shared_memory.h" #endif -#if WASM_ENABLE_THREAD_MGR != 0 && WASM_ENABLE_DEBUG_INTERP != 0 +#if WASM_ENABLE_THREAD_MGR != 0 #include "../libraries/thread-mgr/thread_manager.h" +#endif +#if WASM_ENABLE_THREAD_MGR != 0 && WASM_ENABLE_DEBUG_INTERP != 0 #include "../libraries/debug-engine/debug_engine.h" #endif #if WASM_ENABLE_FAST_JIT != 0 @@ -1036,20 +1038,32 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, #if WASM_ENABLE_DEBUG_INTERP != 0 #define CHECK_SUSPEND_FLAGS() \ do { \ + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ + if (cluster) \ + os_mutex_lock(&cluster->lock); \ if (IS_WAMR_TERM_SIG(exec_env->current_status->signal_flag)) { \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ return; \ } \ if (IS_WAMR_STOP_SIG(exec_env->current_status->signal_flag)) { \ SYNC_ALL_TO_FRAME(); \ wasm_cluster_thread_waiting_run(exec_env); \ } \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ } while (0) #else #define CHECK_SUSPEND_FLAGS() \ do { \ + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ + if (cluster) \ + os_mutex_lock(&cluster->lock); \ if (exec_env->suspend_flags.flags != 0) { \ if (exec_env->suspend_flags.flags & 0x01) { \ /* terminate current thread */ \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ return; \ } \ while (exec_env->suspend_flags.flags & 0x02) { \ @@ -1057,6 +1071,8 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock); \ } \ } \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ } while (0) #endif /* WASM_ENABLE_DEBUG_INTERP */ #endif /* WASM_ENABLE_THREAD_MGR */ diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c index 7a8f474c5d..e076621f8c 100644 --- a/core/iwasm/interpreter/wasm_interp_fast.c +++ b/core/iwasm/interpreter/wasm_interp_fast.c @@ -13,6 +13,10 @@ #include "../common/wasm_shared_memory.h" #endif +#if WASM_ENABLE_THREAD_MGR != 0 +#include "../libraries/thread-mgr/thread_manager.h" +#endif + typedef int32 CellType_I32; typedef int64 CellType_I64; typedef float32 CellType_F32; @@ -1052,15 +1056,22 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, #endif #if WASM_ENABLE_THREAD_MGR != 0 -#define CHECK_SUSPEND_FLAGS() \ - do { \ - if (exec_env->suspend_flags.flags != 0) { \ - if (exec_env->suspend_flags.flags & 0x01) { \ - /* terminate current thread */ \ - return; \ - } \ - /* TODO: support suspend and breakpoint */ \ - } \ +#define CHECK_SUSPEND_FLAGS() \ + do { \ + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ + if (cluster) \ + os_mutex_lock(&cluster->lock); \ + if (exec_env->suspend_flags.flags != 0) { \ + if (exec_env->suspend_flags.flags & 0x01) { \ + /* terminate current thread */ \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ + return; \ + } \ + /* TODO: support suspend and breakpoint */ \ + } \ + if (cluster) \ + os_mutex_unlock(&cluster->lock); \ } while (0) #endif diff --git a/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c b/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c index ab2808c6e9..71f63e13a0 100644 --- a/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c +++ b/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c @@ -1027,7 +1027,18 @@ execute_interruptible_poll_oneoff( return err; } - if (wasm_cluster_is_thread_terminated(exec_env)) { +#if WASM_ENABLE_THREAD_MGR != 0 + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); + if (cluster) + os_mutex_lock(&cluster->lock); +#endif + bool is_thread_terminated = wasm_cluster_is_thread_terminated(exec_env); +#if WASM_ENABLE_THREAD_MGR != 0 + if (cluster) + os_mutex_unlock(&cluster->lock); +#endif + + if (is_thread_terminated) { wasm_runtime_free(in_copy); return EINTR; } diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 79048c04a7..153bb0273c 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -574,12 +574,16 @@ thread_manager_start_routine(void *arg) bh_assert(cluster != NULL); bh_assert(module_inst != NULL); + os_mutex_lock(&cluster->lock); exec_env->handle = os_self_thread(); + os_mutex_unlock(&cluster->lock); ret = exec_env->thread_start_routine(exec_env); #ifdef OS_ENABLE_HW_BOUND_CHECK + os_mutex_lock(&cluster->lock); if (exec_env->suspend_flags.flags & 0x08) ret = exec_env->thread_ret_value; + os_mutex_unlock(&cluster->lock); #endif /* Routine exit */ @@ -822,15 +826,18 @@ clusters_have_exec_env(WASMExecEnv *exec_env) WASMExecEnv *node; while (cluster) { + os_mutex_lock(&cluster->lock); node = bh_list_first_elem(&cluster->exec_env_list); while (node) { if (node == exec_env) { bh_assert(exec_env->cluster == cluster); + os_mutex_unlock(&cluster->lock); return true; } node = bh_list_elem_next(node); } + os_mutex_unlock(&cluster->lock); cluster = bh_list_elem_next(cluster); } @@ -844,7 +851,6 @@ wasm_cluster_join_thread(WASMExecEnv *exec_env, void **ret_val) korp_tid handle; os_mutex_lock(&cluster_list_lock); - os_mutex_lock(&exec_env->cluster->lock); if (!clusters_have_exec_env(exec_env) || exec_env->thread_is_detached) { /* Invalid thread, thread has exited or thread has been detached */ @@ -854,10 +860,12 @@ wasm_cluster_join_thread(WASMExecEnv *exec_env, void **ret_val) os_mutex_unlock(&cluster_list_lock); return 0; } + + os_mutex_lock(&exec_env->cluster->lock); exec_env->wait_count++; handle = exec_env->handle; - os_mutex_unlock(&exec_env->cluster->lock); + os_mutex_unlock(&cluster_list_lock); return os_thread_join(handle, ret_val); From 3ff2609719bb732e0c0e4aa926965e3b2544c486 Mon Sep 17 00:00:00 2001 From: eloparco Date: Mon, 27 Feb 2023 09:30:03 +0000 Subject: [PATCH 2/2] fix: use exec env lock instead of cluster lock to protect exec env access --- core/iwasm/common/wasm_exec_env.c | 7 ++-- core/iwasm/interpreter/wasm_interp_classic.c | 24 ++++---------- core/iwasm/interpreter/wasm_interp_fast.c | 32 +++++++------------ .../lib_wasi_threads_wrapper.c | 3 -- .../libraries/libc-wasi/libc_wasi_wrapper.c | 13 +------- .../libraries/thread-mgr/thread_manager.c | 26 +++++++++------ 6 files changed, 38 insertions(+), 67 deletions(-) diff --git a/core/iwasm/common/wasm_exec_env.c b/core/iwasm/common/wasm_exec_env.c index 3628c9b092..9cda929b13 100644 --- a/core/iwasm/common/wasm_exec_env.c +++ b/core/iwasm/common/wasm_exec_env.c @@ -210,17 +210,14 @@ wasm_exec_env_set_thread_info(WASMExecEnv *exec_env) uint8 *stack_boundary = os_thread_get_stack_boundary(); #if WASM_ENABLE_THREAD_MGR != 0 - WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); - if (cluster) - os_mutex_lock(&cluster->lock); + os_mutex_lock(&exec_env->wait_lock); #endif exec_env->handle = os_self_thread(); exec_env->native_stack_boundary = stack_boundary ? stack_boundary + WASM_STACK_GUARD_SIZE : NULL; exec_env->native_stack_top_min = (void *)UINTPTR_MAX; #if WASM_ENABLE_THREAD_MGR != 0 - if (cluster) - os_mutex_unlock(&cluster->lock); + os_mutex_unlock(&exec_env->wait_lock); #endif } diff --git a/core/iwasm/interpreter/wasm_interp_classic.c b/core/iwasm/interpreter/wasm_interp_classic.c index a82607f19b..5f1072fa9c 100644 --- a/core/iwasm/interpreter/wasm_interp_classic.c +++ b/core/iwasm/interpreter/wasm_interp_classic.c @@ -12,10 +12,8 @@ #if WASM_ENABLE_SHARED_MEMORY != 0 #include "../common/wasm_shared_memory.h" #endif -#if WASM_ENABLE_THREAD_MGR != 0 -#include "../libraries/thread-mgr/thread_manager.h" -#endif #if WASM_ENABLE_THREAD_MGR != 0 && WASM_ENABLE_DEBUG_INTERP != 0 +#include "../libraries/thread-mgr/thread_manager.h" #include "../libraries/debug-engine/debug_engine.h" #endif #if WASM_ENABLE_FAST_JIT != 0 @@ -1038,32 +1036,25 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, #if WASM_ENABLE_DEBUG_INTERP != 0 #define CHECK_SUSPEND_FLAGS() \ do { \ - WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ - if (cluster) \ - os_mutex_lock(&cluster->lock); \ + os_mutex_lock(&exec_env->wait_lock); \ if (IS_WAMR_TERM_SIG(exec_env->current_status->signal_flag)) { \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ + os_mutex_unlock(&exec_env->wait_lock); \ return; \ } \ if (IS_WAMR_STOP_SIG(exec_env->current_status->signal_flag)) { \ SYNC_ALL_TO_FRAME(); \ wasm_cluster_thread_waiting_run(exec_env); \ } \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ + os_mutex_unlock(&exec_env->wait_lock); \ } while (0) #else #define CHECK_SUSPEND_FLAGS() \ do { \ - WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ - if (cluster) \ - os_mutex_lock(&cluster->lock); \ + os_mutex_lock(&exec_env->wait_lock); \ if (exec_env->suspend_flags.flags != 0) { \ if (exec_env->suspend_flags.flags & 0x01) { \ /* terminate current thread */ \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ + os_mutex_unlock(&exec_env->wait_lock); \ return; \ } \ while (exec_env->suspend_flags.flags & 0x02) { \ @@ -1071,8 +1062,7 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock); \ } \ } \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ + os_mutex_unlock(&exec_env->wait_lock); \ } while (0) #endif /* WASM_ENABLE_DEBUG_INTERP */ #endif /* WASM_ENABLE_THREAD_MGR */ diff --git a/core/iwasm/interpreter/wasm_interp_fast.c b/core/iwasm/interpreter/wasm_interp_fast.c index e076621f8c..34cb45c8da 100644 --- a/core/iwasm/interpreter/wasm_interp_fast.c +++ b/core/iwasm/interpreter/wasm_interp_fast.c @@ -13,10 +13,6 @@ #include "../common/wasm_shared_memory.h" #endif -#if WASM_ENABLE_THREAD_MGR != 0 -#include "../libraries/thread-mgr/thread_manager.h" -#endif - typedef int32 CellType_I32; typedef int64 CellType_I64; typedef float32 CellType_F32; @@ -1056,22 +1052,18 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst, #endif #if WASM_ENABLE_THREAD_MGR != 0 -#define CHECK_SUSPEND_FLAGS() \ - do { \ - WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); \ - if (cluster) \ - os_mutex_lock(&cluster->lock); \ - if (exec_env->suspend_flags.flags != 0) { \ - if (exec_env->suspend_flags.flags & 0x01) { \ - /* terminate current thread */ \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ - return; \ - } \ - /* TODO: support suspend and breakpoint */ \ - } \ - if (cluster) \ - os_mutex_unlock(&cluster->lock); \ +#define CHECK_SUSPEND_FLAGS() \ + do { \ + os_mutex_lock(&exec_env->wait_lock); \ + if (exec_env->suspend_flags.flags != 0) { \ + if (exec_env->suspend_flags.flags & 0x01) { \ + /* terminate current thread */ \ + os_mutex_unlock(&exec_env->wait_lock); \ + return; \ + } \ + /* TODO: support suspend and breakpoint */ \ + } \ + os_mutex_unlock(&exec_env->wait_lock); \ } while (0) #endif diff --git a/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c b/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c index 81efc67515..db96898db3 100644 --- a/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c +++ b/core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c @@ -123,19 +123,16 @@ thread_spawn_wrapper(wasm_exec_env_t exec_env, uint32 start_arg) thread_start_arg->arg = start_arg; thread_start_arg->start_func = start_func; - os_mutex_lock(&exec_env->wait_lock); ret = wasm_cluster_create_thread(exec_env, new_module_inst, false, thread_start, thread_start_arg); if (ret != 0) { LOG_ERROR("Failed to spawn a new thread"); goto thread_spawn_fail; } - os_mutex_unlock(&exec_env->wait_lock); return thread_id; thread_spawn_fail: - os_mutex_unlock(&exec_env->wait_lock); deallocate_thread_id(thread_id); thread_preparation_fail: diff --git a/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c b/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c index 71f63e13a0..ab2808c6e9 100644 --- a/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c +++ b/core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c @@ -1027,18 +1027,7 @@ execute_interruptible_poll_oneoff( return err; } -#if WASM_ENABLE_THREAD_MGR != 0 - WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); - if (cluster) - os_mutex_lock(&cluster->lock); -#endif - bool is_thread_terminated = wasm_cluster_is_thread_terminated(exec_env); -#if WASM_ENABLE_THREAD_MGR != 0 - if (cluster) - os_mutex_unlock(&cluster->lock); -#endif - - if (is_thread_terminated) { + if (wasm_cluster_is_thread_terminated(exec_env)) { wasm_runtime_free(in_copy); return EINTR; } diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 153bb0273c..cab24a6958 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -574,16 +574,16 @@ thread_manager_start_routine(void *arg) bh_assert(cluster != NULL); bh_assert(module_inst != NULL); - os_mutex_lock(&cluster->lock); + os_mutex_lock(&exec_env->wait_lock); exec_env->handle = os_self_thread(); - os_mutex_unlock(&cluster->lock); + os_mutex_unlock(&exec_env->wait_lock); ret = exec_env->thread_start_routine(exec_env); #ifdef OS_ENABLE_HW_BOUND_CHECK - os_mutex_lock(&cluster->lock); + os_mutex_lock(&exec_env->wait_lock); if (exec_env->suspend_flags.flags & 0x08) ret = exec_env->thread_ret_value; - os_mutex_unlock(&cluster->lock); + os_mutex_unlock(&exec_env->wait_lock); #endif /* Routine exit */ @@ -826,18 +826,15 @@ clusters_have_exec_env(WASMExecEnv *exec_env) WASMExecEnv *node; while (cluster) { - os_mutex_lock(&cluster->lock); node = bh_list_first_elem(&cluster->exec_env_list); while (node) { if (node == exec_env) { bh_assert(exec_env->cluster == cluster); - os_mutex_unlock(&cluster->lock); return true; } node = bh_list_elem_next(node); } - os_mutex_unlock(&cluster->lock); cluster = bh_list_elem_next(cluster); } @@ -851,6 +848,7 @@ wasm_cluster_join_thread(WASMExecEnv *exec_env, void **ret_val) korp_tid handle; os_mutex_lock(&cluster_list_lock); + os_mutex_lock(&exec_env->cluster->lock); if (!clusters_have_exec_env(exec_env) || exec_env->thread_is_detached) { /* Invalid thread, thread has exited or thread has been detached */ @@ -861,11 +859,12 @@ wasm_cluster_join_thread(WASMExecEnv *exec_env, void **ret_val) return 0; } - os_mutex_lock(&exec_env->cluster->lock); + os_mutex_lock(&exec_env->wait_lock); exec_env->wait_count++; handle = exec_env->handle; - os_mutex_unlock(&exec_env->cluster->lock); + os_mutex_unlock(&exec_env->wait_lock); + os_mutex_unlock(&exec_env->cluster->lock); os_mutex_unlock(&cluster_list_lock); return os_thread_join(handle, ret_val); @@ -944,12 +943,14 @@ wasm_cluster_exit_thread(WASMExecEnv *exec_env, void *retval) static void set_thread_cancel_flags(WASMExecEnv *exec_env) { + os_mutex_lock(&exec_env->wait_lock); /* Set the termination flag */ #if WASM_ENABLE_DEBUG_INTERP != 0 wasm_cluster_thread_send_signal(exec_env, WAMR_SIG_TERM); #else exec_env->suspend_flags.flags |= 0x01; #endif + os_mutex_unlock(&exec_env->wait_lock); } int32 @@ -1217,5 +1218,10 @@ wasm_cluster_spread_custom_data(WASMModuleInstanceCommon *module_inst, bool wasm_cluster_is_thread_terminated(WASMExecEnv *exec_env) { - return (exec_env->suspend_flags.flags & 0x01) ? true : false; + os_mutex_lock(&exec_env->wait_lock); + bool is_thread_terminated = + (exec_env->suspend_flags.flags & 0x01) ? true : false; + os_mutex_unlock(&exec_env->wait_lock); + + return is_thread_terminated; }