Protect access to exec env

[eloparco]

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.

[wenyongh]

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

[eloparco]

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.

[wenyongh]

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

[eloparco]

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.

[wenyongh]

OK, got it, thanks.

[eloparco]

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

[wenyongh]

LGTM