From 3004ee6646afe7abd97a3213b84f711ee174b9cc Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Mon, 28 Aug 2023 18:31:45 +0900 Subject: [PATCH 1/3] Stop abusing shared memory lock to protect exception Use a separate global lock instead. Fixes: https://github.com/bytecodealliance/wasm-micro-runtime/issues/2407 --- core/iwasm/common/wasm_runtime_common.c | 20 ++------ core/iwasm/interpreter/wasm_runtime.h | 10 ++++ .../libraries/thread-mgr/thread_manager.c | 49 +++++++++++-------- 3 files changed, 43 insertions(+), 36 deletions(-) diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c index 09233f2496..c379654eea 100644 --- a/core/iwasm/common/wasm_runtime_common.c +++ b/core/iwasm/common/wasm_runtime_common.c @@ -2379,10 +2379,7 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception) { WASMExecEnv *exec_env = NULL; -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (module_inst->memory_count > 0) - shared_memory_lock(module_inst->memories[0]); -#endif + exception_lock(module_inst); if (exception) { snprintf(module_inst->cur_exception, sizeof(module_inst->cur_exception), "Exception: %s", exception); @@ -2390,10 +2387,7 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception) else { module_inst->cur_exception[0] = '\0'; } -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (module_inst->memory_count > 0) - shared_memory_unlock(module_inst->memories[0]); -#endif + exception_unlock(module_inst); #if WASM_ENABLE_THREAD_MGR != 0 exec_env = @@ -2453,10 +2447,7 @@ wasm_copy_exception(WASMModuleInstance *module_inst, char *exception_buf) { bool has_exception = false; -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (module_inst->memory_count > 0) - shared_memory_lock(module_inst->memories[0]); -#endif + exception_lock(module_inst); if (module_inst->cur_exception[0] != '\0') { /* NULL is passed if the caller is not interested in getting the * exception content, but only in knowing if an exception has been @@ -2468,10 +2459,7 @@ wasm_copy_exception(WASMModuleInstance *module_inst, char *exception_buf) sizeof(module_inst->cur_exception)); has_exception = true; } -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (module_inst->memory_count > 0) - shared_memory_unlock(module_inst->memories[0]); -#endif + exception_unlock(module_inst); return has_exception; } diff --git a/core/iwasm/interpreter/wasm_runtime.h b/core/iwasm/interpreter/wasm_runtime.h index 3e0852487e..784febd44b 100644 --- a/core/iwasm/interpreter/wasm_runtime.h +++ b/core/iwasm/interpreter/wasm_runtime.h @@ -668,6 +668,16 @@ void wasm_propagate_wasi_args(WASMModule *module); #endif +#if WASM_ENABLE_THREAD_MGR != 0 +void +exception_lock(WASMModuleInstance *module_inst); +void +exception_unlock(WASMModuleInstance *module_inst); +#else +#define exception_lock(module_inst) (void)(module_inst) +#define exception_unlock(module_inst) (void)(module_inst) +#endif + #ifdef __cplusplus } #endif diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 21425786ad..448668c030 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -16,10 +16,6 @@ #include "debug_engine.h" #endif -#if WASM_ENABLE_SHARED_MEMORY != 0 -#include "wasm_shared_memory.h" -#endif - typedef struct { bh_list_link l; void (*destroy_cb)(WASMCluster *); @@ -32,6 +28,8 @@ static bh_list cluster_list_head; static bh_list *const cluster_list = &cluster_list_head; static korp_mutex cluster_list_lock; +static korp_mutex _exception_lock; + typedef void (*list_visitor)(void *, void *); static uint32 cluster_max_thread_num = CLUSTER_MAX_THREAD_NUM; @@ -52,6 +50,10 @@ thread_manager_init() return false; if (os_mutex_init(&cluster_list_lock) != 0) return false; + if (os_mutex_init(&_exception_lock) != 0) { + os_mutex_destroy(&cluster_list_lock); + return false; + } return true; } @@ -66,6 +68,7 @@ thread_manager_destroy() cluster = next; } wasm_cluster_cancel_all_callbacks(); + os_mutex_destroy(&_exception_lock); os_mutex_destroy(&cluster_list_lock); } @@ -1253,20 +1256,14 @@ set_exception_visitor(void *node, void *user_data) (WASMModuleInstance *)get_module_inst(curr_exec_env); /* Only spread non "wasi proc exit" exception */ -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_lock(curr_wasm_inst->memories[0]); -#endif + exception_lock(curr_wasm_inst); if (!strstr(wasm_inst->cur_exception, "wasi proc exit")) { bh_memcpy_s(curr_wasm_inst->cur_exception, sizeof(curr_wasm_inst->cur_exception), wasm_inst->cur_exception, sizeof(wasm_inst->cur_exception)); } -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_unlock(curr_wasm_inst->memories[0]); -#endif + exception_unlock(curr_wasm_inst); /* Terminate the thread so it can exit from dead loops */ set_thread_cancel_flags(curr_exec_env); @@ -1283,15 +1280,9 @@ clear_exception_visitor(void *node, void *user_data) WASMModuleInstance *curr_wasm_inst = (WASMModuleInstance *)get_module_inst(curr_exec_env); -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_lock(curr_wasm_inst->memories[0]); -#endif + exception_lock(curr_wasm_inst); curr_wasm_inst->cur_exception[0] = '\0'; -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_unlock(curr_wasm_inst->memories[0]); -#endif + exception_unlock(curr_wasm_inst); } } @@ -1353,3 +1344,21 @@ wasm_cluster_is_thread_terminated(WASMExecEnv *exec_env) return is_thread_terminated; } + +void +exception_lock(WASMModuleInstance *module_inst) +{ + /* + * Note: this lock could be per module instance if desirable. + * We can revisit on AOT version bump. + * It probably doesn't matter though because the exception handling + * logic should not be executed too frequently anyway. + */ + os_mutex_lock(&_exception_lock); +} + +void +exception_unlock(WASMModuleInstance *module_inst) +{ + os_mutex_unlock(&_exception_lock); +} From 90cc664e3afd057a1dd25de59b365bb334cc92d3 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Mon, 28 Aug 2023 18:48:52 +0900 Subject: [PATCH 2/3] wasm_cluster_spread_exception: simplify logic and fix minor locking error --- core/iwasm/common/wasm_runtime_common.c | 8 +-- .../libraries/thread-mgr/thread_manager.c | 70 ++++++++----------- .../libraries/thread-mgr/thread_manager.h | 2 +- 3 files changed, 34 insertions(+), 46 deletions(-) diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c index c379654eea..ac85c61dcf 100644 --- a/core/iwasm/common/wasm_runtime_common.c +++ b/core/iwasm/common/wasm_runtime_common.c @@ -2377,8 +2377,6 @@ wasm_runtime_get_exec_env_singleton(WASMModuleInstanceCommon *module_inst_comm) void wasm_set_exception(WASMModuleInstance *module_inst, const char *exception) { - WASMExecEnv *exec_env = NULL; - exception_lock(module_inst); if (exception) { snprintf(module_inst->cur_exception, sizeof(module_inst->cur_exception), @@ -2390,13 +2388,11 @@ wasm_set_exception(WASMModuleInstance *module_inst, const char *exception) exception_unlock(module_inst); #if WASM_ENABLE_THREAD_MGR != 0 - exec_env = + WASMExecEnv *exec_env = wasm_clusters_search_exec_env((WASMModuleInstanceCommon *)module_inst); if (exec_env) { - wasm_cluster_spread_exception(exec_env, exception ? false : true); + wasm_cluster_spread_exception(exec_env, exception); } -#else - (void)exec_env; #endif } diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 448668c030..9a1e82d316 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -1243,60 +1243,52 @@ wasm_cluster_resume_all(WASMCluster *cluster) os_mutex_unlock(&cluster->lock); } +struct spread_exception_data { + WASMExecEnv *skip; + const char *exception; +}; + static void set_exception_visitor(void *node, void *user_data) { - WASMExecEnv *curr_exec_env = (WASMExecEnv *)node; - WASMExecEnv *exec_env = (WASMExecEnv *)user_data; - WASMModuleInstanceCommon *module_inst = get_module_inst(exec_env); - WASMModuleInstance *wasm_inst = (WASMModuleInstance *)module_inst; - - if (curr_exec_env != exec_env) { - WASMModuleInstance *curr_wasm_inst = - (WASMModuleInstance *)get_module_inst(curr_exec_env); - - /* Only spread non "wasi proc exit" exception */ - exception_lock(curr_wasm_inst); - if (!strstr(wasm_inst->cur_exception, "wasi proc exit")) { - bh_memcpy_s(curr_wasm_inst->cur_exception, - sizeof(curr_wasm_inst->cur_exception), - wasm_inst->cur_exception, - sizeof(wasm_inst->cur_exception)); - } - exception_unlock(curr_wasm_inst); + const struct spread_exception_data *data = user_data; + WASMExecEnv *exec_env = (WASMExecEnv *)node; - /* Terminate the thread so it can exit from dead loops */ - set_thread_cancel_flags(curr_exec_env); - } -} + if (exec_env != data->skip) { + WASMModuleInstance *wasm_inst = + (WASMModuleInstance *)get_module_inst(exec_env); -static void -clear_exception_visitor(void *node, void *user_data) -{ - WASMExecEnv *exec_env = (WASMExecEnv *)user_data; - WASMExecEnv *curr_exec_env = (WASMExecEnv *)node; - - if (curr_exec_env != exec_env) { - WASMModuleInstance *curr_wasm_inst = - (WASMModuleInstance *)get_module_inst(curr_exec_env); + exception_lock(wasm_inst); + if (data->exception != NULL) { + snprintf(wasm_inst->cur_exception, sizeof(wasm_inst->cur_exception), + "Exception: %s", data->exception); + } + else { + wasm_inst->cur_exception[0] = '\0'; + } + exception_unlock(wasm_inst); - exception_lock(curr_wasm_inst); - curr_wasm_inst->cur_exception[0] = '\0'; - exception_unlock(curr_wasm_inst); + /* Terminate the thread so it can exit from dead loops */ + if (data->exception != NULL) { + set_thread_cancel_flags(exec_env); + } } } void -wasm_cluster_spread_exception(WASMExecEnv *exec_env, bool clear) +wasm_cluster_spread_exception(WASMExecEnv *exec_env, const char *exception) { + const bool has_exception = exception != NULL; WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); bh_assert(cluster); + struct spread_exception_data data; + data.skip = exec_env; + data.exception = exception; + os_mutex_lock(&cluster->lock); - cluster->has_exception = !clear; - traverse_list(&cluster->exec_env_list, - clear ? clear_exception_visitor : set_exception_visitor, - exec_env); + cluster->has_exception = has_exception; + traverse_list(&cluster->exec_env_list, set_exception_visitor, &data); os_mutex_unlock(&cluster->lock); } diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.h b/core/iwasm/libraries/thread-mgr/thread_manager.h index 2060869c28..c6bc7a526e 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.h +++ b/core/iwasm/libraries/thread-mgr/thread_manager.h @@ -139,7 +139,7 @@ WASMExecEnv * wasm_clusters_search_exec_env(WASMModuleInstanceCommon *module_inst); void -wasm_cluster_spread_exception(WASMExecEnv *exec_env, bool clear); +wasm_cluster_spread_exception(WASMExecEnv *exec_env, const char *exception); WASMExecEnv * wasm_cluster_spawn_exec_env(WASMExecEnv *exec_env); From f1349d1c2ec186ce57f012b45231dd76054be11f Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Tue, 29 Aug 2023 16:33:50 +0900 Subject: [PATCH 3/3] restore "Only spread non "wasi proc exit" exception" behavior for now --- core/iwasm/libraries/thread-mgr/thread_manager.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index 9a1e82d316..02430fe87e 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -1260,8 +1260,12 @@ set_exception_visitor(void *node, void *user_data) exception_lock(wasm_inst); if (data->exception != NULL) { - snprintf(wasm_inst->cur_exception, sizeof(wasm_inst->cur_exception), - "Exception: %s", data->exception); + /* Only spread non "wasi proc exit" exception */ + if (strcmp(data->exception, "wasi proc exit")) { + snprintf(wasm_inst->cur_exception, + sizeof(wasm_inst->cur_exception), "Exception: %s", + data->exception); + } } else { wasm_inst->cur_exception[0] = '\0';