Fix source debugging issues (#776)

- fix data race issue between debug control thread and main thread
- fix possible memory leaks in breakpoints list
- fix memory uninitialized issues
- remove unused data structures
- add more checks when handling packet and args
- fix mini-loader issues
- fix config_common.cmake fast interp prompt issue

build-scripts/config_common.cmake

@@ -145,7 +145,7 @@ elseif (WAMR_BUILD_LIBC_WASI EQUAL 1)
 else ()
   message ("     Libc WASI disabled")
 endif ()
-if (WAMR_BUILD_FAST_INTERP EQUAL 1)
+if ((WAMR_BUILD_FAST_INTERP EQUAL 1) AND (WAMR_BUILD_INTERP EQUAL 1))
   add_definitions (-DWASM_ENABLE_FAST_INTERP=1)
   message ("     Fast interpreter enabled")
 else ()

core/iwasm/interpreter/wasm_mini_loader.c

@@ -2438,7 +2438,7 @@ wasm_loader_unload(WASMModule *module)
 }
 
 bool
-wasm_loader_find_block_addr(BlockAddr *block_addr_cache,
+wasm_loader_find_block_addr(WASMExecEnv *exec_env, BlockAddr *block_addr_cache,
                             const uint8 *start_addr, const uint8 *code_end_addr,
                             uint8 label_type, uint8 **p_else_addr,
                             uint8 **p_end_addr)

core/iwasm/libraries/debug-engine/debug_engine.c

@@ -24,6 +24,8 @@ typedef struct WASMDebugEngine {
     bool active;
 } WASMDebugEngine;
 
+static WASMDebugEngine *g_debug_engine;
+
 static bool
 should_stop(WASMDebugControlThread *control_thread)
 {
@@ -33,69 +35,112 @@ should_stop(WASMDebugControlThread *control_thread)
 static void *
 control_thread_routine(void *arg)
 {
-    WASMDebugObject *debug_object = (WASMDebugObject *)arg;
+    WASMDebugInstance *debug_inst = (WASMDebugInstance *)arg;
+    WASMDebugControlThread *control_thread = NULL;
+    WASMCluster *cluster = NULL;
+    WASMExecEnv *exec_env;
+    bh_assert(debug_inst);
+
+    control_thread = debug_inst->control_thread;
+    bh_assert(control_thread);
+
+    cluster = debug_inst->cluster;
+    bh_assert(cluster);
+
+    exec_env = bh_list_first_elem(&cluster->exec_env_list);
+    bh_assert(exec_env);
+
+    os_mutex_lock(&exec_env->wait_lock);
+
+    control_thread->status = RUNNING;
+
+    debug_inst->id = g_debug_engine->debug_instance_list.len + 1;
+
+    control_thread->debug_engine = g_debug_engine;
+    control_thread->debug_instance = debug_inst;
+    strcpy(control_thread->ip_addr, g_debug_engine->ip_addr);
+    control_thread->port =
+        g_debug_engine->process_base_port + debug_inst->id;
 
     LOG_WARNING("control thread of debug object %p start at %s:%d\n",
-                debug_object, debug_object->control_thread->ip_addr,
-                debug_object->control_thread->port);
+                debug_inst, control_thread->ip_addr, control_thread->port);
 
-    debug_object->control_thread->server =
-      wasm_launch_gdbserver(debug_object->control_thread->ip_addr,
-                            debug_object->control_thread->port);
-    if (!debug_object->control_thread->server) {
+    control_thread->server =
+      wasm_launch_gdbserver(control_thread->ip_addr, control_thread->port);
+    if (!control_thread->server) {
         LOG_ERROR("Failed to create debug server\n");
+        os_cond_signal(&exec_env->wait_cond);
+        os_mutex_unlock(&exec_env->wait_lock);
         return NULL;
     }
 
-    debug_object->control_thread->server->thread =
-      debug_object->control_thread;
+    control_thread->server->thread = control_thread;
+
+    /* control thread ready, notify main thread */
+    os_cond_signal(&exec_env->wait_cond);
+    os_mutex_unlock(&exec_env->wait_lock);
 
     while (true) {
-        os_mutex_lock(&debug_object->control_thread->wait_lock);
-        if (!should_stop(debug_object->control_thread)) {
-            if (!wasm_gdbserver_handle_packet(
-                  debug_object->control_thread->server))
-                debug_object->control_thread->status = STOPPED;
+        os_mutex_lock(&control_thread->wait_lock);
+        if (!should_stop(control_thread)) {
+            if (!wasm_gdbserver_handle_packet(control_thread->server)) {
+                control_thread->status = STOPPED;
+            }
         }
         else {
-            os_mutex_unlock(&debug_object->control_thread->wait_lock);
+            os_mutex_unlock(&control_thread->wait_lock);
             break;
         }
-        os_mutex_unlock(&debug_object->control_thread->wait_lock);
+        os_mutex_unlock(&control_thread->wait_lock);
     }
 
-    LOG_VERBOSE("control thread of debug object %p stop\n", debug_object);
+    LOG_VERBOSE("control thread of debug object %p stop\n", debug_inst);
     return NULL;
 }
 
 static WASMDebugControlThread *
-wasm_debug_control_thread_create(WASMDebugObject *debug_object)
+wasm_debug_control_thread_create(WASMDebugInstance *debug_instance)
 {
     WASMDebugControlThread *control_thread;
+    WASMCluster *cluster = debug_instance->cluster;
+    WASMExecEnv *exec_env;
+    bh_assert(cluster);
+
+    exec_env = bh_list_first_elem(&cluster->exec_env_list);
+    bh_assert(exec_env);
 
     if (!(control_thread =
             wasm_runtime_malloc(sizeof(WASMDebugControlThread)))) {
         LOG_ERROR("WASM Debug Engine error: failed to allocate memory");
         return NULL;
     }
+    memset(control_thread, 0, sizeof(WASMDebugControlThread));
 
     if (os_mutex_init(&control_thread->wait_lock) != 0)
         goto fail;
 
-    if (os_cond_init(&control_thread->wait_cond) != 0)
-        goto fail1;
+    debug_instance->control_thread = control_thread;
 
-    control_thread->status = RUNNING;
+    os_mutex_lock(&exec_env->wait_lock);
 
     if (0 != os_thread_create(&control_thread->tid, control_thread_routine,
-                              debug_object, APP_THREAD_STACK_SIZE_MAX)) {
-        goto fail2;
+                              debug_instance, APP_THREAD_STACK_SIZE_MAX)) {
+        os_mutex_unlock(&control_thread->wait_lock);
+        goto fail1;
     }
 
+    /* wait until the debug control thread ready */
+    os_cond_wait(&exec_env->wait_cond, &exec_env->wait_lock);
+    os_mutex_unlock(&exec_env->wait_lock);
+    if (!control_thread->server)
+        goto fail1;
+
+    /* create control thread success, append debug instance to debug engine */
+    bh_list_insert(&g_debug_engine->debug_instance_list, debug_instance);
+    wasm_cluster_send_signal_all(debug_instance->cluster, WAMR_SIG_STOP);
+
     return control_thread;
 
-fail2:
-    os_cond_destroy(&control_thread->wait_cond);
 fail1:
     os_mutex_destroy(&control_thread->wait_lock);
 fail:
@@ -104,27 +149,23 @@ fail:
 }
 
 static void
-wasm_debug_control_thread_destroy(WASMDebugObject *debug_object)
+wasm_debug_control_thread_destroy(WASMDebugInstance *debug_instance)
 {
-    WASMDebugControlThread *control_thread = debug_object->control_thread;
+    WASMDebugControlThread *control_thread = debug_instance->control_thread;
     LOG_VERBOSE("control thread of debug object %p stop at %s:%d\n",
-                debug_object, debug_object->control_thread->ip_addr,
-                debug_object->control_thread->port);
-    os_mutex_lock(&control_thread->wait_lock);
+                debug_instance, control_thread->ip_addr,
+                control_thread->port);
     control_thread->status = STOPPED;
+    os_mutex_lock(&control_thread->wait_lock);
     wasm_close_gdbserver(control_thread->server);
     os_mutex_unlock(&control_thread->wait_lock);
-    os_cond_signal(&control_thread->wait_cond);
     os_thread_join(control_thread->tid, NULL);
     wasm_runtime_free(control_thread->server);
 
     os_mutex_destroy(&control_thread->wait_lock);
-    os_cond_destroy(&control_thread->wait_cond);
     wasm_runtime_free(control_thread);
 }
 
-static WASMDebugEngine *g_debug_engine;
-
 static WASMDebugEngine *
 wasm_debug_engine_create()
 {
@@ -215,24 +256,20 @@ wasm_debug_instance_create(WASMCluster *cluster)
         return NULL;
     }
     memset(instance, 0, sizeof(WASMDebugInstance));
-    instance->cluster = cluster;
-    instance->control_thread =
-      wasm_debug_control_thread_create((WASMDebugObject *)instance);
-    instance->control_thread->debug_engine = (WASMDebugObject *)g_debug_engine;
-    instance->control_thread->debug_instance = (WASMDebugObject *)instance;
-    strcpy(instance->control_thread->ip_addr, g_debug_engine->ip_addr);
-    instance->id = g_debug_engine->debug_instance_list.len + 1;
+    bh_list_init(&instance->break_point_list);
 
+    instance->cluster = cluster;
     exec_env = bh_list_first_elem(&cluster->exec_env_list);
+    bh_assert(exec_env);
 
-    /* exec_evn is created, but handle may not be set yet. */
-    instance->current_tid = exec_env ? exec_env->handle : 0;
+    instance->current_tid = exec_env->handle;
+
+    if (!wasm_debug_control_thread_create(instance)) {
+        LOG_ERROR("WASM Debug Engine error: failed to create control thread");
+        wasm_runtime_free(instance);
+        return NULL;
+    }
 
-    instance->control_thread->port =
-      g_debug_engine->process_base_port + instance->id;
-    bh_list_init(&instance->break_point_list);
-    bh_list_insert(&g_debug_engine->debug_instance_list, instance);
-    wasm_cluster_send_signal_all(instance->cluster, WAMR_SIG_STOP);
     return instance;
 }
 
