Skip to content

Protect access to exec env #1991

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenyongh merged 2 commits into from
Feb 27, 2023
Merged

Protect access to exec env #1991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de86504
fix(threads): protect access to exec env
eloparco Feb 26, 2023
3ff2609
fix: use exec env lock instead of cluster lock to protect exec env ac…
eloparco Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/iwasm/common/wasm_exec_env.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,17 @@ void
wasm_exec_env_set_thread_info(WASMExecEnv *exec_env)
{
uint8 *stack_boundary = os_thread_get_stack_boundary();

#if WASM_ENABLE_THREAD_MGR != 0
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
os_mutex_unlock(&exec_env->wait_lock);
#endif
}

#if WASM_ENABLE_THREAD_MGR != 0
Expand Down
6 changes: 6 additions & 0 deletions core/iwasm/interpreter/wasm_interp_classic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,27 +1036,33 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst,
#if WASM_ENABLE_DEBUG_INTERP != 0
#define CHECK_SUSPEND_FLAGS() \
do { \
os_mutex_lock(&exec_env->wait_lock); \
if (IS_WAMR_TERM_SIG(exec_env->current_status->signal_flag)) { \
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); \
} \
os_mutex_unlock(&exec_env->wait_lock); \
} while (0)
#else
#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; \
} \
while (exec_env->suspend_flags.flags & 0x02) { \
/* suspend current thread */ \
os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock); \
} \
} \
os_mutex_unlock(&exec_env->wait_lock); \
} while (0)
#endif /* WASM_ENABLE_DEBUG_INTERP */
#endif /* WASM_ENABLE_THREAD_MGR */
Expand Down
3 changes: 3 additions & 0 deletions core/iwasm/interpreter/wasm_interp_fast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,13 +1054,16 @@ wasm_interp_call_func_import(WASMModuleInstance *module_inst,
#if WASM_ENABLE_THREAD_MGR != 0
#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

Expand Down
3 changes: 0 additions & 3 deletions core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

@wenyongh wenyongh Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there data race here?
Had better not remove the lock, instead, add a cond_wait like pthread_create_wrapper in lib_pthread_wrapper.c:

wasm-micro-runtime/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c

Lines 665 to 668 in 38c67b3

/* Wait for the thread routine to assign the exec_env to
thread_info_node, otherwise the exec_env in the thread
info node may be NULL in the next pthread API call */
os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock);

And in the thread_start, add cond_signal like pthread_start_routine:

wasm-micro-runtime/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c

Line 509 in 38c67b3

os_cond_signal(&parent_exec_env->wait_cond);

Copy link
Contributor Author

@eloparco eloparco Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why I removed it is that, after getting the exec_env lock there, the cluster lock gets acquired

wasm-micro-runtime/core/iwasm/libraries/thread-mgr/thread_manager.c

Line 623 in 38c67b3

os_mutex_lock(&cluster->lock);

but then an attempt to get the exec_env lock is made again at https://github.com/eloparco/wasm-micro-runtime/blob/5550b5647037a7cc9674dda03413a95e3c06cba3/core/iwasm/libraries/thread-mgr/thread_manager.c#L946 generating a warning in the sanitizer for potential deadlock.

Let me see how it can be fixed.

Copy link
Collaborator

@wenyongh wenyongh Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the thread isn't actually started, but it is already added to the cluster's exec_env list, and another thread (seems main thread) tries to terminate it? Maybe after adding the similar logic like pthread_create_wrapper, the issue can be resolved since the main thread will wait unit the child thread actually started.
Here try adding cond_wait:

    os_mutex_lock(&exec_env->wait_lock);
    ret = wasm_cluster_create_thread(exec_env, new_module_inst, false,
                                     thread_start, thread_start_arg);
    ...
    os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock);
    os_mutex_unlock(&exec_env->wait_lock);

And in thread_start:

    os_mutex_lock(&parent_exec_env->wait_lock);
    ...
    os_cond_signal(&parent_exec_env->wait_cond, &parent_exec_env->wait_lock);
    os_mutex_unlock(&parent_exec_env->wait_lock);

Copy link
Contributor Author

@eloparco eloparco Feb 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually only happens when it's the spawned thread that calls proc_exit.

I see that for pthreads that lock is used to wait for the update of ThreadInfoNode.

wasm-micro-runtime/core/iwasm/libraries/lib-pthread/lib_pthread_wrapper.c

Lines 665 to 668 in 38c67b3

/* Wait for the thread routine to assign the exec_env to
thread_info_node, otherwise the exec_env in the thread
info node may be NULL in the next pthread API call */
os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock);

In lib_wasi_threads_wrapper.c we don't use ThreadInfoNode so I don't think the lock there is even needed.

Copy link
Collaborator

@wenyongh wenyongh Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, got it, thanks.

Copy link
Contributor Author

@eloparco eloparco Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, once this one is merged, we can merge #1985 too


return thread_id;

thread_spawn_fail:
os_mutex_unlock(&exec_env->wait_lock);
deallocate_thread_id(thread_id);

thread_preparation_fail:
Expand Down
16 changes: 15 additions & 1 deletion core/iwasm/libraries/thread-mgr/thread_manager.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,12 +574,16 @@ thread_manager_start_routine(void *arg)
bh_assert(cluster != NULL);
bh_assert(module_inst != NULL);

os_mutex_lock(&exec_env->wait_lock);
exec_env->handle = os_self_thread();
os_mutex_unlock(&exec_env->wait_lock);
ret = exec_env->thread_start_routine(exec_env);

#ifdef OS_ENABLE_HW_BOUND_CHECK
os_mutex_lock(&exec_env->wait_lock);
if (exec_env->suspend_flags.flags & 0x08)
ret = exec_env->thread_ret_value;
os_mutex_unlock(&exec_env->wait_lock);
#endif

/* Routine exit */
Expand Down Expand Up @@ -854,8 +858,11 @@ wasm_cluster_join_thread(WASMExecEnv *exec_env, void **ret_val)
os_mutex_unlock(&cluster_list_lock);
return 0;
}

os_mutex_lock(&exec_env->wait_lock);
exec_env->wait_count++;
handle = exec_env->handle;
os_mutex_unlock(&exec_env->wait_lock);

os_mutex_unlock(&exec_env->cluster->lock);
os_mutex_unlock(&cluster_list_lock);
Expand Down Expand Up @@ -936,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
Expand Down Expand Up @@ -1209,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;
}