Skip to content

Commit

Permalink
Fix potential recursive lock in pthread_create_wrapper (bytecodeallia…
Browse files Browse the repository at this point in the history
…nce#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 bytecodealliance#2977.
  • Loading branch information
wenyongh authored Jan 8, 2024
1 parent 75d5f09 commit 9c9ccea
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 22 deletions.
21 changes: 18 additions & 3 deletions core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -636,6 +649,8 @@ pthread_create_wrapper(wasm_exec_env_t exec_env,
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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
52 changes: 35 additions & 17 deletions core/iwasm/libraries/thread-mgr/thread_manager.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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:
Expand Down
11 changes: 10 additions & 1 deletion core/iwasm/libraries/thread-mgr/thread_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 9c9ccea

Please sign in to comment.