* [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive @ 2017-05-01 17:16 Doug Gale 2017-05-02 14:22 ` Stefan Hajnoczi 0 siblings, 1 reply; 9+ messages in thread From: Doug Gale @ 2017-05-01 17:16 UTC (permalink / raw) To: qemu-devel [-- Attachment #1: Type: text/plain, Size: 516 bytes --] The attached patch implements the GDB Remote Serial Protocol for command receive as per the documentation provided at https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html#Remote-Protocol and from inspection of remote.c in the gdb source (the documentation didn't clearly document whether the packet or the unescaped data were used for the checksum, turns out the packet data is used for the checksum, as expected). get_maintainer.pl didn't find a maintainer for gdbstub.c, so I didn't cc any maintainers. [-- Attachment #2: patch --] [-- Type: application/octet-stream, Size: 6782 bytes --] From a41a77dd013a663a39660e79e64fbf1ec0856e4d Mon Sep 17 00:00:00 2001 From: Doug Gale <doug16k@gmail.com> Date: Mon, 1 May 2017 12:22:10 -0400 Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for command receive - decode escape sequences - decompress run-length encoding escape sequences - report command parsing problems to output when debug output is enabled - reject packet checksums that are not valid hex digits - compute the checksum based on the packet stream, not based on the decoded packet Tested with GDB and QtCreator integrated debugger on SMP QEMU instance. Works for me. Signed-off-by: Doug Gale <doug16k@gmail.com> --- gdbstub.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 99 insertions(+), 10 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index 9911153..866a39f 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -271,7 +271,7 @@ static int gdb_signal_to_target (int sig) return -1; } -//#define DEBUG_GDB +#define DEBUG_GDB typedef struct GDBRegisterState { int base_reg; @@ -286,6 +286,8 @@ enum RSState { RS_INACTIVE, RS_IDLE, RS_GETLINE, + RS_GETLINE_ESC, + RS_GETLINE_RLE, RS_CHKSUM1, RS_CHKSUM2, }; @@ -296,7 +298,8 @@ typedef struct GDBState { enum RSState state; /* parsing state */ char line_buf[MAX_PACKET_LENGTH]; int line_buf_index; - int line_csum; + int line_sum; /* running checksum */ + int line_csum; /* checksum at the end of the packet */ uint8_t last_packet[MAX_PACKET_LENGTH + 4]; int last_packet_len; int signal; @@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) static void gdb_read_byte(GDBState *s, int ch) { - int i, csum; uint8_t reply; #ifndef CONFIG_USER_ONLY @@ -1542,35 +1544,122 @@ static void gdb_read_byte(GDBState *s, int ch) switch(s->state) { case RS_IDLE: if (ch == '$') { + /* start of command packet */ s->line_buf_index = 0; + s->line_sum = 0; s->state = RS_GETLINE; + } else { +#ifdef DEBUG_GDB + printf("gdbstub received garbage between packets: 0x%x\n", ch); +#endif } break; case RS_GETLINE: + if (ch == '}') { + /* start escape sequence */ + s->state = RS_GETLINE_ESC; + s->line_sum += ch; + } else if (ch == '*') { + /* start run length encoding sequence */ + s->state = RS_GETLINE_RLE; + s->line_sum += ch; + } else if (ch == '#') { + /* end of command, start of checksum*/ + s->state = RS_CHKSUM1; + } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif + s->state = RS_IDLE; + } else { + /* unescaped command character */ + s->line_buf[s->line_buf_index++] = ch; + s->line_sum += ch; + } + break; + case RS_GETLINE_ESC: if (ch == '#') { - s->state = RS_CHKSUM1; + /* unexpected end of command in escape sequence */ + s->state = RS_CHKSUM1; } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { + /* command buffer overrun */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif s->state = RS_IDLE; } else { - s->line_buf[s->line_buf_index++] = ch; + /* parse escaped character and leave escape state */ + s->line_buf[s->line_buf_index++] = ch ^ 0x20; + s->line_sum += ch; + s->state = RS_GETLINE; + } + break; + case RS_GETLINE_RLE: + if (ch < ' ') { + /* invalid RLE count encoding */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE count: 0x%x\n", ch); +#endif + s->state = RS_GETLINE; + } else { + /* decode repeat length */ + int repeat = (unsigned char)ch - ' ' + 3; + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { + /* that many repeats would overrun the command buffer */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun," + " dropping command\n"); +#endif + s->state = RS_IDLE; + } else if (s->line_buf_index <= 2) { + /* got a repeat but we have nothing to repeat */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE sequence\n"); +#endif + } else { + /* repeat the last character */ + memset(s->line_buf + s->line_buf_index, + s->line_buf[s->line_buf_index - 1], repeat); + s->line_buf_index += repeat; + s->line_sum += ch; + s->state = RS_GETLINE; + } } break; case RS_CHKSUM1: + /* get high hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_buf[s->line_buf_index] = '\0'; s->line_csum = fromhex(ch) << 4; s->state = RS_CHKSUM2; break; case RS_CHKSUM2: + /* get low hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_csum |= fromhex(ch); - csum = 0; - for(i = 0; i < s->line_buf_index; i++) { - csum += s->line_buf[i]; - } - if (s->line_csum != (csum & 0xff)) { + + if (s->line_csum != (s->line_sum & 0xff)) { + /* send NAK reply */ reply = '-'; put_buffer(s, &reply, 1); +#ifdef DEBUG_GDB + printf("gdbstub got command packet with incorrect checksum\n"); +#endif s->state = RS_IDLE; } else { + /* send ACK reply */ reply = '+'; put_buffer(s, &reply, 1); s->state = gdb_handle_packet(s, s->line_buf); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-01 17:16 [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive Doug Gale @ 2017-05-02 14:22 ` Stefan Hajnoczi 2017-05-02 14:32 ` Doug Gale 0 siblings, 1 reply; 9+ messages in thread From: Stefan Hajnoczi @ 2017-05-02 14:22 UTC (permalink / raw) To: Doug Gale; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 776 bytes --] On Mon, May 01, 2017 at 01:16:55PM -0400, Doug Gale wrote: > The attached patch implements the GDB Remote Serial Protocol for > command receive as per the documentation provided at > https://sourceware.org/gdb/onlinedocs/gdb/Remote-Protocol.html#Remote-Protocol > and from inspection of remote.c in the gdb source (the documentation > didn't clearly document whether the packet or the unescaped data were > used for the checksum, turns out the packet data is used for the > checksum, as expected). > > get_maintainer.pl didn't find a maintainer for gdbstub.c, so I didn't > cc any maintainers. Please see the guidelines for submitting patches: http://wiki.qemu.org/Contribute/SubmitAPatch Patches must be sent inline, not as attachments. Thanks, Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-02 14:22 ` Stefan Hajnoczi @ 2017-05-02 14:32 ` Doug Gale 2017-05-05 14:45 ` Stefan Hajnoczi 0 siblings, 1 reply; 9+ messages in thread From: Doug Gale @ 2017-05-02 14:32 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel Oops. Thanks, here's the patch inline. >From c238752f10434970af8ef620ce3bf6c0e18a20b5 Mon Sep 17 00:00:00 2001 From: Doug Gale <doug16k@gmail.com> Date: Mon, 1 May 2017 12:22:10 -0400 Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for command receive - decode escape sequences - decompress run-length encoding escape sequences - report command parsing problems to output when debug output is enabled - reject packet checksums that are not valid hex digits - compute the checksum based on the packet stream, not based on the decoded packet Tested with GDB and QtCreator integrated debugger on SMP QEMU instance. Works for me. Signed-off-by: Doug Gale <doug16k@gmail.com> --- gdbstub.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 98 insertions(+), 9 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index 9911153..d6f1b0e 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -286,6 +286,8 @@ enum RSState { RS_INACTIVE, RS_IDLE, RS_GETLINE, + RS_GETLINE_ESC, + RS_GETLINE_RLE, RS_CHKSUM1, RS_CHKSUM2, }; @@ -296,7 +298,8 @@ typedef struct GDBState { enum RSState state; /* parsing state */ char line_buf[MAX_PACKET_LENGTH]; int line_buf_index; - int line_csum; + int line_sum; /* running checksum */ + int line_csum; /* checksum at the end of the packet */ uint8_t last_packet[MAX_PACKET_LENGTH + 4]; int last_packet_len; int signal; @@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) static void gdb_read_byte(GDBState *s, int ch) { - int i, csum; uint8_t reply; #ifndef CONFIG_USER_ONLY @@ -1542,35 +1544,122 @@ static void gdb_read_byte(GDBState *s, int ch) switch(s->state) { case RS_IDLE: if (ch == '$') { + /* start of command packet */ s->line_buf_index = 0; + s->line_sum = 0; s->state = RS_GETLINE; + } else { +#ifdef DEBUG_GDB + printf("gdbstub received garbage between packets: 0x%x\n", ch); +#endif } break; case RS_GETLINE: + if (ch == '}') { + /* start escape sequence */ + s->state = RS_GETLINE_ESC; + s->line_sum += ch; + } else if (ch == '*') { + /* start run length encoding sequence */ + s->state = RS_GETLINE_RLE; + s->line_sum += ch; + } else if (ch == '#') { + /* end of command, start of checksum*/ + s->state = RS_CHKSUM1; + } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif + s->state = RS_IDLE; + } else { + /* unescaped command character */ + s->line_buf[s->line_buf_index++] = ch; + s->line_sum += ch; + } + break; + case RS_GETLINE_ESC: if (ch == '#') { - s->state = RS_CHKSUM1; + /* unexpected end of command in escape sequence */ + s->state = RS_CHKSUM1; } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { + /* command buffer overrun */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif s->state = RS_IDLE; } else { - s->line_buf[s->line_buf_index++] = ch; + /* parse escaped character and leave escape state */ + s->line_buf[s->line_buf_index++] = ch ^ 0x20; + s->line_sum += ch; + s->state = RS_GETLINE; + } + break; + case RS_GETLINE_RLE: + if (ch < ' ') { + /* invalid RLE count encoding */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE count: 0x%x\n", ch); +#endif + s->state = RS_GETLINE; + } else { + /* decode repeat length */ + int repeat = (unsigned char)ch - ' ' + 3; + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { + /* that many repeats would overrun the command buffer */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun," + " dropping command\n"); +#endif + s->state = RS_IDLE; + } else if (s->line_buf_index <= 2) { + /* got a repeat but we have nothing to repeat */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE sequence\n"); +#endif + } else { + /* repeat the last character */ + memset(s->line_buf + s->line_buf_index, + s->line_buf[s->line_buf_index - 1], repeat); + s->line_buf_index += repeat; + s->line_sum += ch; + s->state = RS_GETLINE; + } } break; case RS_CHKSUM1: + /* get high hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_buf[s->line_buf_index] = '\0'; s->line_csum = fromhex(ch) << 4; s->state = RS_CHKSUM2; break; case RS_CHKSUM2: + /* get low hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_csum |= fromhex(ch); - csum = 0; - for(i = 0; i < s->line_buf_index; i++) { - csum += s->line_buf[i]; - } - if (s->line_csum != (csum & 0xff)) { + + if (s->line_csum != (s->line_sum & 0xff)) { + /* send NAK reply */ reply = '-'; put_buffer(s, &reply, 1); +#ifdef DEBUG_GDB + printf("gdbstub got command packet with incorrect checksum\n"); +#endif s->state = RS_IDLE; } else { + /* send ACK reply */ reply = '+'; put_buffer(s, &reply, 1); s->state = gdb_handle_packet(s, s->line_buf); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-02 14:32 ` Doug Gale @ 2017-05-05 14:45 ` Stefan Hajnoczi 2017-05-07 15:27 ` Doug Gale [not found] ` <CAEfK_44TpSJVi7JULXMRLftM5BFUJs=vHSxKG3h=YOUjQz1t+g@mail.gmail.com> 0 siblings, 2 replies; 9+ messages in thread From: Stefan Hajnoczi @ 2017-05-05 14:45 UTC (permalink / raw) To: Doug Gale; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 954 bytes --] On Tue, May 02, 2017 at 10:32:40AM -0400, Doug Gale wrote: > + } else { > + /* decode repeat length */ > + int repeat = (unsigned char)ch - ' ' + 3; > + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { > + /* that many repeats would overrun the command buffer */ > +#ifdef DEBUG_GDB > + printf("gdbstub command buffer overrun," > + " dropping command\n"); > +#endif > + s->state = RS_IDLE; > + } else if (s->line_buf_index <= 2) { Why s->line_buf_index <= 2? I expected s->line_buf_index < 1 since we just need 1 character to clone for run-length decoding. > + /* got a repeat but we have nothing to repeat */ > +#ifdef DEBUG_GDB > + printf("gdbstub got invalid RLE sequence\n"); > +#endif > + } else { Missing s->state = RS_IDLE? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-05 14:45 ` Stefan Hajnoczi @ 2017-05-07 15:27 ` Doug Gale 2017-05-07 18:59 ` Doug Gale [not found] ` <CAEfK_44TpSJVi7JULXMRLftM5BFUJs=vHSxKG3h=YOUjQz1t+g@mail.gmail.com> 1 sibling, 1 reply; 9+ messages in thread From: Doug Gale @ 2017-05-07 15:27 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel [Oops, forgot to reply all, resending...] Yes, on second thought, <= 2 is off by one. [0] would be the '$', [1] would be the repeated character, and [2] would be the '*'. And yes, there is a missing s->state = RS_IDLE there. Good catch. I'll post updated patch shortly... On Fri, May 5, 2017 at 10:45 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, May 02, 2017 at 10:32:40AM -0400, Doug Gale wrote: >> + } else { >> + /* decode repeat length */ >> + int repeat = (unsigned char)ch - ' ' + 3; >> + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { >> + /* that many repeats would overrun the command buffer */ >> +#ifdef DEBUG_GDB >> + printf("gdbstub command buffer overrun," >> + " dropping command\n"); >> +#endif >> + s->state = RS_IDLE; >> + } else if (s->line_buf_index <= 2) { > > Why s->line_buf_index <= 2? I expected s->line_buf_index < 1 since we > just need 1 character to clone for run-length decoding. > >> + /* got a repeat but we have nothing to repeat */ >> +#ifdef DEBUG_GDB >> + printf("gdbstub got invalid RLE sequence\n"); >> +#endif >> + } else { > > Missing s->state = RS_IDLE? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-07 15:27 ` Doug Gale @ 2017-05-07 18:59 ` Doug Gale 0 siblings, 0 replies; 9+ messages in thread From: Doug Gale @ 2017-05-07 18:59 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel Updated patch with comments addressed: >From 6bce4e5c87c255f10b22d2bf6fc951dde2bbf457 Mon Sep 17 00:00:00 2001 From: Doug Gale <doug16k@gmail.com> Date: Mon, 1 May 2017 12:22:10 -0400 Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for command receive - decode escape sequences - decompress run-length encoding escape sequences - report command parsing problems to output when debug output is enabled - reject packet checksums that are not valid hex digits - compute the checksum based on the packet stream, not based on the decoded packet Tested with GDB and QtCreator integrated debugger on SMP QEMU instance. Works for me. Signed-off-by: Doug Gale <doug16k@gmail.com> --- gdbstub.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 99 insertions(+), 9 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index 9911153..3abaf7c 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -286,6 +286,8 @@ enum RSState { RS_INACTIVE, RS_IDLE, RS_GETLINE, + RS_GETLINE_ESC, + RS_GETLINE_RLE, RS_CHKSUM1, RS_CHKSUM2, }; @@ -296,7 +298,8 @@ typedef struct GDBState { enum RSState state; /* parsing state */ char line_buf[MAX_PACKET_LENGTH]; int line_buf_index; - int line_csum; + int line_sum; /* running checksum */ + int line_csum; /* checksum at the end of the packet */ uint8_t last_packet[MAX_PACKET_LENGTH + 4]; int last_packet_len; int signal; @@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) static void gdb_read_byte(GDBState *s, int ch) { - int i, csum; uint8_t reply; #ifndef CONFIG_USER_ONLY @@ -1542,35 +1544,123 @@ static void gdb_read_byte(GDBState *s, int ch) switch(s->state) { case RS_IDLE: if (ch == '$') { + /* start of command packet */ s->line_buf_index = 0; + s->line_sum = 0; s->state = RS_GETLINE; + } else { +#ifdef DEBUG_GDB + printf("gdbstub received garbage between packets: 0x%x\n", ch); +#endif } break; case RS_GETLINE: + if (ch == '}') { + /* start escape sequence */ + s->state = RS_GETLINE_ESC; + s->line_sum += ch; + } else if (ch == '*') { + /* start run length encoding sequence */ + s->state = RS_GETLINE_RLE; + s->line_sum += ch; + } else if (ch == '#') { + /* end of command, start of checksum*/ + s->state = RS_CHKSUM1; + } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif + s->state = RS_IDLE; + } else { + /* unescaped command character */ + s->line_buf[s->line_buf_index++] = ch; + s->line_sum += ch; + } + break; + case RS_GETLINE_ESC: if (ch == '#') { - s->state = RS_CHKSUM1; + /* unexpected end of command in escape sequence */ + s->state = RS_CHKSUM1; } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { + /* command buffer overrun */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif s->state = RS_IDLE; } else { - s->line_buf[s->line_buf_index++] = ch; + /* parse escaped character and leave escape state */ + s->line_buf[s->line_buf_index++] = ch ^ 0x20; + s->line_sum += ch; + s->state = RS_GETLINE; + } + break; + case RS_GETLINE_RLE: + if (ch < ' ') { + /* invalid RLE count encoding */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE count: 0x%x\n", ch); +#endif + s->state = RS_GETLINE; + } else { + /* decode repeat length */ + int repeat = (unsigned char)ch - ' ' + 3; + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { + /* that many repeats would overrun the command buffer */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun," + " dropping command\n"); +#endif + s->state = RS_IDLE; + } else if (s->line_buf_index < 2) { + /* got a repeat but we have nothing to repeat */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE sequence\n"); +#endif + s->state = RS_GETLINE; + } else { + /* repeat the last character */ + memset(s->line_buf + s->line_buf_index, + s->line_buf[s->line_buf_index - 1], repeat); + s->line_buf_index += repeat; + s->line_sum += ch; + s->state = RS_GETLINE; + } } break; case RS_CHKSUM1: + /* get high hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_buf[s->line_buf_index] = '\0'; s->line_csum = fromhex(ch) << 4; s->state = RS_CHKSUM2; break; case RS_CHKSUM2: + /* get low hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_csum |= fromhex(ch); - csum = 0; - for(i = 0; i < s->line_buf_index; i++) { - csum += s->line_buf[i]; - } - if (s->line_csum != (csum & 0xff)) { + + if (s->line_csum != (s->line_sum & 0xff)) { + /* send NAK reply */ reply = '-'; put_buffer(s, &reply, 1); +#ifdef DEBUG_GDB + printf("gdbstub got command packet with incorrect checksum\n"); +#endif s->state = RS_IDLE; } else { + /* send ACK reply */ reply = '+'; put_buffer(s, &reply, 1); s->state = gdb_handle_packet(s, s->line_buf); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <CAEfK_44TpSJVi7JULXMRLftM5BFUJs=vHSxKG3h=YOUjQz1t+g@mail.gmail.com>]
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive [not found] ` <CAEfK_44TpSJVi7JULXMRLftM5BFUJs=vHSxKG3h=YOUjQz1t+g@mail.gmail.com> @ 2017-05-08 9:01 ` Stefan Hajnoczi 2017-05-08 13:16 ` Doug Gale 0 siblings, 1 reply; 9+ messages in thread From: Stefan Hajnoczi @ 2017-05-08 9:01 UTC (permalink / raw) To: Doug Gale; +Cc: qemu-devel On Sun, May 7, 2017 at 4:26 PM, Doug Gale <doug16k@gmail.com> wrote: > On Fri, May 5, 2017 at 10:45 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Tue, May 02, 2017 at 10:32:40AM -0400, Doug Gale wrote: >>> + } else { >>> + /* decode repeat length */ >>> + int repeat = (unsigned char)ch - ' ' + 3; >>> + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { >>> + /* that many repeats would overrun the command buffer */ >>> +#ifdef DEBUG_GDB >>> + printf("gdbstub command buffer overrun," >>> + " dropping command\n"); >>> +#endif >>> + s->state = RS_IDLE; >>> + } else if (s->line_buf_index <= 2) { >> >> Why s->line_buf_index <= 2? I expected s->line_buf_index < 1 since we >> just need 1 character to clone for run-length decoding. > > Yes, on second thought, <= 2 is off by one. [0] would be the '$', [1] > would be the repeated character, and [2] would be the '*'. '$' and '*' are not placed into line_buf[] and do not increment line_buf_index. They don't count. I think the correct condition is line_buf_index < 1 so that the following input from the GDB documentation parses: "$0* " -> "0000". https://sourceware.org/gdb/onlinedocs/gdb/Overview.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-08 9:01 ` Stefan Hajnoczi @ 2017-05-08 13:16 ` Doug Gale 2017-05-08 13:32 ` Stefan Hajnoczi 0 siblings, 1 reply; 9+ messages in thread From: Doug Gale @ 2017-05-08 13:16 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: qemu-devel Right, only GETLINE_* states write to the linebuf, so line_buf_index < 1 is correct. Updated patch: >From 2e6c45260cae60bbae446bffe43f948ab002c529 Mon Sep 17 00:00:00 2001 From: Doug Gale <doug16k@gmail.com> Date: Mon, 1 May 2017 12:22:10 -0400 Subject: [PATCH] gdbstub: implement remote debugging protocol escapes for command receive - decode escape sequences - decompress run-length encoding escape sequences - report command parsing problems to output when debug output is enabled - reject packet checksums that are not valid hex digits - compute the checksum based on the packet stream, not based on the decoded packet Tested with GDB and QtCreator integrated debugger on SMP QEMU instance. Works for me. Signed-off-by: Doug Gale <doug16k@gmail.com> --- gdbstub.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 99 insertions(+), 9 deletions(-) diff --git a/gdbstub.c b/gdbstub.c index 9911153..dee0ff3 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -286,6 +286,8 @@ enum RSState { RS_INACTIVE, RS_IDLE, RS_GETLINE, + RS_GETLINE_ESC, + RS_GETLINE_RLE, RS_CHKSUM1, RS_CHKSUM2, }; @@ -296,7 +298,8 @@ typedef struct GDBState { enum RSState state; /* parsing state */ char line_buf[MAX_PACKET_LENGTH]; int line_buf_index; - int line_csum; + int line_sum; /* running checksum */ + int line_csum; /* checksum at the end of the packet */ uint8_t last_packet[MAX_PACKET_LENGTH + 4]; int last_packet_len; int signal; @@ -1508,7 +1511,6 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...) static void gdb_read_byte(GDBState *s, int ch) { - int i, csum; uint8_t reply; #ifndef CONFIG_USER_ONLY @@ -1542,35 +1544,123 @@ static void gdb_read_byte(GDBState *s, int ch) switch(s->state) { case RS_IDLE: if (ch == '$') { + /* start of command packet */ s->line_buf_index = 0; + s->line_sum = 0; s->state = RS_GETLINE; + } else { +#ifdef DEBUG_GDB + printf("gdbstub received garbage between packets: 0x%x\n", ch); +#endif } break; case RS_GETLINE: + if (ch == '}') { + /* start escape sequence */ + s->state = RS_GETLINE_ESC; + s->line_sum += ch; + } else if (ch == '*') { + /* start run length encoding sequence */ + s->state = RS_GETLINE_RLE; + s->line_sum += ch; + } else if (ch == '#') { + /* end of command, start of checksum*/ + s->state = RS_CHKSUM1; + } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif + s->state = RS_IDLE; + } else { + /* unescaped command character */ + s->line_buf[s->line_buf_index++] = ch; + s->line_sum += ch; + } + break; + case RS_GETLINE_ESC: if (ch == '#') { - s->state = RS_CHKSUM1; + /* unexpected end of command in escape sequence */ + s->state = RS_CHKSUM1; } else if (s->line_buf_index >= sizeof(s->line_buf) - 1) { + /* command buffer overrun */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun, dropping command\n"); +#endif s->state = RS_IDLE; } else { - s->line_buf[s->line_buf_index++] = ch; + /* parse escaped character and leave escape state */ + s->line_buf[s->line_buf_index++] = ch ^ 0x20; + s->line_sum += ch; + s->state = RS_GETLINE; + } + break; + case RS_GETLINE_RLE: + if (ch < ' ') { + /* invalid RLE count encoding */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE count: 0x%x\n", ch); +#endif + s->state = RS_GETLINE; + } else { + /* decode repeat length */ + int repeat = (unsigned char)ch - ' ' + 3; + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { + /* that many repeats would overrun the command buffer */ +#ifdef DEBUG_GDB + printf("gdbstub command buffer overrun," + " dropping command\n"); +#endif + s->state = RS_IDLE; + } else if (s->line_buf_index < 1) { + /* got a repeat but we have nothing to repeat */ +#ifdef DEBUG_GDB + printf("gdbstub got invalid RLE sequence\n"); +#endif + s->state = RS_GETLINE; + } else { + /* repeat the last character */ + memset(s->line_buf + s->line_buf_index, + s->line_buf[s->line_buf_index - 1], repeat); + s->line_buf_index += repeat; + s->line_sum += ch; + s->state = RS_GETLINE; + } } break; case RS_CHKSUM1: + /* get high hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_buf[s->line_buf_index] = '\0'; s->line_csum = fromhex(ch) << 4; s->state = RS_CHKSUM2; break; case RS_CHKSUM2: + /* get low hex digit of checksum */ + if (!isxdigit(ch)) { +#ifdef DEBUG_GDB + printf("gdbstub got invalid command checksum digit\n"); +#endif + s->state = RS_GETLINE; + break; + } s->line_csum |= fromhex(ch); - csum = 0; - for(i = 0; i < s->line_buf_index; i++) { - csum += s->line_buf[i]; - } - if (s->line_csum != (csum & 0xff)) { + + if (s->line_csum != (s->line_sum & 0xff)) { + /* send NAK reply */ reply = '-'; put_buffer(s, &reply, 1); +#ifdef DEBUG_GDB + printf("gdbstub got command packet with incorrect checksum\n"); +#endif s->state = RS_IDLE; } else { + /* send ACK reply */ reply = '+'; put_buffer(s, &reply, 1); s->state = gdb_handle_packet(s, s->line_buf); -- 2.7.4 On Mon, May 8, 2017 at 5:01 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Sun, May 7, 2017 at 4:26 PM, Doug Gale <doug16k@gmail.com> wrote: >> On Fri, May 5, 2017 at 10:45 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >>> On Tue, May 02, 2017 at 10:32:40AM -0400, Doug Gale wrote: >>>> + } else { >>>> + /* decode repeat length */ >>>> + int repeat = (unsigned char)ch - ' ' + 3; >>>> + if (s->line_buf_index + repeat >= sizeof(s->line_buf) - 1) { >>>> + /* that many repeats would overrun the command buffer */ >>>> +#ifdef DEBUG_GDB >>>> + printf("gdbstub command buffer overrun," >>>> + " dropping command\n"); >>>> +#endif >>>> + s->state = RS_IDLE; >>>> + } else if (s->line_buf_index <= 2) { >>> >>> Why s->line_buf_index <= 2? I expected s->line_buf_index < 1 since we >>> just need 1 character to clone for run-length decoding. >> >> Yes, on second thought, <= 2 is off by one. [0] would be the '$', [1] >> would be the repeated character, and [2] would be the '*'. > > '$' and '*' are not placed into line_buf[] and do not increment > line_buf_index. They don't count. > > I think the correct condition is line_buf_index < 1 so that the > following input from the GDB documentation parses: "$0* " -> "0000". > > https://sourceware.org/gdb/onlinedocs/gdb/Overview.html ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive 2017-05-08 13:16 ` Doug Gale @ 2017-05-08 13:32 ` Stefan Hajnoczi 0 siblings, 0 replies; 9+ messages in thread From: Stefan Hajnoczi @ 2017-05-08 13:32 UTC (permalink / raw) To: Doug Gale; +Cc: qemu-devel Thanks, applied to my staging tree: https://github.com/stefanha/qemu/commits/staging I applied this patch manually because it was not formatted according to the patch submission guidelines. Please take note of them for future contributions: 1. "Send each new revision as a new top-level thread, rather than burying it in-reply-to an earlier revision, as many reviewers are not looking inside deep threads for new patches." 2. Revision changelog or comments that should not go into git history must be below '---'. This way git-am(1) can apply your mail in a single command. Thanks, Stefan ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-08 13:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-01 17:16 [Qemu-devel] [PATCH] gdbstub: implement remote debugging protocol escapes for command receive Doug Gale
2017-05-02 14:22 ` Stefan Hajnoczi
2017-05-02 14:32 ` Doug Gale
2017-05-05 14:45 ` Stefan Hajnoczi
2017-05-07 15:27 ` Doug Gale
2017-05-07 18:59 ` Doug Gale
[not found] ` <CAEfK_44TpSJVi7JULXMRLftM5BFUJs=vHSxKG3h=YOUjQz1t+g@mail.gmail.com>
2017-05-08 9:01 ` Stefan Hajnoczi
2017-05-08 13:16 ` Doug Gale
2017-05-08 13:32 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).