From 40df96bcf6a27993da9bbe2984a83fef907e29d6 Mon Sep 17 00:00:00 2001 From: Xu <693788454@qq.com> Date: Wed, 24 Nov 2021 22:38:04 +0800 Subject: [PATCH 1/2] fix duplicate debug instance id in multiple native thread issue The debug_instance_list is not protected by mutex, and the debug instance use the length of this list as its id. If several threads create debug instance at the same time, they may get duplicate ids. This patch add mutex to protect this list --- .../libraries/debug-engine/debug_engine.c | 67 +++++++++++++++---- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/core/iwasm/libraries/debug-engine/debug_engine.c b/core/iwasm/libraries/debug-engine/debug_engine.c index 9694c7d789..827a35e357 100644 --- a/core/iwasm/libraries/debug-engine/debug_engine.c +++ b/core/iwasm/libraries/debug-engine/debug_engine.c @@ -21,11 +21,28 @@ typedef struct WASMDebugEngine { int platform_port; int process_base_port; bh_list debug_instance_list; + korp_mutex instance_list_lock; bool active; } WASMDebugEngine; static WASMDebugEngine *g_debug_engine; +static uint32 current_instance_id = 1; + +static uint32 +allocate_instance_id() +{ + uint32 id; + + bh_assert(g_debug_engine); + + os_mutex_lock(&g_debug_engine->instance_list_lock); + id = current_instance_id++; + os_mutex_unlock(&g_debug_engine->instance_list_lock); + + return id; +} + static bool should_stop(WASMDebugControlThread *control_thread) { @@ -54,7 +71,7 @@ control_thread_routine(void *arg) control_thread->status = RUNNING; - debug_inst->id = g_debug_engine->debug_instance_list.len + 1; + debug_inst->id = allocate_instance_id(); control_thread->debug_engine = g_debug_engine; control_thread->debug_instance = debug_inst; @@ -137,8 +154,11 @@ wasm_debug_control_thread_create(WASMDebugInstance *debug_instance) if (!control_thread->server) goto fail1; + os_mutex_lock(&g_debug_engine->instance_list_lock); /* create control thread success, append debug instance to debug engine */ bh_list_insert(&g_debug_engine->debug_instance_list, debug_instance); + os_mutex_unlock(&g_debug_engine->instance_list_lock); + wasm_cluster_send_signal_all(debug_instance->cluster, WAMR_SIG_STOP); return control_thread; @@ -177,6 +197,14 @@ wasm_debug_engine_create() } memset(engine, 0, sizeof(WASMDebugEngine)); + if (os_mutex_init(&engine->instance_list_lock) != 0) { + LOG_ERROR("WASM Debug Engine error: failed to init mutex"); + return NULL; + } + + /* reset current instance id */ + current_instance_id = 1; + /* TODO: support Wasm platform in LLDB */ /* engine->control_thread = @@ -191,6 +219,16 @@ wasm_debug_engine_create() return engine; } +void +wasm_debug_engine_destroy() +{ + if (g_debug_engine) { + os_mutex_destroy(&g_debug_engine->instance_list_lock); + wasm_runtime_free(g_debug_engine); + g_debug_engine = NULL; + } +} + bool wasm_debug_engine_init(char *ip_addr, int platform_port, int process_port) { @@ -230,15 +268,6 @@ wasm_debug_get_engine_active(void) return false; } -void -wasm_debug_engine_destroy() -{ - if (g_debug_engine) { - wasm_runtime_free(g_debug_engine); - g_debug_engine = NULL; - } -} - /* A debug Instance is a debug "process" in gdb remote protocol and bound to a runtime cluster */ WASMDebugInstance * @@ -276,13 +305,19 @@ wasm_debug_instance_create(WASMCluster *cluster) static WASMDebugInstance * wasm_cluster_get_debug_instance(WASMDebugEngine *engine, WASMCluster *cluster) { - WASMDebugInstance *instance = - bh_list_first_elem(&engine->debug_instance_list); + WASMDebugInstance *instance; + + os_mutex_lock(&g_debug_engine->instance_list_lock); + instance = bh_list_first_elem(&engine->debug_instance_list); while (instance) { - if (instance->cluster == cluster) + if (instance->cluster == cluster) { + os_mutex_unlock(&g_debug_engine->instance_list_lock); return instance; + } instance = bh_list_elem_next(instance); } + os_mutex_unlock(&g_debug_engine->instance_list_lock); + return instance; } @@ -315,7 +350,10 @@ wasm_debug_instance_destroy(WASMCluster *cluster) if (instance) { /* destroy control thread */ wasm_debug_control_thread_destroy(instance); + + os_mutex_lock(&g_debug_engine->instance_list_lock); bh_list_remove(&g_debug_engine->debug_instance_list, instance); + os_mutex_unlock(&g_debug_engine->instance_list_lock); /* destroy all breakpoints */ wasm_debug_instance_destroy_breakpoints(instance); @@ -700,12 +738,15 @@ wasm_exec_env_get_instance(WASMExecEnv *exec_env) WASMDebugInstance *instance = NULL; bh_assert(g_debug_engine); + os_mutex_lock(&g_debug_engine->instance_list_lock); instance = bh_list_first_elem(&g_debug_engine->debug_instance_list); while (instance) { if (instance->cluster == exec_env->cluster) break; instance = bh_list_elem_next(instance); } + + os_mutex_unlock(&g_debug_engine->instance_list_lock); return instance; } From 066b18ae7eecd0673b4ab821d477938bdd5d87d8 Mon Sep 17 00:00:00 2001 From: Xu <693788454@qq.com> Date: Thu, 25 Nov 2021 11:11:47 +0800 Subject: [PATCH 2/2] address PR comment: free debug engine if init failed --- core/iwasm/libraries/debug-engine/debug_engine.c | 1 + 1 file changed, 1 insertion(+) diff --git a/core/iwasm/libraries/debug-engine/debug_engine.c b/core/iwasm/libraries/debug-engine/debug_engine.c index 827a35e357..d7b73298d2 100644 --- a/core/iwasm/libraries/debug-engine/debug_engine.c +++ b/core/iwasm/libraries/debug-engine/debug_engine.c @@ -198,6 +198,7 @@ wasm_debug_engine_create() memset(engine, 0, sizeof(WASMDebugEngine)); if (os_mutex_init(&engine->instance_list_lock) != 0) { + wasm_runtime_free(engine); LOG_ERROR("WASM Debug Engine error: failed to init mutex"); return NULL; }