@@ -249,6 +286,22 @@ wasm_cluster_get_debug_instance(WASMDebugEngine *engine, WASMCluster *cluster)
     return instance;
 }
 
+static void
+wasm_debug_instance_destroy_breakpoints(WASMDebugInstance *instance)
+{
+    WASMDebugBreakPoint *breakpoint, *next_bp;
+
+    breakpoint = bh_list_first_elem(&instance->break_point_list);
+    while (breakpoint) {
+        next_bp = bh_list_elem_next(breakpoint);
+
+        bh_list_remove(&instance->break_point_list, breakpoint);
+        wasm_runtime_free(breakpoint);
+
+        breakpoint = next_bp;
+    }
+}
+
 void
 wasm_debug_instance_destroy(WASMCluster *cluster)
 {
@@ -260,8 +313,13 @@ wasm_debug_instance_destroy(WASMCluster *cluster)
 
     instance = wasm_cluster_get_debug_instance(g_debug_engine, cluster);
     if (instance) {
-        wasm_debug_control_thread_destroy((WASMDebugObject *)instance);
+        /* destroy control thread */
+        wasm_debug_control_thread_destroy(instance);
         bh_list_remove(&g_debug_engine->debug_instance_list, instance);
+
+        /* destroy all breakpoints */
+        wasm_debug_instance_destroy_breakpoints(instance);
+
         wasm_runtime_free(instance);
     }
 }
