From 382d52fc05dbb543dfafb969182104d6c4856c63 Mon Sep 17 00:00:00 2001 From: YAMAMOTO Takashi Date: Thu, 31 Aug 2023 21:39:08 +0900 Subject: [PATCH] Stop abusing shared memory lock to protect exception (#2509) Use a separate global lock instead. Fixes: https://github.com/bytecodealliance/wasm-micro-runtime/issues/2407 --- core/iwasm/common/wasm_runtime_common.c | 28 +---- core/iwasm/interpreter/wasm_runtime.h | 10 ++ .../libraries/thread-mgr/thread_manager.c | 111 +++++++++--------- .../libraries/thread-mgr/thread_manager.h | 2 +- 4 files changed, 75 insertions(+), 76 deletions(-) diff --git a/core/iwasm/common/wasm_runtime_common.c b/core/iwasm/common/wasm_runtime_common.c index 09233f249..ac85c61dc 100644 --- a/core/iwasm/common/wasm_runtime_common.c +++ b/core/iwasm/common/wasm_runtime_common.c @@ -2377,12 +2377,7 @@ wasm_runtime_get_exec_env_singleton(WASMModuleInstanceCommon *module_inst_comm) void 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,19 +2385,14 @@ 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 = + 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 } @@ -2453,10 +2443,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 +2455,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 3e0852487..784febd44 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 21425786a..02430fe87 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); } @@ -1240,72 +1243,56 @@ 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; + const struct spread_exception_data *data = user_data; + WASMExecEnv *exec_env = (WASMExecEnv *)node; - if (curr_exec_env != exec_env) { - WASMModuleInstance *curr_wasm_inst = - (WASMModuleInstance *)get_module_inst(curr_exec_env); + if (exec_env != data->skip) { + WASMModuleInstance *wasm_inst = + (WASMModuleInstance *)get_module_inst(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 - 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_lock(wasm_inst); + if (data->exception != NULL) { + /* 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); + } } -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_unlock(curr_wasm_inst->memories[0]); -#endif + else { + wasm_inst->cur_exception[0] = '\0'; + } + exception_unlock(wasm_inst); /* Terminate the thread so it can exit from dead loops */ - set_thread_cancel_flags(curr_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); - -#if WASM_ENABLE_SHARED_MEMORY != 0 - if (curr_wasm_inst->memory_count > 0) - shared_memory_lock(curr_wasm_inst->memories[0]); -#endif - 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 + 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); } @@ -1353,3 +1340,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); +} diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.h b/core/iwasm/libraries/thread-mgr/thread_manager.h index 2060869c2..c6bc7a526 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);