From c39214e8a5348d18931c62c91f94d225e9664599 Mon Sep 17 00:00:00 2001 From: Wenyong Huang Date: Mon, 8 Jan 2024 09:43:31 +0800 Subject: [PATCH] Fix potential recursive lock in pthread_create_wrapper (#2980) Potential recursive lock occurs in: ``` pthread_create_wrapper (acquire exec_env->wait_lock) => wasm_cluster_create_thread => allocate_aux_stack => wasm_runtime_module_malloc_internal => wasm_call_function => wasm_exec_env_set_thread_info (acquire exec_env->wait_lock again) ``` Allocate aux stack before calling wasm_cluster_create_thread to resolve it. Reported in https://github.com/bytecodealliance/wasm-micro-runtime/pull/2977. --- .../lib-pthread/lib_pthread_wrapper.c | 21 ++++++-- .../lib_wasi_threads_wrapper.c | 2 +- .../libraries/thread-mgr/thread_manager.c | 52 +++++++++++++------ .../libraries/thread-mgr/thread_manager.h | 11 +++- 4 files changed, 64 insertions(+), 22 deletions(-) diff --git a/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c b/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c index 56deaff32..a178306b2 100644 --- a/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c +++ b/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c @@ -558,6 +558,7 @@ pthread_create_wrapper(wasm_exec_env_t exec_env, ThreadRoutineArgs *routine_args = NULL; uint32 thread_handle; uint32 stack_size = 8192; + uint32 aux_stack_start = 0, aux_stack_size; int32 ret = -1; bh_assert(module); @@ -609,10 +610,22 @@ pthread_create_wrapper(wasm_exec_env_t exec_env, routine_args->info_node = info_node; routine_args->module_inst = new_module_inst; + /* Allocate aux stack previously since exec_env->wait_lock is acquired + below, and if the stack is allocated in wasm_cluster_create_thread, + runtime may call the exported malloc function to allocate the stack, + which acquires exec_env->wait again in wasm_exec_env_set_thread_info, + and recursive lock (or hang) occurs */ + if (!wasm_cluster_allocate_aux_stack(exec_env, &aux_stack_start, + &aux_stack_size)) { + LOG_ERROR("thread manager error: " + "failed to allocate aux stack space for new thread"); + goto fail; + } + os_mutex_lock(&exec_env->wait_lock); - ret = - wasm_cluster_create_thread(exec_env, new_module_inst, true, - pthread_start_routine, (void *)routine_args); + ret = wasm_cluster_create_thread( + exec_env, new_module_inst, true, aux_stack_start, aux_stack_size, + pthread_start_routine, (void *)routine_args); if (ret != 0) { os_mutex_unlock(&exec_env->wait_lock); goto fail; @@ -636,6 +649,8 @@ fail: wasm_runtime_free(info_node); if (routine_args) wasm_runtime_free(routine_args); + if (aux_stack_start) + wasm_cluster_free_aux_stack(exec_env, aux_stack_start); return ret; } 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 7e557be90..392266113 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 @@ -119,7 +119,7 @@ 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; - ret = wasm_cluster_create_thread(exec_env, new_module_inst, false, + ret = wasm_cluster_create_thread(exec_env, new_module_inst, false, 0, 0, thread_start, thread_start_arg); if (ret != 0) { LOG_ERROR("Failed to spawn a new thread"); diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.c b/core/iwasm/libraries/thread-mgr/thread_manager.c index f8bdf02bc..ce7ff82ac 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.c +++ b/core/iwasm/libraries/thread-mgr/thread_manager.c @@ -208,6 +208,33 @@ free_aux_stack(WASMExecEnv *exec_env, uint32 start) #endif } +bool +wasm_cluster_allocate_aux_stack(WASMExecEnv *exec_env, uint32 *p_start, + uint32 *p_size) +{ + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); + bool ret; + + os_mutex_lock(&cluster->lock); + ret = allocate_aux_stack(exec_env, p_start, p_size); + os_mutex_unlock(&cluster->lock); + + return ret; +} + +bool +wasm_cluster_free_aux_stack(WASMExecEnv *exec_env, uint32 start) +{ + WASMCluster *cluster = wasm_exec_env_get_cluster(exec_env); + bool ret; + + os_mutex_lock(&cluster->lock); + ret = free_aux_stack(exec_env, start); + os_mutex_unlock(&cluster->lock); + + return ret; +} + WASMCluster * wasm_cluster_create(WASMExecEnv *exec_env) { @@ -654,12 +681,13 @@ thread_manager_start_routine(void *arg) int32 wasm_cluster_create_thread(WASMExecEnv *exec_env, - wasm_module_inst_t module_inst, bool alloc_aux_stack, + wasm_module_inst_t module_inst, + bool is_aux_stack_allocated, uint32 aux_stack_start, + uint32 aux_stack_size, void *(*thread_routine)(void *), void *arg) { WASMCluster *cluster; WASMExecEnv *new_exec_env; - uint32 aux_stack_start = 0, aux_stack_size; korp_tid tid; cluster = wasm_exec_env_get_cluster(exec_env); @@ -676,17 +704,11 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env, if (!new_exec_env) goto fail1; - if (alloc_aux_stack) { - if (!allocate_aux_stack(exec_env, &aux_stack_start, &aux_stack_size)) { - LOG_ERROR("thread manager error: " - "failed to allocate aux stack space for new thread"); - goto fail2; - } - + if (is_aux_stack_allocated) { /* Set aux stack for current thread */ if (!wasm_exec_env_set_aux_stack(new_exec_env, aux_stack_start, aux_stack_size)) { - goto fail3; + goto fail2; } } else { @@ -699,7 +721,7 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env, new_exec_env->suspend_flags.flags = exec_env->suspend_flags.flags; if (!wasm_cluster_add_exec_env(cluster, new_exec_env)) - goto fail3; + goto fail2; new_exec_env->thread_start_routine = thread_routine; new_exec_env->thread_arg = arg; @@ -711,7 +733,7 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env, (void *)new_exec_env, APP_THREAD_STACK_SIZE_DEFAULT)) { os_mutex_unlock(&new_exec_env->wait_lock); - goto fail4; + goto fail3; } /* Wait until the new_exec_env->handle is set to avoid it is @@ -723,12 +745,8 @@ wasm_cluster_create_thread(WASMExecEnv *exec_env, return 0; -fail4: - wasm_cluster_del_exec_env_internal(cluster, new_exec_env, false); fail3: - /* free the allocated aux stack space */ - if (alloc_aux_stack) - free_aux_stack(exec_env, aux_stack_start); + wasm_cluster_del_exec_env_internal(cluster, new_exec_env, false); fail2: wasm_exec_env_destroy_internal(new_exec_env); fail1: diff --git a/core/iwasm/libraries/thread-mgr/thread_manager.h b/core/iwasm/libraries/thread-mgr/thread_manager.h index b95f434ae..588adac44 100644 --- a/core/iwasm/libraries/thread-mgr/thread_manager.h +++ b/core/iwasm/libraries/thread-mgr/thread_manager.h @@ -81,7 +81,9 @@ wasm_cluster_dup_c_api_imports(WASMModuleInstanceCommon *module_inst_dst, int32 wasm_cluster_create_thread(WASMExecEnv *exec_env, - wasm_module_inst_t module_inst, bool alloc_aux_stack, + wasm_module_inst_t module_inst, + bool is_aux_stack_allocated, uint32 aux_stack_start, + uint32 aux_stack_size, void *(*thread_routine)(void *), void *arg); int32 @@ -221,6 +223,13 @@ wasm_cluster_traverse_lock(WASMExecEnv *exec_env); void wasm_cluster_traverse_unlock(WASMExecEnv *exec_env); +bool +wasm_cluster_allocate_aux_stack(WASMExecEnv *exec_env, uint32 *p_start, + uint32 *p_size); + +bool +wasm_cluster_free_aux_stack(WASMExecEnv *exec_env, uint32 start); + #ifdef __cplusplus } #endif