[PATCH 1/5] winedbg: Add _snscanf function to safely parse packets.
We don't build winedbg with msvcrt so it's not actually available but we can fake it instead. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 052e73b2ad69..ddd2c60e25ea 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -120,6 +120,21 @@ static struct be_process_io be_process_gdbproxy_io = * =============================================== * */ +static int _snscanf(char* src, size_t len, const char* format, ...) +{ + int n; + char c = src[len]; + va_list args; + + src[len] = '\0'; + va_start(args, format); + n = vsscanf(src, format, args); + va_end(args); + src[len] = c; + + return n; +} + static inline int hex_from0(char ch) { if (ch >= '0' && ch <= '9') return ch - '0'; @@ -1201,8 +1216,7 @@ 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 (_snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p,%x", &addr, &len) != 2) return packet_error; if (len <= 0) return packet_error; TRACE("Read %u bytes at %p\n", len, addr); for (nread = 0; nread < len; nread += r, addr += r) @@ -1240,7 +1254,7 @@ static enum packet_return packet_write_memory(struct gdb_context* gdbctx) } *ptr++ = '\0'; - if (sscanf(gdbctx->in_packet, "%p,%x", &addr, &len) != 2) + if (_snscanf(gdbctx->in_packet, gdbctx->in_packet_len, "%p,%x", &addr, &len) != 2) { ERR("Failed to parse %s\n", debugstr_a(gdbctx->in_packet)); return packet_error; -- 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 | 147 +++++++++++++++++------------------- 1 file changed, 68 insertions(+), 79 deletions(-) diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index ddd2c60e25ea..66b1ed626030 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1761,94 +1761,83 @@ 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 char 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)) && + _snscanf(sum, end - 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; - 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 - { - /* 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"); - } + ERR("Nacking: %s (checksum: %d != %d)\n", debugstr_an(ptr, sum - ptr), + cksum, checksum(ptr + 1, len)); + write(gdbctx->sock, "-", 1); } - else + } + + while ((ret & packet_last_f) == 0 && + (ptr = memchr(gdbctx->in_buf, '$', gdbctx->in_len)) && + (sum = memchr(ptr, '#', end - ptr)) && + _snscanf(sum, end - 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; + + TRACE("Handling: %s\n", debugstr_an(ptr, sum - ptr)); + if (cksum == checksum(ptr + 1, len)) { - write(gdbctx->sock, "+", 1); - ERR("Dropping packet; invalid checksum %d <> %d\n", in_cksum, loc_cksum); + ret = packet_error; + gdbctx->in_packet = ptr + 2; + gdbctx->in_packet_len = len - 1; + + 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) + { + 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; } - gdbctx->in_len -= plen + 4; - memmove(gdbctx->in_buf, end + 3, gdbctx->in_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 66b1ed626030..5c089a508040 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, @@ -1629,15 +1630,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': @@ -1674,6 +1671,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 */ @@ -1751,7 +1759,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 */ @@ -1772,7 +1780,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)) && _snscanf(sum, end - sum, "#%02x", &cksum) == 1) { @@ -2006,6 +2015,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 5c089a508040..41644f4049c1 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -1085,8 +1085,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 41644f4049c1..8f0038647810 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -829,15 +829,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); @@ -855,19 +852,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 @@ -1174,10 +1169,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
Rémi Bernon <rbernon(a)codeweavers.com> writes:
We don't build winedbg with msvcrt so it's not actually available but we can fake it instead.
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- programs/winedbg/gdbproxy.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
There are other functions that would have the same issue, I'd suggest to always null-terminate the packet before parsing it. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Rémi Bernon