@@ -444,6 +502,7 @@ wasm_debug_instance_get_memregion(WASMDebugInstance *instance, uint64 addr)
         LOG_ERROR("WASM Debug Engine error: failed to allocate memory");
         return NULL;
     }
+    memset(mem_info, 0, sizeof(WASMDebugMemoryInfo));
     mem_info->start = WASM_ADDR(WasmInvalid, 0, 0);
     mem_info->size = 0;
     mem_info->name[0] = '\0';
@@ -715,6 +774,7 @@ wasm_debug_instance_add_breakpoint(WASMDebugInstance *instance,
                   "WASM Debug Engine error: failed to allocate memory");
                 return false;
             }
+            memset(breakpoint, 0, sizeof(WASMDebugBreakPoint));
             breakpoint->addr = offset;
             /* TODO: how to if more than one breakpoints are set
                      at the same addr? */
@@ -872,6 +932,10 @@ wasm_debug_instance_get_local(WASMDebugInstance *instance,
         return false;
 
     param_count = cur_func->param_count;
+
+    if (local_index >= param_count + cur_func->local_count)
+        return false;
+
     local_offset = cur_func->local_offsets[local_index];
     if (local_index < param_count)
         local_type = cur_func->param_types[local_index];
@@ -928,6 +992,11 @@ wasm_debug_instance_get_global(WASMDebugInstance *instance,
     module_inst = (WASMModuleInstance *)exec_env->module_inst;
     global_data = module_inst->global_data;
     globals = module_inst->globals;
+
+    if ((global_index < 0)
+        || ((uint32)global_index >= module_inst->global_count)) {
+        return false;
+    }
     global = globals + global_index;
 
 #if WASM_ENABLE_MULTI_MODULE == 0

core/iwasm/libraries/debug-engine/debug_engine.h

@@ -15,24 +15,20 @@ typedef enum WASMDebugControlThreadStatus {
     STOPPED,
 } WASMDebugControlThreadStatus;
 
-struct WASMDebugObject;
+struct WASMDebugEngine;
+struct WASMDebugInstance;
+
 typedef struct WASMDebugControlThread {
     WASMGDBServer *server;
     korp_tid tid;
     korp_mutex wait_lock;
-    korp_cond wait_cond;
     char ip_addr[128];
     int port;
     WASMDebugControlThreadStatus status;
-    struct WASMDebugObject *debug_engine;
-    struct WASMDebugObject *debug_instance;
+    struct WASMDebugEngine *debug_engine;
+    struct WASMDebugInstance *debug_instance;
 } WASMDebugControlThread;
 
-typedef struct WASMDebugObject {
-    struct WASMDebugObject *next;
-    WASMDebugControlThread *control_thread;
-} WASMDebugObject;
-
 typedef struct WASMDebugBreakPoint {
     struct WASMDebugBreakPoint *next;
     uint64 addr;

core/iwasm/libraries/debug-engine/gdbserver.c

@@ -66,6 +66,8 @@ wasm_launch_gdbserver(char *host, int port)
         return NULL;
     }
 
+    memset(server, 0, sizeof(WASMGDBServer));
+
     listen_fd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
     if (listen_fd < 0) {
         LOG_ERROR("wasm gdb server error: socket() failed");
@@ -114,7 +116,7 @@ wasm_launch_gdbserver(char *host, int port)
     return server;
 
 fail:
-    if (listen_fd > 0) {
+    if (listen_fd >= 0) {
         shutdown(listen_fd, SHUT_RDWR);
         close(listen_fd);
     }
@@ -143,26 +145,41 @@ handler_packet(WASMGDBServer *server, char request, char *payload)
         packet_handler_table[(int)request].handler(server, payload);
 }
 
+/**
+ * The packet layout is:
+ *   '$' + payload + '#' + checksum(2bytes)
+ *                    ^
+ *                    packetend_ptr
+ */
 static void
 process_packet(WASMGDBServer *server)
 {
     uint8_t *inbuf = server->pkt.buf;
-    int inbuf_size = server->pkt.end;
+    int inbuf_size = server->pkt.size;
     uint8_t *packetend_ptr = (uint8_t *)memchr(inbuf, '#', inbuf_size);
     int packetend = packetend_ptr - inbuf;
-    bh_assert('$' == inbuf[0]);
     char request = inbuf[1];
-    char *payload = (char *)&inbuf[2];
+    char *payload = NULL;
+    uint8_t checksum = 0;
+
+    if (packetend == 1) {
+        LOG_VERBOSE("receive empty request, ignore it\n");
+        return;
+    }
+
+    bh_assert('$' == inbuf[0]);
     inbuf[packetend] = '\0';
 
-    uint8_t checksum = 0;
     for (int i = 1; i < packetend; i++)
         checksum += inbuf[i];
     bh_assert(checksum
               == (hex(inbuf[packetend + 1]) << 4 | hex(inbuf[packetend + 2])));
 
+    payload = (char *)&inbuf[2];
+
     LOG_VERBOSE("receive request:%c %s\n", request, payload);
     handler_packet(server, request, payload);
+
     inbuf_erase_head(server, packetend + 3);
 }
 

core/iwasm/libraries/debug-engine/gdbserver.h

@@ -20,7 +20,7 @@ enum GDBStoppointType {
 };
 typedef struct WasmDebugPacket {
     unsigned char buf[PACKET_BUF_SIZE];
-    unsigned int end;
+    unsigned int size;
 } WasmDebugPacket;
 
 struct WASMDebugControlThread;

core/iwasm/libraries/debug-engine/handler.c

@@ -51,7 +51,8 @@ process_xfer(WASMGDBServer *server, const char *name, char *args)
     const char *mode = args;
 
     args = strchr(args, ':');
-    *args++ = '\0';
+    if (args)
+        *args++ = '\0';
 
     if (!strcmp(name, "libraries") && !strcmp(mode, "read")) {
         //TODO: how to get current wasm file name?
@@ -145,9 +146,18 @@ handle_generay_query(WASMGDBServer *server, char *payload)
 
     if (!strcmp(name, "Xfer")) {
         name = args;
+
+        if (!args) {
+            LOG_ERROR("payload parse error during handle_generay_query");
+            return;
+        }
+
         args = strchr(args, ':');
-        *args++ = '\0';
-        process_xfer(server, name, args);
+
+        if (args) {
+            *args++ = '\0';
+            process_xfer(server, name, args);
+        }
     }
 
     if (!strcmp(name, "HostInfo")) {
@@ -202,7 +212,7 @@ handle_generay_query(WASMGDBServer *server, char *payload)
         write_packet(server, "");
     }
 
-    if (!strcmp(name, "MemoryRegionInfo")) {
+    if (args && (!strcmp(name, "MemoryRegionInfo"))) {
         uint64_t addr = strtol(args, NULL, 16);
         WASMDebugMemoryInfo *mem_info = wasm_debug_instance_get_memregion(
           (WASMDebugInstance *)server->thread->debug_instance, addr);
@@ -229,7 +239,7 @@ handle_generay_query(WASMGDBServer *server, char *payload)
         write_packet(server, "");
     }
 
-    if (!strcmp(name, "WasmCallStack")) {
+    if (args && (!strcmp(name, "WasmCallStack"))) {
         uint64_t tid = strtol(args, NULL, 16);
         uint64_t buf[1024 / sizeof(uint64_t)];
         uint64_t count = wasm_debug_instance_get_call_stack_pcs(
@@ -243,11 +253,11 @@ handle_generay_query(WASMGDBServer *server, char *payload)
             write_packet(server, "");
     }
 
-    if (!strcmp(name, "WasmLocal")) {
+    if (args && (!strcmp(name, "WasmLocal"))) {
         porcess_wasm_local(server, args);
     }
 
-    if (!strcmp(name, "WasmGlobal")) {
+    if (args && (!strcmp(name, "WasmGlobal"))) {
         porcess_wasm_global(server, args);
     }
 }
@@ -320,7 +330,7 @@ handle_v_packet(WASMGDBServer *server, char *payload)
         write_packet(server, "vCont;c;C;s;S;");
 
     if (!strcmp("Cont", name)) {
-        if (args[0] == 's') {
+        if (args && args[0] == 's') {
             char *numstring = strchr(args, ':');
             if (numstring) {
                 *numstring++ = '\0';
@@ -539,8 +549,13 @@ handle_malloc(WASMGDBServer *server, char *payload)
     sprintf(tmpbuf, "%s", "E03");
 
     args = strstr(payload, ",");
-    if (args)
+    if (args) {
         *args++ = '\0';
+    }
+    else {
+        LOG_ERROR("Payload parse error during handle malloc");
+        return;
+    }
 
     size = strtol(payload, NULL, 16);
     if (size > 0) {

core/iwasm/libraries/debug-engine/packets.c

@@ -15,33 +15,34 @@ pktbuf_insert(WASMGDBServer *gdbserver, const uint8_t *buf, ssize_t len)
 {
     WasmDebugPacket *pkt = &gdbserver->pkt;
 
-    if ((unsigned long)(pkt->end + len) >= sizeof(pkt->buf)) {
+    if ((unsigned long)(pkt->size + len) >= sizeof(pkt->buf)) {
         LOG_ERROR("Packet buffer overflow");
         exit(-2);
     }
-    memcpy(pkt->buf + pkt->end, buf, len);
-    pkt->end += len;
+
+    memcpy(pkt->buf + pkt->size, buf, len);
+    pkt->size += len;
 }
 
 void
-pktbuf_erase_head(WASMGDBServer *gdbserver, ssize_t end)
+pktbuf_erase_head(WASMGDBServer *gdbserver, ssize_t index)
 {
     WasmDebugPacket *pkt = &gdbserver->pkt;
-    memmove(pkt->buf, pkt->buf + end, pkt->end - end);
-    pkt->end -= end;
+    memmove(pkt->buf, pkt->buf + index, pkt->size - index);
+    pkt->size -= index;
 }
 
 void
-inbuf_erase_head(WASMGDBServer *gdbserver, ssize_t end)
+inbuf_erase_head(WASMGDBServer *gdbserver, ssize_t index)
 {
-    pktbuf_erase_head(gdbserver, end);
+    pktbuf_erase_head(gdbserver, index);
 }
 
 void
 pktbuf_clear(WASMGDBServer *gdbserver)
 {
     WasmDebugPacket *pkt = &gdbserver->pkt;
-    pkt->end = 0;
+    pkt->size = 0;
 }
 
 int
@@ -112,10 +113,17 @@ write_binary_packet(WASMGDBServer *gdbserver,
 {
     uint8_t *buf;
     ssize_t pfx_num_chars = strlen(pfx);
-    ssize_t buf_num_bytes = 0;
+    ssize_t buf_num_bytes = 0, total_size;
     int i;
 
-    buf = malloc(2 * num_bytes + pfx_num_chars);
+    total_size = 2 * num_bytes + pfx_num_chars;
+    buf = wasm_runtime_malloc(total_size);
+    if (!buf) {
+        LOG_ERROR("Failed to allocate memory for binary packet");
+        return;
+    }
+
+    memset(buf, 0, total_size);
     memcpy(buf, pfx, pfx_num_chars);
     buf_num_bytes += pfx_num_chars;
 
@@ -135,28 +143,31 @@ write_binary_packet(WASMGDBServer *gdbserver,
         }
     }
     write_packet_bytes(gdbserver, buf, buf_num_bytes);
-    free(buf);
+    wasm_runtime_free(buf);
 }
 
 bool
 skip_to_packet_start(WASMGDBServer *gdbserver)
 {
-    ssize_t end = -1;
+    ssize_t start_index = -1, i;
 
-    for (size_t i = 0; i < gdbserver->pkt.end; ++i)
+    for (i = 0; i < gdbserver->pkt.size; ++i) {
         if (gdbserver->pkt.buf[i] == '$') {
-            end = i;
+            start_index = i;
             break;
         }
+    }
 
-    if (end < 0) {
+    if (start_index < 0) {
         pktbuf_clear(gdbserver);
         return false;
     }
 
-    pktbuf_erase_head(gdbserver, end);
-    bh_assert(1 <= gdbserver->pkt.end);
+    pktbuf_erase_head(gdbserver, start_index);
+
+    bh_assert(1 <= gdbserver->pkt.size);
     bh_assert('$' == gdbserver->pkt.buf[0]);
+
     return true;
 }
 
@@ -164,7 +175,7 @@ bool
 read_packet(WASMGDBServer *gdbserver)
 {
     while (!skip_to_packet_start(gdbserver)) {
-        if(read_data_once(gdbserver) < 0)
+        if (read_data_once(gdbserver) < 0)
             return false;
     }
     if (!gdbserver->noack)