Protect access to exec env #1991

eloparco · 2023-02-26T00:16:24Z

Fix concurrent access to exec env: it is accessed by the thread setting the suspend flags (to stop other threads) and threads (in the same cluster) reading it to check for termination.

Tested it by using the thread sanitizer and running WASI tests in the proposal https://github.com/WebAssembly/wasi-threads/tree/main/test/testsuite.

core/iwasm/interpreter/wasm_interp_classic.c

core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c

core/iwasm/common/wasm_exec_env.c

wenyongh · 2023-02-27T09:44:15Z

core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c

    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);


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);

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.

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);

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.

OK, got it, thanks.

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

…cess

wenyongh

LGTM

We have observed a significant performance degradation after merging bytecodealliance#1991 Instead of protecting suspend flags with a mutex, we implement the flags as atomic variable and only use mutex when atomics are not available on a given platform.

We have observed a significant performance degradation after merging #1991 Instead of protecting suspend flags with a mutex, we implement the flags as atomic variable and only use mutex when atomics are not available on a given platform.

…iance#1991) Data race may occur when accessing exec_env's fields, e.g. suspend_flags and handle. Add lock `exec_env->wait_lock` for them to resolve the issue.

We have observed a significant performance degradation after merging bytecodealliance#1991 Instead of protecting suspend flags with a mutex, we implement the flags as atomic variable and only use mutex when atomics are not available on a given platform.

eloparco mentioned this pull request Feb 26, 2023

Update WASI thread proposal tests in CI #1985

Merged

eloparco force-pushed the eloparco/exec-env-lock branch from 0f31f28 to 9df7a5b Compare February 27, 2023 01:55

fix(threads): protect access to exec env

de86504

eloparco force-pushed the eloparco/exec-env-lock branch from 9df7a5b to de86504 Compare February 27, 2023 02:17

wenyongh reviewed Feb 27, 2023

View reviewed changes

core/iwasm/interpreter/wasm_interp_classic.c Outdated Show resolved Hide resolved

core/iwasm/libraries/libc-wasi/libc_wasi_wrapper.c Outdated Show resolved Hide resolved

core/iwasm/common/wasm_exec_env.c Outdated Show resolved Hide resolved

eloparco force-pushed the eloparco/exec-env-lock branch 2 times, most recently from d44f851 to 5550b56 Compare February 27, 2023 09:42

wenyongh reviewed Feb 27, 2023

View reviewed changes

eloparco force-pushed the eloparco/exec-env-lock branch from 5550b56 to 15a2826 Compare February 27, 2023 10:15

fix: use exec env lock instead of cluster lock to protect exec env ac…

3ff2609

…cess

eloparco force-pushed the eloparco/exec-env-lock branch from 15a2826 to 3ff2609 Compare February 27, 2023 11:12

wenyongh approved these changes Feb 27, 2023

View reviewed changes

wenyongh merged commit 52e26e5 into bytecodealliance:main Feb 27, 2023

xujuntwt95329 mentioned this pull request Mar 19, 2023

fix dead lock in source debugger #2040

Merged

loganek mentioned this pull request Jul 14, 2023

Implement suspend flags as atomic variable #2361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect access to exec env #1991

Protect access to exec env #1991

eloparco commented Feb 26, 2023 •

edited

Loading

wenyongh Feb 27, 2023

eloparco Feb 27, 2023

wenyongh Feb 27, 2023

eloparco Feb 27, 2023 •

edited

Loading

wenyongh Feb 27, 2023

eloparco Feb 27, 2023

wenyongh left a comment

	/* 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);

Protect access to exec env #1991

Protect access to exec env #1991

Conversation

eloparco commented Feb 26, 2023 • edited Loading

wenyongh Feb 27, 2023

Choose a reason for hiding this comment

eloparco Feb 27, 2023

Choose a reason for hiding this comment

wenyongh Feb 27, 2023

Choose a reason for hiding this comment

eloparco Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

wenyongh Feb 27, 2023

Choose a reason for hiding this comment

eloparco Feb 27, 2023

Choose a reason for hiding this comment

wenyongh left a comment

Choose a reason for hiding this comment

eloparco commented Feb 26, 2023 •

edited

Loading

eloparco Feb 27, 2023 •

edited

Loading