[PATCH v2 1/6] winedbg: Force read data to be NUL terminated.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 052e73b2ad69..e2954c08dddf 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1848,12 +1848,14 @@ static int fetch_data(struct gdb_context* gdbctx) if (gdbctx->in_len + STEP > gdbctx->in_buf_alloc) gdbctx->in_buf = packet_realloc(gdbctx->in_buf, gdbctx->in_buf_alloc += STEP); #undef STEP - len = read(gdbctx->sock, gdbctx->in_buf + gdbctx->in_len, gdbctx->in_buf_alloc - gdbctx->in_len); + len = read(gdbctx->sock, gdbctx->in_buf + gdbctx->in_len, gdbctx->in_buf_alloc - gdbctx->in_len - 1); if (len <= 0) break; gdbctx->in_len += len; assert(gdbctx->in_len <= gdbctx->in_buf_alloc); if (len < gdbctx->in_buf_alloc - gdbctx->in_len) break; } + + gdbctx->in_buf[gdbctx->in_len] = '\0'; return gdbctx->in_len - in_len; } -- 2.26.0.rc2
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e2954c08dddf..fde6c556c26d 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1201,7 +1201,6 @@ static enum packet_return packet_read_memory(struct gdb_context* gdbctx) SIZE_T r = 0; assert(gdbctx->in_trap); - /* FIXME:check in_packet_len for reading %p,%x */ if (sscanf(gdbctx->in_packet, "%p,%x", &addr, &len) != 2) return packet_error; if (len <= 0) return packet_error; TRACE("Read %u bytes at %p\n", len, addr); @@ -1794,6 +1793,7 @@ static BOOL extract_packets(struct gdb_context* gdbctx) { gdbctx->in_packet = gdbctx->in_buf + 2; gdbctx->in_packet_len = plen - 1; + gdbctx->in_packet[gdbctx->in_packet_len] = '\0'; ret = (packet_entries[i].handler)(gdbctx); } switch (ret & ~packet_last_f) -- 2.26.0.rc2
Sometimes multiple packets are received and we were assuming it was some repeated requests due to slow ack. We can ack packets first. It was also dropping some perfectly valid packets and we should process them all. For instance, lldb frontend sometimes send multiple packets at a the same time and expects them to be handled. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 151 +++++++++++++++++------------------- 1 file changed, 72 insertions(+), 79 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index fde6c556c26d..211d39edd596 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1746,95 +1746,88 @@ static struct packet_entry packet_entries[] = static BOOL extract_packets(struct gdb_context* gdbctx) { - char* end; - int plen; - unsigned char in_cksum, loc_cksum; - char* ptr; - enum packet_return ret = packet_error; - int num_packet = 0; + char *ptr, *sum = gdbctx->in_buf, *end = gdbctx->in_buf + gdbctx->in_len; + enum packet_return ret = packet_error; + unsigned int cksum; + int i, len; + + /* ptr points to the beginning ('$') of the current packet + * sum points to the beginning ('#') of the current packet checksum ("#xx") + * len is the length of the current packet data (sum - ptr - 1) + * end points to the end of the received data buffer + */ - while ((ret & packet_last_f) == 0) + while ((ptr = memchr(sum, '$', end - sum)) && + (sum = memchr(ptr, '#', end - ptr)) && + (end - sum >= 3) && sscanf(sum, "#%02x", &cksum) == 1) { - TRACE("Packet: %s\n", debugstr_an(gdbctx->in_buf, gdbctx->in_len)); - ptr = memchr(gdbctx->in_buf, '$', gdbctx->in_len); - if (ptr == NULL) return FALSE; - if (ptr != gdbctx->in_buf) + len = sum - ptr - 1; + sum += 3; + + if (cksum == checksum(ptr + 1, len)) { - int glen = ptr - gdbctx->in_buf; /* garbage len */ - WARN("Removing garbage: %s\n", debugstr_an(gdbctx->in_buf, glen)); - gdbctx->in_len -= glen; - memmove(gdbctx->in_buf, ptr, gdbctx->in_len); + TRACE("Acking: %s\n", debugstr_an(ptr, sum - ptr)); + write(gdbctx->sock, "+", 1); } - end = memchr(gdbctx->in_buf + 1, '#', gdbctx->in_len); - if (end == NULL) return FALSE; - /* no checksum yet */ - if (end + 3 > gdbctx->in_buf + gdbctx->in_len) return FALSE; - plen = end - gdbctx->in_buf - 1; - hex_from(&in_cksum, end + 1, 1); - loc_cksum = checksum(gdbctx->in_buf + 1, plen); - if (loc_cksum == in_cksum) + else { - if (num_packet == 0) { - int i; - - ret = packet_error; - - write(gdbctx->sock, "+", 1); - assert(plen); - - /* FIXME: should use bsearch if packet_entries was sorted */ - for (i = 0; i < ARRAY_SIZE(packet_entries); i++) - { - if (packet_entries[i].key == gdbctx->in_buf[1]) break; - } - if (i == ARRAY_SIZE(packet_entries)) - WARN("Unhandled packet %s\n", debugstr_an(&gdbctx->in_buf[1], plen)); - else - { - gdbctx->in_packet = gdbctx->in_buf + 2; - gdbctx->in_packet_len = plen - 1; - gdbctx->in_packet[gdbctx->in_packet_len] = '\0'; - ret = (packet_entries[i].handler)(gdbctx); - } - switch (ret & ~packet_last_f) - { - case packet_error: packet_reply(gdbctx, ""); break; - case packet_ok: packet_reply(gdbctx, "OK"); break; - case packet_done: break; - } - TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf, gdbctx->out_len)); - i = write(gdbctx->sock, gdbctx->out_buf, gdbctx->out_len); - assert(i == gdbctx->out_len); - /* if this fails, we'll have to use POLLOUT... - */ - gdbctx->out_len = 0; - num_packet++; - } - else + ERR("Nacking: %s (checksum: %d != %d)\n", debugstr_an(ptr, sum - ptr), + cksum, checksum(ptr + 1, len)); + write(gdbctx->sock, "-", 1); + } + } + + while ((ret & packet_last_f) == 0 && + (ptr = memchr(gdbctx->in_buf, '$', gdbctx->in_len)) && + (sum = memchr(ptr, '#', end - ptr)) && + (end - sum >= 3) && sscanf(sum, "#%02x", &cksum) == 1) + { + if (ptr != gdbctx->in_buf) + WARN("Ignoring: %s\n", debugstr_an(gdbctx->in_buf, ptr - gdbctx->in_buf)); + + len = sum - ptr - 1; + sum += 3; + + if (cksum == checksum(ptr + 1, len)) + { + TRACE("Handling: %s\n", debugstr_an(ptr, sum - ptr)); + + ret = packet_error; + gdbctx->in_packet = ptr + 2; + gdbctx->in_packet_len = len - 1; + gdbctx->in_packet[gdbctx->in_packet_len] = '\0'; + + for (i = 0; i < ARRAY_SIZE(packet_entries); i++) + if (packet_entries[i].key == ptr[1]) + break; + + if (i == ARRAY_SIZE(packet_entries)) + WARN("Unhandled: %s\n", debugstr_an(ptr + 1, len)); + else if (((ret = (packet_entries[i].handler)(gdbctx)) & ~packet_last_f) == packet_error) + WARN("Failed: %s\n", debugstr_an(ptr + 1, len)); + + switch (ret & ~packet_last_f) { - /* FIXME: If we have more than one packet in our input buffer, - * it's very likely that we took too long to answer to a given packet - * and gdb is sending us the same packet again. - * So we simply drop the second packet. This will lower the risk of error, - * but there are still some race conditions here. - * A better fix (yet not perfect) would be to have two threads: - * - one managing the packets for gdb - * - the second one managing the commands... - * This would allow us to send the reply with the '+' character (Ack of - * the command) way sooner than we do now. - */ - ERR("Dropping packet; I was too slow to respond\n"); + case packet_error: packet_reply(gdbctx, ""); break; + case packet_ok: packet_reply(gdbctx, "OK"); break; + case packet_done: break; } + + TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf, gdbctx->out_len)); + i = write(gdbctx->sock, gdbctx->out_buf, gdbctx->out_len); + assert(i == gdbctx->out_len); + gdbctx->out_len = 0; } else - { - write(gdbctx->sock, "+", 1); - ERR("Dropping packet; invalid checksum %d <> %d\n", in_cksum, loc_cksum); - } - gdbctx->in_len -= plen + 4; - memmove(gdbctx->in_buf, end + 3, gdbctx->in_len); + WARN("Ignoring: %s (checksum: %d != %d)\n", debugstr_an(ptr, sum - ptr), + cksum, checksum(ptr + 1, len)); + + gdbctx->in_len = end - sum; + memmove(gdbctx->in_buf, sum, end - sum); + end = gdbctx->in_buf + gdbctx->in_len; } - return TRUE; + + return (ret & packet_last_f); } static int fetch_data(struct gdb_context* gdbctx) -- 2.26.0.rc2
We don't have to validate and acknowledge the packets as long as this mode is enabled, this will reduce verbosity especially when tracing. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 211d39edd596..5e9e274beb90 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -94,6 +94,7 @@ struct gdb_context struct dbg_process* process; /* Unix environment */ unsigned long wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ + BOOL no_ack_mode; }; static BOOL tgt_process_gdbproxy_read(HANDLE hProcess, const void* addr, @@ -1614,15 +1615,11 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_ok; if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) { - if (*target_xml) - return packet_reply(gdbctx, "PacketSize=400;qXfer:features:read+"); - else - { - /* no features supported */ - packet_reply_open(gdbctx); - packet_reply_close(gdbctx); - return packet_done; - } + packet_reply_open(gdbctx); + packet_reply_add(gdbctx, "QStartNoAckMode+;"); + if (*target_xml) packet_reply_add(gdbctx, "PacketSize=400;qXfer:features:read+"); + packet_reply_close(gdbctx); + return packet_done; } break; case 'T': @@ -1659,6 +1656,17 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_error; } +static enum packet_return packet_set(struct gdb_context* gdbctx) +{ + if (strncmp(gdbctx->in_packet, "StartNoAckMode", 14) == 0) + { + gdbctx->no_ack_mode = TRUE; + return packet_ok; + } + + return packet_error; +} + static enum packet_return packet_step(struct gdb_context* gdbctx) { /* FIXME: add support for address in packet */ @@ -1736,7 +1744,7 @@ static struct packet_entry packet_entries[] = {'p', packet_read_register}, {'P', packet_write_register}, {'q', packet_query}, - /* {'Q', packet_set}, */ + {'Q', packet_set}, /* {'R', packet,restart}, only in extended mode ! */ {'s', packet_step}, /*{'S', packet_step_signal}, hard(er) to implement */ @@ -1757,7 +1765,8 @@ static BOOL extract_packets(struct gdb_context* gdbctx) * end points to the end of the received data buffer */ - while ((ptr = memchr(sum, '$', end - sum)) && + while (!gdbctx->no_ack_mode && + (ptr = memchr(sum, '$', end - sum)) && (sum = memchr(ptr, '#', end - ptr)) && (end - sum >= 3) && sscanf(sum, "#%02x", &cksum) == 1) { @@ -1998,6 +2007,7 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne gdbctx->last_sig = 0; gdbctx->in_trap = FALSE; gdbctx->process = NULL; + gdbctx->no_ack_mode = FALSE; for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0; -- 2.26.0.rc2
We now always print a warning when packet_error is returned. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 5e9e274beb90..a9f9210f5291 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1070,8 +1070,9 @@ static enum packet_return packet_verbose(struct gdb_context* gdbctx) return packet_verbose_cont(gdbctx); } - WARN("Unhandled verbose packet %s\n", - debugstr_an(gdbctx->in_packet, gdbctx->in_packet_len)); + if (gdbctx->in_packet_len == 14 && !memcmp(gdbctx->in_packet, "MustReplyEmpty", 14)) + return packet_reply(gdbctx, ""); + return packet_error; } -- 2.26.0.rc2
There's a special packet_last_f flag to indicate we should quit, use that on kill packet instead of exiting abruptly. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index a9f9210f5291..02522ea31955 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -814,15 +814,12 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, unsi static enum packet_return packet_reply_status(struct gdb_context* gdbctx) { - enum packet_return ret = packet_done; - - packet_reply_open(gdbctx); - if (gdbctx->process != NULL) { unsigned char sig; unsigned i; + packet_reply_open(gdbctx); packet_reply_add(gdbctx, "T"); sig = gdbctx->last_sig; packet_reply_val(gdbctx, sig, 1); @@ -840,19 +837,17 @@ static enum packet_return packet_reply_status(struct gdb_context* gdbctx) packet_reply_register_hex_to(gdbctx, i); packet_reply_add(gdbctx, ";"); } + + packet_reply_close(gdbctx); + return packet_done; } else { /* Try to put an exit code * Cannot use GetExitCodeProcess, wouldn't fit in a 8 bit value, so * just indicate the end of process and exit */ - packet_reply_add(gdbctx, "W00"); - /*if (!gdbctx->extended)*/ ret |= packet_last_f; + return packet_reply(gdbctx, "W00") | packet_last_f; } - - packet_reply_close(gdbctx); - - return ret; } #if 0 @@ -1159,10 +1154,7 @@ static enum packet_return packet_kill(struct gdb_context* gdbctx) if (!gdbctx->extended) /* dunno whether GDB cares or not */ #endif - wait(NULL); - exit(0); - /* assume we can't really answer something here */ - /* return packet_done; */ + return packet_ok | packet_last_f; } static enum packet_return packet_thread(struct gdb_context* gdbctx) -- 2.26.0.rc2
participants (1)
-
Rémi Bernon