Skip to content

Fix wait_info data race for deletion + fix atomic_wait logic #2016

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 15 commits into from
Mar 13, 2023
Merged

Fix wait_info data race for deletion + fix atomic_wait logic #2016

Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c0f13de
Fix wait_info data race for deletion
g0djan Mar 8, 2023
8fdbc12
clang format
g0djan Mar 8, 2023
b30a14c
Wait_info->wait_list access always should be under its lock actually
g0djan Mar 8, 2023
962e46f
forgot to set initial count_acquisition to 1
g0djan Mar 8, 2023
327c1a3
Merge branch 'main' into godjan/fix-wait-info-data-race
g0djan Mar 10, 2023
2f69b0d
use wait_list->len, not counter
g0djan Mar 10, 2023
3e1a974
clang format
g0djan Mar 10, 2023
bc9fa8b
Fix wromg waiting on atomic when proc_exit was called before thread r…
g0djan Mar 11, 2023
388684d
clang-format
g0djan Mar 11, 2023
8bf4aab
remove timeouts to catch actual errors for atomic_wait with BLOCKING_…
g0djan Mar 11, 2023
1efcb64
clang-format-12
g0djan Mar 11, 2023
bcd639d
fix passed args for fast interpreter
g0djan Mar 11, 2023
6c21413
Address review comments
g0djan Mar 11, 2023
e81fa95
Added warning about using wait-info->wait_list
g0djan Mar 11, 2023
2970e7d
fix comments
g0djan Mar 11, 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
75 changes: 41 additions & 34 deletions core/iwasm/common/wasm_shared_memory.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ notify_wait_list(bh_list *wait_list, uint32 count)
}

static AtomicWaitInfo *
acquire_wait_info(void *address, bool create)
acquire_wait_info(void *address, bool create, AtomicWaitNode *wait_node)
Comment thread
wenyongh marked this conversation as resolved.
Outdated
{
AtomicWaitInfo *wait_info = NULL;
bh_list_status ret;
Expand All @@ -308,7 +308,7 @@ acquire_wait_info(void *address, bool create)
if (address)
wait_info = (AtomicWaitInfo *)bh_hash_map_find(wait_map, address);

if (!create) {
if (!create && !wait_info) {
os_mutex_unlock(&wait_map_lock);
return wait_info;
}
Expand All @@ -335,6 +335,13 @@ acquire_wait_info(void *address, bool create)
goto fail3;
}
}
if (wait_node) {
os_mutex_lock(&wait_info->wait_list_lock);
ret = bh_list_insert(wait_info->wait_list, wait_node);
os_mutex_unlock(&wait_info->wait_list_lock);
bh_assert(ret == BH_LIST_SUCCESS);
(void)ret;
}

os_mutex_unlock(&wait_map_lock);

Expand Down Expand Up @@ -380,11 +387,17 @@ static bool
map_remove_wait_info(HashMap *wait_map_, AtomicWaitInfo *wait_info,
void *address)
{
os_mutex_lock(&wait_map_lock);
Comment thread
g0djan marked this conversation as resolved.
os_mutex_lock(&wait_info->wait_list_lock);
if (wait_info->wait_list->len > 0) {
Comment thread
g0djan marked this conversation as resolved.
os_mutex_unlock(&wait_info->wait_list_lock);
os_mutex_unlock(&wait_map_lock);
return false;
}
os_mutex_unlock(&wait_info->wait_list_lock);
Comment thread
wenyongh marked this conversation as resolved.

bh_hash_map_remove(wait_map_, address, NULL, NULL);
os_mutex_unlock(&wait_map_lock);
return true;
}

Expand Down Expand Up @@ -418,14 +431,6 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address,
return -1;
}

/* acquire the wait info, create new one if not exists */
wait_info = acquire_wait_info(address, true);

if (!wait_info) {
wasm_runtime_set_exception(module, "failed to acquire wait_info");
return -1;
}

node = search_module((WASMModuleCommon *)module_inst->module);
os_mutex_lock(&node->shared_mem_lock);
no_wait = (!wait64 && *(uint32 *)address != (uint32)expect)
Expand All @@ -435,32 +440,34 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address,
if (no_wait) {
return 1;
}
else {
bh_list_status ret;

if (!(wait_node = wasm_runtime_malloc(sizeof(AtomicWaitNode)))) {
wasm_runtime_set_exception(module, "failed to create wait node");
return -1;
}
memset(wait_node, 0, sizeof(AtomicWaitNode));
if (!(wait_node = wasm_runtime_malloc(sizeof(AtomicWaitNode)))) {
wasm_runtime_set_exception(module, "failed to create wait node");
return -1;
}
memset(wait_node, 0, sizeof(AtomicWaitNode));

if (0 != os_mutex_init(&wait_node->wait_lock)) {
wasm_runtime_free(wait_node);
return -1;
}
if (0 != os_mutex_init(&wait_node->wait_lock)) {
wasm_runtime_free(wait_node);
return -1;
}

if (0 != os_cond_init(&wait_node->wait_cond)) {
os_mutex_destroy(&wait_node->wait_lock);
wasm_runtime_free(wait_node);
return -1;
}
if (0 != os_cond_init(&wait_node->wait_cond)) {
os_mutex_destroy(&wait_node->wait_lock);
wasm_runtime_free(wait_node);
return -1;
}

wait_node->status = S_WAITING;
os_mutex_lock(&wait_info->wait_list_lock);
ret = bh_list_insert(wait_info->wait_list, wait_node);
os_mutex_unlock(&wait_info->wait_list_lock);
bh_assert(ret == BH_LIST_SUCCESS);
(void)ret;
wait_node->status = S_WAITING;

/* acquire the wait info, create new one if not exists */
wait_info = acquire_wait_info(address, true, wait_node);
Comment thread
wenyongh marked this conversation as resolved.
Outdated

if (!wait_info) {
os_mutex_destroy(&wait_node->wait_lock);
wasm_runtime_free(wait_node);
wasm_runtime_set_exception(module, "failed to acquire wait_info");
return -1;
}

/* condition wait start */
Expand All @@ -486,8 +493,8 @@ wasm_runtime_atomic_wait(WASMModuleInstanceCommon *module, void *address,
wasm_runtime_free(wait_node);

/* Release wait info if no wait nodes attached */
removed_from_map = map_remove_wait_info(wait_map, wait_info, address);
os_mutex_unlock(&wait_info->wait_list_lock);
removed_from_map = map_remove_wait_info(wait_map, wait_info, address);
if (removed_from_map)
destroy_wait_info(wait_info);
Comment thread
wenyongh marked this conversation as resolved.
Outdated
os_mutex_unlock(&node->shared_mem_lock);
Expand Down Expand Up @@ -523,7 +530,7 @@ wasm_runtime_atomic_notify(WASMModuleInstanceCommon *module, void *address,
return -1;
}

wait_info = acquire_wait_info(address, false);
wait_info = acquire_wait_info(address, false, NULL);

/* Nobody wait on this address */
if (!wait_info) {
Expand Down