[PATCH v3 0/9] winedbg(gdbproxy): don't misbehave on module names w/ special characters; modernise
Changelog:
v1 -> v2: address review comments so far v2 -> v3: - Change cover letter subject - Capitalise subject - Split qXfer cache patch into two
Jinoh Kang (9): winedbg: Crash when allocation fails in packet_realloc(). winedbg: Free buffer and return NULL if size==0 in packet_realloc(). winedbg: Generalise packet_realloc(), rename to buffer_realloc(). winedbg: Use unsigned int for offset/length in GDB qXfer handler. winedbg: Escape special characters in GDB packet reply. winedbg: Buffer and escape output of GDB qXfer commands. winedbg: Define table for GDB qXfer command handlers. winedbg: Cache GDB qXfer command result for chunked fetching. winedbg: Implement GDB qXfer object exec-file.
programs/winedbg/gdbproxy.c | 565 ++++++++++++++++++++++++++++-------- 1 file changed, 452 insertions(+), 113 deletions(-)
Today, every caller of packet_realloc simply assumes that the allocation always succeeds. Not crashing outright on allocation failure makes diagnosing problems more difficult.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 522e4fdb506..deb6b0973f5 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -708,9 +708,8 @@ enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, static char* packet_realloc(char* buf, int size) { if (!buf) - return HeapAlloc(GetProcessHeap(), 0, size); - return HeapReAlloc(GetProcessHeap(), 0, buf, size); - + return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, size); + return HeapReAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, buf, size); }
static void packet_reply_grow(struct gdb_context* gdbctx, size_t size)
packet_realloc() does not return on error, so we can use NULL for normal circumstances.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index deb6b0973f5..3f5584633e4 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -707,6 +707,12 @@ enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02,
static char* packet_realloc(char* buf, int size) { + if (!size) + { + if (buf) + HeapFree(GetProcessHeap(), 0, buf); + return NULL; + } if (!buf) return HeapAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, size); return HeapReAlloc(GetProcessHeap(), HEAP_GENERATE_EXCEPTIONS, buf, size);
Make it return void * and accept size_t as the size.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 3f5584633e4..db0b8d77ba9 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -705,7 +705,7 @@ static int addr_width(struct gdb_context* gdbctx) enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, packet_last_f = 0x80};
-static char* packet_realloc(char* buf, int size) +static void* buffer_realloc(void* buf, size_t size) { if (!size) { @@ -723,7 +723,7 @@ static void packet_reply_grow(struct gdb_context* gdbctx, size_t size) if (gdbctx->out_buf_alloc < gdbctx->out_len + size) { gdbctx->out_buf_alloc = ((gdbctx->out_len + size) / 32 + 1) * 32; - gdbctx->out_buf = packet_realloc(gdbctx->out_buf, gdbctx->out_buf_alloc); + gdbctx->out_buf = buffer_realloc(gdbctx->out_buf, gdbctx->out_buf_alloc); } }
@@ -2090,7 +2090,7 @@ static int fetch_data(struct gdb_context* gdbctx) { #define STEP 128 if (gdbctx->in_len + STEP > gdbctx->in_buf_alloc) - gdbctx->in_buf = packet_realloc(gdbctx->in_buf, gdbctx->in_buf_alloc += STEP); + gdbctx->in_buf = buffer_realloc(gdbctx->in_buf, gdbctx->in_buf_alloc += STEP); #undef STEP len = recv(gdbctx->sock, gdbctx->in_buf + gdbctx->in_len, gdbctx->in_buf_alloc - gdbctx->in_len - 1, 0); if (len <= 0) break;
On 11/16/21 17:51, Jinoh Kang wrote:
Make it return void * and accept size_t as the size.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Hi Jinoh,
I think that now winegdb has been converted to PE format, you can instead convert all Heap* allocations to standard C allocations, and use realloc directly instead of the helper.
Regarding the errors, maybe add some assert calls if you like, although I don't think we usually use that or HEAP_GENERATE_EXCEPTIONS flag, and instead we just consider that if allocation returned NULL it should either be handled properly, or ignored and later access will cause exceptions.
On 11/17/21 01:58, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Make it return void * and accept size_t as the size.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Hi Jinoh,
I think that now winegdb has been converted to PE format, you can instead convert all Heap* allocations to standard C allocations, and use realloc directly instead of the helper.
Sounds good.
Regarding the errors, maybe add some assert calls if you like, although I don't think we usually use that or HEAP_GENERATE_EXCEPTIONS flag,
You're right.
and instead we just consider that if allocation returned NULL it should either be handled properly, or ignored and later access will cause exceptions.
For some reason, no uses of that function checks for the error condition and just assumes that allocation always succeeds.
So there seems to be three options:
1. Don't check for allocation failure at all. This will AV the next time the packet buffer (gdbctx->{in,out}_buf) is accessed. 2. Gracefully handle all allocation failures; however, this would be out of scope of this patch serie. 3. Add a new helper called xrealloc() that terminates the process if allocation fails.
So I believe we should do (1) first, and then maybe later go (2)?
On 11/16/21 18:20, Jinoh Kang wrote:
On 11/17/21 01:58, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Make it return void * and accept size_t as the size.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
Hi Jinoh,
I think that now winegdb has been converted to PE format, you can instead convert all Heap* allocations to standard C allocations, and use realloc directly instead of the helper.
Sounds good.
Regarding the errors, maybe add some assert calls if you like, although I don't think we usually use that or HEAP_GENERATE_EXCEPTIONS flag,
You're right.
and instead we just consider that if allocation returned NULL it should either be handled properly, or ignored and later access will cause exceptions.
For some reason, no uses of that function checks for the error condition and just assumes that allocation always succeeds.
Well this is often the case when it's quite unexpected to fail and there's no proper error handling. An abort or assertion failure will not be much different from a segmentation fault in the end.
So there seems to be three options:
- Don't check for allocation failure at all. This will AV the next time the packet buffer (gdbctx->{in,out}_buf) is accessed.
- Gracefully handle all allocation failures; however, this would be out of scope of this patch serie.
- Add a new helper called xrealloc() that terminates the process if allocation fails.
So I believe we should do (1) first, and then maybe later go (2)?
If it's actually useful to handle the errors, and if there's a way to properly tell gdb about it, without ending up in a unstable state, then yes I think it would be better.
Until then just ignore the failure, yeah.
Cheers,
packet_query uses sscanf format "%x" to parse out offset and length values. Since %x corresponds to unsigned int in the C standard, adjust the variable types appropriately.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index db0b8d77ba9..e6b1d998f7d 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -785,7 +785,7 @@ static void packet_reply_open_xfer(struct gdb_context* gdbctx) packet_reply_add(gdbctx, "m"); }
-static void packet_reply_close_xfer(struct gdb_context* gdbctx, int off, int len) +static void packet_reply_close_xfer(struct gdb_context* gdbctx, unsigned int off, unsigned int len) { int begin = gdbctx->out_curr_packet + 1; int plen; @@ -802,7 +802,7 @@ static void packet_reply_close_xfer(struct gdb_context* gdbctx, int off, int len }
plen = gdbctx->out_len - begin; - if (len >= 0 && plen > len) gdbctx->out_len -= (plen - len); + if (plen > len) gdbctx->out_len -= (plen - len); else gdbctx->out_buf[gdbctx->out_curr_packet] = 'l';
packet_reply_close(gdbctx); @@ -1764,7 +1764,7 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
static enum packet_return packet_query(struct gdb_context* gdbctx) { - int off, len; + unsigned int off, len; struct backend_cpu *cpu;
switch (gdbctx->in_packet[0])
There are four special characters in GDB's remote serial protocol:
- '$' (0x24): start of packet - '}' (0x7D): escape - '*' (0x2A): run-length encoding repeat count delimiter - '#' (0x23): end of packet; start of checksum
In particular, the '#' and '}' characters are problematic since they are often used in library filenames. A few examples:
- %SystemRoot%\assembly\NativeImages_v[.NET ver][module+hash]#**.dll - {CLSID or UUID}*.dll
To make GDB happy with those filenames, we scan for those characters and escape them properly.
While we are at it, also remove the assert in the packet_reply function that checks for '$' and '#' in the packet payload.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 68 ++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e6b1d998f7d..bdb73659ade 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -752,18 +752,71 @@ static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len) } }
+static const unsigned char gdb_special_chars_lookup_table[4] = { + /* The characters should be sorted by its value modulo table length. */ + + 0x24, /* $: 001001|00 */ + 0x7D, /* }: 011111|01 */ + 0x2A, /* *: 001010|10 */ + 0x23 /* #: 001000|11 */ +}; + +static inline BOOL is_gdb_special_char(unsigned char val) +{ + /* A note on the GDB special character scanning code: + * + * We cannot use strcspn() since we plan to transmit binary data in + * packet reply, which can contain NULL (0x00) bytes. We also don't want + * to slow down memory dump transfers. Therefore, we use a tiny lookup + * table that contains all the four special characters to speed up scanning. + * + * Note: strcspn() uses lookup tables as well, but as of Wine 6.19 + * msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table + * on the stack and populates it on the fly. It would be slower and less + * cache-friendly than a preallocated, tiny static lookup table. + */ + + const size_t length = ARRAY_SIZE(gdb_special_chars_lookup_table); + return gdb_special_chars_lookup_table[val % length] == val; +} + +static void packet_reply_add_raw(struct gdb_context* gdbctx, const void* data, size_t len) +{ + const unsigned char *ptr = data, *sptr, *end = ptr + len; + size_t seg_len; + int is_end; + + while (ptr != end) + { + for (sptr = ptr; sptr != end && !is_gdb_special_char(*sptr); sptr++) + ; + + seg_len = sptr - ptr; + is_end = sptr == end; + + packet_reply_grow(gdbctx, is_end ? seg_len : seg_len + 2); + memcpy(&gdbctx->out_buf[gdbctx->out_len], ptr, seg_len); + gdbctx->out_len += seg_len; + ptr += seg_len; + + if (is_end) + break; + + gdbctx->out_buf[gdbctx->out_len++] = 0x7D; + gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; + } +} + static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str) { - int len = strlen(str); - packet_reply_grow(gdbctx, len); - memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len); - gdbctx->out_len += len; + packet_reply_add_raw(gdbctx, str, strlen(str)); }
static void packet_reply_open(struct gdb_context* gdbctx) { assert(gdbctx->out_curr_packet == -1); - packet_reply_add(gdbctx, "$"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '$'; gdbctx->out_curr_packet = gdbctx->out_len; }
@@ -773,7 +826,8 @@ static void packet_reply_close(struct gdb_context* gdbctx) int plen;
plen = gdbctx->out_len - gdbctx->out_curr_packet; - packet_reply_add(gdbctx, "#"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '#'; cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen); packet_reply_hex_to(gdbctx, &cksum, 1); gdbctx->out_curr_packet = -1; @@ -812,8 +866,6 @@ static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* p { packet_reply_open(gdbctx);
- assert(strchr(packet, '$') == NULL && strchr(packet, '#') == NULL); - packet_reply_add(gdbctx, packet);
packet_reply_close(gdbctx);
On 11/16/21 17:51, Jinoh Kang wrote:
There are four special characters in GDB's remote serial protocol:
- '$' (0x24): start of packet
- '}' (0x7D): escape
- '*' (0x2A): run-length encoding repeat count delimiter
- '#' (0x23): end of packet; start of checksum
In particular, the '#' and '}' characters are problematic since they are often used in library filenames. A few examples:
- %SystemRoot%\assembly\NativeImages_v[.NET ver][module+hash]#**.dll
- {CLSID or UUID}*.dll
To make GDB happy with those filenames, we scan for those characters and escape them properly.
While we are at it, also remove the assert in the packet_reply function that checks for '$' and '#' in the packet payload.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 68 ++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e6b1d998f7d..bdb73659ade 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -752,18 +752,71 @@ static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len) } }
+static const unsigned char gdb_special_chars_lookup_table[4] = {
- /* The characters should be sorted by its value modulo table length. */
- 0x24, /* $: 001001|00 */
- 0x7D, /* }: 011111|01 */
- 0x2A, /* *: 001010|10 */
- 0x23 /* #: 001000|11 */
+};
+static inline BOOL is_gdb_special_char(unsigned char val) +{
- /* A note on the GDB special character scanning code:
** We cannot use strcspn() since we plan to transmit binary data in* packet reply, which can contain NULL (0x00) bytes. We also don't want* to slow down memory dump transfers. Therefore, we use a tiny lookup* table that contains all the four special characters to speed up scanning.** Note: strcspn() uses lookup tables as well, but as of Wine 6.19* msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table* on the stack and populates it on the fly. It would be slower and less* cache-friendly than a preallocated, tiny static lookup table.*/- const size_t length = ARRAY_SIZE(gdb_special_chars_lookup_table);
- return gdb_special_chars_lookup_table[val % length] == val;
+}
+static void packet_reply_add_raw(struct gdb_context* gdbctx, const void* data, size_t len) +{
- const unsigned char *ptr = data, *sptr, *end = ptr + len;
- size_t seg_len;
- int is_end;
I'm not sure what "seg_" is supposed to mean? How about just "len", and "tmp" instead of sptr?
- while (ptr != end)
- {
for (sptr = ptr; sptr != end && !is_gdb_special_char(*sptr); sptr++);seg_len = sptr - ptr;is_end = sptr == end;packet_reply_grow(gdbctx, is_end ? seg_len : seg_len + 2);memcpy(&gdbctx->out_buf[gdbctx->out_len], ptr, seg_len);gdbctx->out_len += seg_len;ptr += seg_len;if (is_end)break;gdbctx->out_buf[gdbctx->out_len++] = 0x7D;gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++;- }
+}
What about something like:
const unsigned char *begin = data, *end = begin + len, *ptr = begin;
while (ptr != end) { while (ptr != end && !is_gdb_special_char(*ptr)) ptr++;
packet_reply_grow(gdbctx, ptr - begin + 2); memcpy(&gdbctx->out_buf[gdbctx->out_len], begin, ptr - begin); gdbctx->out_len += ptr - begin; if (ptr == end) break;
gdbctx->out_buf[gdbctx->out_len++] = 0x7D; gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; begin = ptr; }
Could do with a len variable too if you feel like it's better.
static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str) {
- int len = strlen(str);
- packet_reply_grow(gdbctx, len);
- memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len);
- gdbctx->out_len += len;
packet_reply_add_raw(gdbctx, str, strlen(str)); }
static void packet_reply_open(struct gdb_context* gdbctx) { assert(gdbctx->out_curr_packet == -1);
- packet_reply_add(gdbctx, "$");
- packet_reply_grow(gdbctx, 1);
- gdbctx->out_buf[gdbctx->out_len++] = '$'; gdbctx->out_curr_packet = gdbctx->out_len; }
@@ -773,7 +826,8 @@ static void packet_reply_close(struct gdb_context* gdbctx) int plen;
plen = gdbctx->out_len - gdbctx->out_curr_packet;
- packet_reply_add(gdbctx, "#");
- packet_reply_grow(gdbctx, 1);
- gdbctx->out_buf[gdbctx->out_len++] = '#'; cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen); packet_reply_hex_to(gdbctx, &cksum, 1); gdbctx->out_curr_packet = -1;
@@ -812,8 +866,6 @@ static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* p { packet_reply_open(gdbctx);
assert(strchr(packet, '$') == NULL && strchr(packet, '#') == NULL);
packet_reply_add(gdbctx, packet); packet_reply_close(gdbctx);
After this change packet_reply_add helper seems a little useless, I suggest maybe:
* Keep packet_reply_add like it is, and use it everywhere some static strings are added, including in open/close here, where we know we don't need escaping.
* Introduce the new packet_reply_add_raw, and use it when appending binary or dynamic data.
On 11/17/21 03:30, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
There are four special characters in GDB's remote serial protocol:
- '$' (0x24): start of packet
- '}' (0x7D): escape
- '*' (0x2A): run-length encoding repeat count delimiter
- '#' (0x23): end of packet; start of checksum
In particular, the '#' and '}' characters are problematic since they are often used in library filenames. A few examples:
- %SystemRoot%\assembly\NativeImages_v[.NET ver][module+hash]#**.dll
- {CLSID or UUID}*.dll
To make GDB happy with those filenames, we scan for those characters and escape them properly.
While we are at it, also remove the assert in the packet_reply function that checks for '$' and '#' in the packet payload.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 68 ++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 8 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index e6b1d998f7d..bdb73659ade 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -752,18 +752,71 @@ static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len) } } +static const unsigned char gdb_special_chars_lookup_table[4] = { + /* The characters should be sorted by its value modulo table length. */
+ 0x24, /* $: 001001|00 */ + 0x7D, /* }: 011111|01 */ + 0x2A, /* *: 001010|10 */ + 0x23 /* #: 001000|11 */ +};
+static inline BOOL is_gdb_special_char(unsigned char val) +{ + /* A note on the GDB special character scanning code: + * + * We cannot use strcspn() since we plan to transmit binary data in + * packet reply, which can contain NULL (0x00) bytes. We also don't want + * to slow down memory dump transfers. Therefore, we use a tiny lookup + * table that contains all the four special characters to speed up scanning. + * + * Note: strcspn() uses lookup tables as well, but as of Wine 6.19 + * msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table + * on the stack and populates it on the fly. It would be slower and less + * cache-friendly than a preallocated, tiny static lookup table. + */
+ const size_t length = ARRAY_SIZE(gdb_special_chars_lookup_table); + return gdb_special_chars_lookup_table[val % length] == val; +}
+static void packet_reply_add_raw(struct gdb_context* gdbctx, const void* data, size_t len) +{ + const unsigned char *ptr = data, *sptr, *end = ptr + len; + size_t seg_len; + int is_end;
I'm not sure what "seg_" is supposed to mean? How about just "len", and "tmp" instead of sptr?
"seg" stands for segments (substrings), separated by special characters to be escaped. Ditto for "s" in "sptr".
That said, I agree "tmp" would work just as fine. Since "len" is already taken, maybe something like "tmplen" would do?
+ while (ptr != end) + { + for (sptr = ptr; sptr != end && !is_gdb_special_char(*sptr); sptr++) + ;
+ seg_len = sptr - ptr; + is_end = sptr == end;
+ packet_reply_grow(gdbctx, is_end ? seg_len : seg_len + 2); + memcpy(&gdbctx->out_buf[gdbctx->out_len], ptr, seg_len); + gdbctx->out_len += seg_len; + ptr += seg_len;
+ if (is_end) + break;
+ gdbctx->out_buf[gdbctx->out_len++] = 0x7D; + gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; + } +}
What about something like:
Thanks a lot for taking time to improve it. A few comments, though:
const unsigned char *begin = data, *end = begin + len, *ptr = begin;
I'm not sure the name "begin" is clear enough, due to the asymmetry of semantics of "begin" and "end".
- "begin" denotes the start of the subsequence we are currently working on. - "end" denotes the end of the entire input data.
Instead of "begin", how about "segment", "part", or "curr"? Alternatives include "fragment" and "curr_part", but they don't sound as attractive.
while (ptr != end) { while (ptr != end && !is_gdb_special_char(*ptr)) ptr++;
packet_reply_grow(gdbctx, ptr - begin + 2);
I see that the extra "is_end" variable doesn't help with readability.
If we're rewriting for clarity, how about calling "packet_reply_grow" twice instead of unconditionally growing the buffer for the extra two bytes of escape sequence? The reasons are:
1. Since it always allocates two more bytes at the end of the data, "packet_reply_add_raw" ends up eating extra 2B of space for each call (2*n).
2. It should be more often the case of "ptr == end" than "is_gdb_special_char()". Special characters are rare, and it's better to optimise for the more likely path.
memcpy(&gdbctx->out_buf[gdbctx->out_len], begin, ptr - begin); gdbctx->out_len += ptr - begin; if (ptr == end) break;
gdbctx->out_buf[gdbctx->out_len++] = 0x7D; gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; begin = ptr;
I suppose this assignment could be moved to the beginning of the loop body...
}
Could do with a len variable too if you feel like it's better.
...unless we're using tmp_len instead of "ptr - begin", of course.
static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str) { - int len = strlen(str); - packet_reply_grow(gdbctx, len); - memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len); - gdbctx->out_len += len; + packet_reply_add_raw(gdbctx, str, strlen(str)); } static void packet_reply_open(struct gdb_context* gdbctx) { assert(gdbctx->out_curr_packet == -1); - packet_reply_add(gdbctx, "$"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '$'; gdbctx->out_curr_packet = gdbctx->out_len; } @@ -773,7 +826,8 @@ static void packet_reply_close(struct gdb_context* gdbctx) int plen; plen = gdbctx->out_len - gdbctx->out_curr_packet; - packet_reply_add(gdbctx, "#"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '#'; cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen); packet_reply_hex_to(gdbctx, &cksum, 1); gdbctx->out_curr_packet = -1; @@ -812,8 +866,6 @@ static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* p { packet_reply_open(gdbctx); - assert(strchr(packet, '$') == NULL && strchr(packet, '#') == NULL);
packet_reply_add(gdbctx, packet); packet_reply_close(gdbctx);
After this change packet_reply_add helper seems a little useless, I suggest maybe:>
- Keep packet_reply_add like it is, and use it everywhere some static strings are added, including in open/close here, where we know we don't need escaping.
My intention was to repurpose packet_reply_add to be used *only* for emitting payload data in GDB packets, and not sending arbitrary data to the socket. Otherwise, someone else might end up using the wrong function and run into hard-to-debug protocol errors. By repurposing it, the code clearly draws the line between the (high-level) GDB protocol and the underlying raw serial transport.
Also, the overhead of scanning short string literals should not be significant.
- Introduce the new packet_reply_add_raw, and use it when appending binary or dynamic data.
On 11/17/21 08:36, Jinoh Kang wrote:
+static void packet_reply_add_raw(struct gdb_context* gdbctx, const void* data, size_t len) +{ + const unsigned char *ptr = data, *sptr, *end = ptr + len; + size_t seg_len; + int is_end;
I'm not sure what "seg_" is supposed to mean? How about just "len", and "tmp" instead of sptr?
"seg" stands for segments (substrings), separated by special characters to be escaped. Ditto for "s" in "sptr".
That said, I agree "tmp" would work just as fine. Since "len" is already taken, maybe something like "tmplen" would do?
+ while (ptr != end) + { + for (sptr = ptr; sptr != end && !is_gdb_special_char(*sptr); sptr++) + ;
+ seg_len = sptr - ptr; + is_end = sptr == end;
+ packet_reply_grow(gdbctx, is_end ? seg_len : seg_len + 2); + memcpy(&gdbctx->out_buf[gdbctx->out_len], ptr, seg_len); + gdbctx->out_len += seg_len; + ptr += seg_len;
+ if (is_end) + break;
+ gdbctx->out_buf[gdbctx->out_len++] = 0x7D; + gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; + } +}
What about something like:
Thanks a lot for taking time to improve it. A few comments, though:
const unsigned char *begin = data, *end = begin + len, *ptr = begin;
I'm not sure the name "begin" is clear enough, due to the asymmetry of semantics of "begin" and "end".
- "begin" denotes the start of the subsequence we are currently working on.
- "end" denotes the end of the entire input data.
Instead of "begin", how about "segment", "part", or "curr"? Alternatives include "fragment" and "curr_part", but they don't sound as attractive.
Sure, "tmp" or "curr" should be generic enough to not worry about the semantics anymore.
while (ptr != end) { while (ptr != end && !is_gdb_special_char(*ptr)) ptr++;
packet_reply_grow(gdbctx, ptr - begin + 2);
I see that the extra "is_end" variable doesn't help with readability.
If we're rewriting for clarity, how about calling "packet_reply_grow" twice instead of unconditionally growing the buffer for the extra two bytes of escape sequence? The reasons are:
Since it always allocates two more bytes at the end of the data, "packet_reply_add_raw" ends up eating extra 2B of space for each call (2*n).
It should be more often the case of "ptr == end" than "is_gdb_special_char()". Special characters are rare, and it's better to optimise for the more likely path.
Works for me too, although I don't think the extra bytes would change anything in general as the allocated areas are probably rounded anyway.
memcpy(&gdbctx->out_buf[gdbctx->out_len], begin, ptr - begin); gdbctx->out_len += ptr - begin; if (ptr == end) break;
gdbctx->out_buf[gdbctx->out_len++] = 0x7D; gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++; begin = ptr;
I suppose this assignment could be moved to the beginning of the loop body...
}
Could do with a len variable too if you feel like it's better.
...unless we're using tmp_len instead of "ptr - begin", of course.
static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str) { - int len = strlen(str); - packet_reply_grow(gdbctx, len); - memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len); - gdbctx->out_len += len; + packet_reply_add_raw(gdbctx, str, strlen(str)); } static void packet_reply_open(struct gdb_context* gdbctx) { assert(gdbctx->out_curr_packet == -1); - packet_reply_add(gdbctx, "$"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '$'; gdbctx->out_curr_packet = gdbctx->out_len; } @@ -773,7 +826,8 @@ static void packet_reply_close(struct gdb_context* gdbctx) int plen; plen = gdbctx->out_len - gdbctx->out_curr_packet; - packet_reply_add(gdbctx, "#"); + packet_reply_grow(gdbctx, 1); + gdbctx->out_buf[gdbctx->out_len++] = '#'; cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen); packet_reply_hex_to(gdbctx, &cksum, 1); gdbctx->out_curr_packet = -1; @@ -812,8 +866,6 @@ static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* p { packet_reply_open(gdbctx); - assert(strchr(packet, '$') == NULL && strchr(packet, '#') == NULL);
packet_reply_add(gdbctx, packet); packet_reply_close(gdbctx);
After this change packet_reply_add helper seems a little useless, I suggest maybe:>
- Keep packet_reply_add like it is, and use it everywhere some static strings are added, including in open/close here, where we know we don't need escaping.
My intention was to repurpose packet_reply_add to be used *only* for emitting payload data in GDB packets, and not sending arbitrary data to the socket. Otherwise, someone else might end up using the wrong function and run into hard-to-debug protocol errors. By repurposing it, the code clearly draws the line between the (high-level) GDB protocol and the underlying raw serial transport.
Also, the overhead of scanning short string literals should not be significant.
Yeah I wasn't really worried about that, more about the mostly empty helper that's only calling the _raw version.
Maybe we can have just a single "packet_reply_add" helper which always escape the data, and not call it for the '$' and '#' cases?
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; };
+struct vl_buffer +{ + void *base; + size_t len; + size_t alloc; +}; + struct gdb_context { /* gdb information */ @@ -82,6 +89,7 @@ struct gdb_context /* Unix environment */ ULONG_PTR wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ BOOL no_ack_mode; + struct vl_buffer qxfer_buffer; };
/* assume standard signal and errno values */ @@ -224,6 +232,102 @@ static void hex_to(char* dst, const void* src, size_t len) } }
+static void* buffer_realloc(void* buf, size_t size); + +static void vl_resize(struct vl_buffer* vlbuf, size_t alloc) +{ + vlbuf->alloc = alloc; + vlbuf->base = buffer_realloc(vlbuf->base, vlbuf->alloc); +} + +static void vl_empty(struct vl_buffer *vlbuf) +{ + vlbuf->len = 0; + vl_resize(vlbuf, 0); +} + +static void vl_grow(struct vl_buffer* vlbuf, size_t size) +{ + if (vlbuf->alloc < vlbuf->len + size) + vl_resize(vlbuf, ((vlbuf->len + size) / 32 + 1) * 32); +} + +static void vl_append(struct vl_buffer* vlbuf, const void *data, size_t size) +{ + vl_grow(vlbuf, size); + memcpy((void *)((unsigned char *)vlbuf->base + vlbuf->len), data, size); + vlbuf->len += size; +} + +static inline void vl_append_str(struct vl_buffer* vlbuf, const char* str) +{ + vl_append(vlbuf, (const void *)str, strlen(str)); +} + +static inline void vl_append_uinthex(struct vl_buffer* vlbuf, ULONG_PTR val, int len) +{ + char buf[sizeof(ULONG_PTR) * 2], *ptr; + + assert(len <= sizeof(ULONG_PTR)); + + for (ptr = buf + len * 2; ptr != buf; val >>= 4) + *--ptr = hex_to0(val & 0x0F); + + vl_append(vlbuf, ptr, len * 2); +} + +static const unsigned char xml_special_chars_lookup_table[16] = { + /* The characters should be sorted by its value modulo table length. */ + + 0x00, /* NUL */ + 0, + 0x22, /* ": 0010|0010 */ + 0, 0, 0, + 0x26, /* &: 0010|0110 */ + 0x27, /* ': 0010|0111 */ + 0, 0, 0, 0, + 0x3C, /* <: 0011|1100 */ + 0, + 0x3E, /* >: 0011|1110 */ + 0 +}; + +static inline BOOL is_nul_or_xml_special_char(unsigned char val) +{ + /* Note: strcspn() uses lookup tables as well, but as of Wine 6.19 + * msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table + * on the stack and populates it on the fly. It would be slower and less + * cache-friendly than a preallocated, tiny static lookup table. + */ + + const size_t length = ARRAY_SIZE(xml_special_chars_lookup_table); + return xml_special_chars_lookup_table[val % length] == val; +} + +static void vl_append_xmlstr(struct vl_buffer* vlbuf, const char *str) +{ + const char *ptr = str, *sptr; + + for (;;) + { + for (sptr = ptr; !is_nul_or_xml_special_char((unsigned char)*sptr); sptr++) + ; + + vl_append(vlbuf, ptr, sptr - ptr); + ptr = sptr; + + switch (*ptr++) + { + case '"': vl_append_str(vlbuf, """); break; + case '&': vl_append_str(vlbuf, "&"); break; + case ''': vl_append_str(vlbuf, "'"); break; + case '<': vl_append_str(vlbuf, "<"); break; + case '>': vl_append_str(vlbuf, ">"); break; + default: return; + } + } +} + static unsigned char checksum(const char* ptr, int len) { unsigned cksum = 0; @@ -833,34 +937,6 @@ static void packet_reply_close(struct gdb_context* gdbctx) gdbctx->out_curr_packet = -1; }
-static void packet_reply_open_xfer(struct gdb_context* gdbctx) -{ - packet_reply_open(gdbctx); - packet_reply_add(gdbctx, "m"); -} - -static void packet_reply_close_xfer(struct gdb_context* gdbctx, unsigned int off, unsigned int len) -{ - int begin = gdbctx->out_curr_packet + 1; - int plen; - - if (begin + off < gdbctx->out_len) - { - gdbctx->out_len -= off; - memmove(gdbctx->out_buf + begin, gdbctx->out_buf + begin + off, gdbctx->out_len); - } - else - { - gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; - gdbctx->out_len = gdbctx->out_curr_packet + 1; - } - - plen = gdbctx->out_len - begin; - if (plen > len) gdbctx->out_len -= (plen - len); - else gdbctx->out_buf[gdbctx->out_curr_packet] = 'l'; - - packet_reply_close(gdbctx); -}
static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* packet) { @@ -891,6 +967,38 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, dbg_ packet_reply_hex_to(gdbctx, cpu_register_ptr(gdbctx, ctx, idx), cpu_register_map[idx].length); }
+static void packet_reply_xfer( + struct gdb_context* gdbctx, + const void *data, + size_t datalen, + unsigned int off, + unsigned int len, + BOOL *more_p +) +{ + BOOL nonempty, more; + size_t trunc_len; + + packet_reply_open(gdbctx); + + nonempty = (size_t)off < datalen; + more = nonempty && (size_t)off + len < datalen; + if (more) + packet_reply_add(gdbctx, "m"); + else + packet_reply_add(gdbctx, "l"); + + if (nonempty) + { + trunc_len = min((size_t)len, datalen - off); + packet_reply_add_raw(gdbctx, (const unsigned char *)data + off, trunc_len); + } + + packet_reply_close(gdbctx); + + *more_p = more; +} + /* =============================================== * * P A C K E T H A N D L E R S * * =============================================== * @@ -1587,6 +1695,7 @@ static enum packet_return packet_query_remote_command(struct gdb_context* gdbctx static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVOID ctx) { struct gdb_context* gdbctx = ctx; + struct vl_buffer* vlbuf = &gdbctx->qxfer_buffer; MEMORY_BASIC_INFORMATION mbi; IMAGE_SECTION_HEADER *sec; IMAGE_DOS_HEADER *dos = NULL; @@ -1599,11 +1708,11 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO mod.SizeOfStruct = sizeof(mod); SymGetModuleInfo64(gdbctx->process->handle, base, &mod);
- packet_reply_add(gdbctx, "<library name=""); + vl_append_str(vlbuf, "<library name=""); if (strcmp(mod.LoadedImageName, "[vdso].so") == 0) - packet_reply_add(gdbctx, "linux-vdso.so.1"); + vl_append_xmlstr(vlbuf, "linux-vdso.so.1"); else if (mod.LoadedImageName[0] == '/') - packet_reply_add(gdbctx, mod.LoadedImageName); + vl_append_xmlstr(vlbuf, mod.LoadedImageName); else { UNICODE_STRING nt_name; @@ -1618,15 +1727,15 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO if (IsWow64Process(gdbctx->process->handle, &is_wow64) && is_wow64 && (tmp = strstr(unix_path, "system32"))) memcpy(tmp, "syswow64", 8); - packet_reply_add(gdbctx, unix_path); + vl_append_xmlstr(vlbuf, unix_path); } else - packet_reply_add(gdbctx, mod.LoadedImageName); + vl_append_xmlstr(vlbuf, mod.LoadedImageName);
HeapFree(GetProcessHeap(), 0, unix_path); RtlFreeUnicodeString(&nt_name); } - packet_reply_add(gdbctx, "">"); + vl_append_str(vlbuf, "">");
size = sizeof(buffer); if (VirtualQueryEx(gdbctx->process->handle, (void *)(UINT_PTR)mod.BaseOfImage, &mbi, sizeof(mbi)) >= sizeof(mbi) && @@ -1657,72 +1766,75 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO for (i = 0; i < max(nth->FileHeader.NumberOfSections, 1); ++i) { if ((char *)(sec + i) >= buffer + size) break; - packet_reply_add(gdbctx, "<segment address="0x"); - packet_reply_val(gdbctx, mod.BaseOfImage + sec[i].VirtualAddress, sizeof(ULONG_PTR)); - packet_reply_add(gdbctx, ""/>"); + vl_append_str(vlbuf, "<segment address="0x"); + vl_append_uinthex(vlbuf, mod.BaseOfImage + sec[i].VirtualAddress, sizeof(ULONG_PTR)); + vl_append_str(vlbuf, ""/>"); }
- packet_reply_add(gdbctx, "</library>"); + vl_append_str(vlbuf, "</library>");
return TRUE; }
static void packet_query_libraries(struct gdb_context* gdbctx) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; BOOL opt;
/* this will resynchronize builtin dbghelp's internal ELF module list */ SymLoadModule(gdbctx->process->handle, 0, 0, 0, 0, 0);
- packet_reply_add(gdbctx, "<library-list>"); + vl_append_str(vlbuf, "<library-list>"); opt = SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, TRUE); SymEnumerateModules64(gdbctx->process->handle, packet_query_libraries_cb, gdbctx); SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, opt); - packet_reply_add(gdbctx, "</library-list>"); + vl_append_str(vlbuf, "</library-list>"); }
static void packet_query_threads(struct gdb_context* gdbctx) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread;
- packet_reply_add(gdbctx, "<threads>"); + vl_append_str(vlbuf, "<threads>"); LIST_FOR_EACH_ENTRY(thread, &process->threads, struct dbg_thread, entry) { - packet_reply_add(gdbctx, "<thread "); - packet_reply_add(gdbctx, "id=""); - packet_reply_val(gdbctx, thread->tid, 4); - packet_reply_add(gdbctx, "" name=""); - packet_reply_add(gdbctx, thread->name); - packet_reply_add(gdbctx, ""/>"); + vl_append_str(vlbuf, "<thread "); + vl_append_str(vlbuf, "id=""); + vl_append_uinthex(vlbuf, thread->tid, 4); + vl_append_str(vlbuf, "" name=""); + vl_append_str(vlbuf, thread->name); + vl_append_str(vlbuf, ""/>"); } - packet_reply_add(gdbctx, "</threads>"); + vl_append_str(vlbuf, "</threads>"); }
static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_cpu* cpu) { + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; const char* feature_prefix = NULL; const char* feature = NULL; char buffer[256]; int i;
- packet_reply_add(gdbctx, "<target>"); + vl_append_str(vlbuf, "<target>"); switch (cpu->machine) { case IMAGE_FILE_MACHINE_AMD64: - packet_reply_add(gdbctx, "<architecture>i386:x86-64</architecture>"); + vl_append_str(vlbuf, "<architecture>i386:x86-64</architecture>"); feature_prefix = "org.gnu.gdb.i386."; break; case IMAGE_FILE_MACHINE_I386: - packet_reply_add(gdbctx, "<architecture>i386</architecture>"); + vl_append_str(vlbuf, "<architecture>i386</architecture>"); feature_prefix = "org.gnu.gdb.i386."; break; case IMAGE_FILE_MACHINE_ARMNT: - packet_reply_add(gdbctx, "<architecture>arm</architecture>"); + vl_append_str(vlbuf, "<architecture>arm</architecture>"); feature_prefix = "org.gnu.gdb.arm."; break; case IMAGE_FILE_MACHINE_ARM64: - packet_reply_add(gdbctx, "<architecture>aarch64</architecture>"); + vl_append_str(vlbuf, "<architecture>aarch64</architecture>"); feature_prefix = "org.gnu.gdb.aarch64."; break; } @@ -1731,17 +1843,17 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c { if (cpu->gdb_register_map[i].feature) { - if (feature) packet_reply_add(gdbctx, "</feature>"); + if (feature) vl_append_str(vlbuf, "</feature>"); feature = cpu->gdb_register_map[i].feature;
- packet_reply_add(gdbctx, "<feature name=""); - if (feature_prefix) packet_reply_add(gdbctx, feature_prefix); - packet_reply_add(gdbctx, feature); - packet_reply_add(gdbctx, "">"); + vl_append_str(vlbuf, "<feature name=""); + if (feature_prefix) vl_append_str(vlbuf, feature_prefix); + vl_append_str(vlbuf, feature); + vl_append_str(vlbuf, "">");
if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 && strcmp(feature, "core") == 0) - packet_reply_add(gdbctx, "<flags id="i386_eflags" size="4">" + vl_append_str(vlbuf, "<flags id="i386_eflags" size="4">" "<field name="CF" start="0" end="0"/>" "<field name="" start="1" end="1"/>" "<field name="PF" start="2" end="2"/>" @@ -1763,7 +1875,7 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 && strcmp(feature, "sse") == 0) - packet_reply_add(gdbctx, "<vector id="v4f" type="ieee_single" count="4"/>" + vl_append_str(vlbuf, "<vector id="v4f" type="ieee_single" count="4"/>" "<vector id="v2d" type="ieee_double" count="2"/>" "<vector id="v16i8" type="int8" count="16"/>" "<vector id="v8i16" type="int16" count="8"/>" @@ -1798,20 +1910,20 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
snprintf(buffer, ARRAY_SIZE(buffer), "<reg name="%s" bitsize="%Iu"", cpu->gdb_register_map[i].name, 8 * cpu->gdb_register_map[i].length); - packet_reply_add(gdbctx, buffer); + vl_append_str(vlbuf, buffer);
if (cpu->gdb_register_map[i].type) { - packet_reply_add(gdbctx, " type=""); - packet_reply_add(gdbctx, cpu->gdb_register_map[i].type); - packet_reply_add(gdbctx, """); + vl_append_str(vlbuf, " type=""); + vl_append_str(vlbuf, cpu->gdb_register_map[i].type); + vl_append_str(vlbuf, """); }
- packet_reply_add(gdbctx, "/>"); + vl_append_str(vlbuf, "/>"); }
- if (feature) packet_reply_add(gdbctx, "</feature>"); - packet_reply_add(gdbctx, "</target>"); + if (feature) vl_append_str(vlbuf, "</feature>"); + vl_append_str(vlbuf, "</target>"); }
static enum packet_return packet_query(struct gdb_context* gdbctx) @@ -1941,32 +2053,56 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) case 'X': if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error;
- packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_libraries(gdbctx); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; }
if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error;
- packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_threads(gdbctx); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; }
if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2) { + BOOL more; + if (!gdbctx->process) return packet_error; if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
- packet_reply_open_xfer(gdbctx); + vl_empty(&gdbctx->qxfer_buffer); packet_query_target_xml(gdbctx, cpu); - packet_reply_close_xfer(gdbctx, off, len); + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + vl_empty(&gdbctx->qxfer_buffer); return packet_done; } break; @@ -2283,6 +2419,8 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0;
+ memset(&gdbctx->qxfer_buffer, 0, sizeof(gdbctx->qxfer_buffer)); + /* wait for first trap */ while (WaitForDebugEvent(&gdbctx->de, INFINITE)) {
On 11/16/21 17:51, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; };
+struct vl_buffer +{
- void *base;
- size_t len;
- size_t alloc;
+};
I think vl_ prefix is not very useful, something like strbuf_ would be more readable IMHO.
On 11/16/21 18:22, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; }; +struct vl_buffer +{ + void *base; + size_t len; + size_t alloc; +};
I think vl_ prefix is not very useful, something like strbuf_ would be more readable IMHO.
Thinking about it "string_buffer" would be even better, I don't know why we keep trying to make things short.
Since vl_buffer is intended for non-ASCII data (including null byte), how about "packet_buffer" or "reply_buffer"?
On 11/17/21 02:24, Rémi Bernon wrote:
On 11/16/21 18:22, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; }; +struct vl_buffer +{ + void *base; + size_t len; + size_t alloc; +};
I think vl_ prefix is not very useful, something like strbuf_ would be more readable IMHO.
Thinking about it "string_buffer" would be even better, I don't know why we keep trying to make things short.
On 11/16/21 18:27, Jinoh Kang wrote:
Since vl_buffer is intended for non-ASCII data (including null byte), how about "packet_buffer" or "reply_buffer"?
"reply_buffer" seems alright I guess.
On 11/17/21 02:33, Rémi Bernon wrote:
On 11/16/21 18:27, Jinoh Kang wrote:
Since vl_buffer is intended for non-ASCII data (including null byte), how about "packet_buffer" or "reply_buffer"?
"reply_buffer" seems alright I guess.
Thanks! So I guess the change shall be:
- struct vl_buffer -> struct reply_buffer - vl_append -> reply_buffer_append (ditto for other functions)
(also, sorry for top-posting)
On 11/16/21 18:34, Jinoh Kang wrote:
On 11/17/21 02:33, Rémi Bernon wrote:
On 11/16/21 18:27, Jinoh Kang wrote:
Since vl_buffer is intended for non-ASCII data (including null byte), how about "packet_buffer" or "reply_buffer"?
"reply_buffer" seems alright I guess.
Thanks! So I guess the change shall be:
- struct vl_buffer -> struct reply_buffer
- vl_append -> reply_buffer_append (ditto for other functions)
(also, sorry for top-posting)
Sounds good.
On 11/16/21 17:51, Jinoh Kang wrote:
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 284 +++++++++++++++++++++++++++--------- 1 file changed, 211 insertions(+), 73 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index bdb73659ade..caf64983c49 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -54,6 +54,13 @@ struct gdb_xpoint unsigned int value; };
+struct vl_buffer +{
- void *base;
- size_t len;
- size_t alloc;
+};
- struct gdb_context { /* gdb information */
@@ -82,6 +89,7 @@ struct gdb_context /* Unix environment */ ULONG_PTR wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ BOOL no_ack_mode;
struct vl_buffer qxfer_buffer; };
/* assume standard signal and errno values */
@@ -224,6 +232,102 @@ static void hex_to(char* dst, const void* src, size_t len) } }
+static void* buffer_realloc(void* buf, size_t size);
+static void vl_resize(struct vl_buffer* vlbuf, size_t alloc) +{
- vlbuf->alloc = alloc;
- vlbuf->base = buffer_realloc(vlbuf->base, vlbuf->alloc);
+}
+static void vl_empty(struct vl_buffer *vlbuf) +{
- vlbuf->len = 0;
- vl_resize(vlbuf, 0);
+}
+static void vl_grow(struct vl_buffer* vlbuf, size_t size) +{
- if (vlbuf->alloc < vlbuf->len + size)
vl_resize(vlbuf, ((vlbuf->len + size) / 32 + 1) * 32);+}
+static void vl_append(struct vl_buffer* vlbuf, const void *data, size_t size) +{
- vl_grow(vlbuf, size);
- memcpy((void *)((unsigned char *)vlbuf->base + vlbuf->len), data, size);
- vlbuf->len += size;
+}
+static inline void vl_append_str(struct vl_buffer* vlbuf, const char* str) +{
- vl_append(vlbuf, (const void *)str, strlen(str));
+}
+static inline void vl_append_uinthex(struct vl_buffer* vlbuf, ULONG_PTR val, int len) +{
- char buf[sizeof(ULONG_PTR) * 2], *ptr;
- assert(len <= sizeof(ULONG_PTR));
- for (ptr = buf + len * 2; ptr != buf; val >>= 4)
*--ptr = hex_to0(val & 0x0F);- vl_append(vlbuf, ptr, len * 2);
+}
+static const unsigned char xml_special_chars_lookup_table[16] = {
- /* The characters should be sorted by its value modulo table length. */
- 0x00, /* NUL */
- 0,
- 0x22, /* ": 0010|0010 */
- 0, 0, 0,
- 0x26, /* &: 0010|0110 */
- 0x27, /* ': 0010|0111 */
- 0, 0, 0, 0,
- 0x3C, /* <: 0011|1100 */
- 0,
- 0x3E, /* >: 0011|1110 */
- 0
+};
+static inline BOOL is_nul_or_xml_special_char(unsigned char val) +{
- /* Note: strcspn() uses lookup tables as well, but as of Wine 6.19
* msvcrt!strcspn allocates 1024 bytes (sizeof(BOOL)*256) of table* on the stack and populates it on the fly. It would be slower and less* cache-friendly than a preallocated, tiny static lookup table.*/- const size_t length = ARRAY_SIZE(xml_special_chars_lookup_table);
- return xml_special_chars_lookup_table[val % length] == val;
+}
+static void vl_append_xmlstr(struct vl_buffer* vlbuf, const char *str) +{
- const char *ptr = str, *sptr;
- for (;;)
- {
for (sptr = ptr; !is_nul_or_xml_special_char((unsigned char)*sptr); sptr++);vl_append(vlbuf, ptr, sptr - ptr);ptr = sptr;switch (*ptr++){case '"': vl_append_str(vlbuf, """); break;case '&': vl_append_str(vlbuf, "&"); break;case '\'': vl_append_str(vlbuf, "'"); break;case '<': vl_append_str(vlbuf, "<"); break;case '>': vl_append_str(vlbuf, ">"); break;default: return;}- }
+}
There too, I find the for loop less readable than a more usual
while (...) ptr++;
[...]
+static void packet_reply_xfer(
- struct gdb_context* gdbctx,
- const void *data,
- size_t datalen,
- unsigned int off,
- unsigned int len,
- BOOL *more_p
+)
FWIW I've seen other places where it's written like this, but Wine code is more usually breaking long lines on a (flexible) column limit, not on commas.
It's also usually aligned with the previous parenthese, (or using two indentation levels, but looking around in windbg, I think it's more appropriate to use tha aligned style here).
Replace the series of sscanf in packet_query with a dedicated qXfer request parser function and a lookup into a predefined handler table.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 218 ++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 58 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index caf64983c49..55c970c1d1e 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -61,6 +61,8 @@ struct vl_buffer size_t alloc; };
+#define MAX_QX_ANNEX_LEN 32 + struct gdb_context { /* gdb information */ @@ -89,6 +91,8 @@ struct gdb_context /* Unix environment */ ULONG_PTR wine_segs[3]; /* load addresses of the ELF wine exec segments (text, bss and data) */ BOOL no_ack_mode; + int qxfer_object_idx; + char qxfer_object_annex[MAX_QX_ANNEX_LEN]; struct vl_buffer qxfer_buffer; };
@@ -807,7 +811,7 @@ static int addr_width(struct gdb_context* gdbctx) }
enum packet_return {packet_error = 0x00, packet_ok = 0x01, packet_done = 0x02, - packet_last_f = 0x80}; + packet_send_buffer = 0x03, packet_last_f = 0x80};
static void* buffer_realloc(void* buf, size_t size) { @@ -1776,11 +1780,16 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO return TRUE; }
-static void packet_query_libraries(struct gdb_context* gdbctx) +static enum packet_return packet_query_libraries(struct gdb_context* gdbctx) { struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; BOOL opt;
+ if (!gdbctx->process) return packet_error; + + if (gdbctx->qxfer_object_annex[0]) + return packet_reply_error(gdbctx, 0); + /* this will resynchronize builtin dbghelp's internal ELF module list */ SymLoadModule(gdbctx->process->handle, 0, 0, 0, 0, 0);
@@ -1789,14 +1798,21 @@ static void packet_query_libraries(struct gdb_context* gdbctx) SymEnumerateModules64(gdbctx->process->handle, packet_query_libraries_cb, gdbctx); SymSetExtendedOption(SYMOPT_EX_WINE_NATIVE_MODULES, opt); vl_append_str(vlbuf, "</library-list>"); + + return packet_send_buffer; }
-static void packet_query_threads(struct gdb_context* gdbctx) +static enum packet_return packet_query_threads(struct gdb_context* gdbctx) { struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; struct dbg_process* process = gdbctx->process; struct dbg_thread* thread;
+ if (!process) return packet_error; + + if (gdbctx->qxfer_object_annex[0]) + return packet_reply_error(gdbctx, 0); + vl_append_str(vlbuf, "<threads>"); LIST_FOR_EACH_ENTRY(thread, &process->threads, struct dbg_thread, entry) { @@ -1808,11 +1824,12 @@ static void packet_query_threads(struct gdb_context* gdbctx) vl_append_str(vlbuf, ""/>"); } vl_append_str(vlbuf, "</threads>"); + + return packet_send_buffer; }
-static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_cpu* cpu) +static void packet_query_target_xml(struct gdb_context* gdbctx, struct vl_buffer *vlbuf, struct backend_cpu* cpu) { - struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; const char* feature_prefix = NULL; const char* feature = NULL; char buffer[256]; @@ -1926,10 +1943,89 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c vl_append_str(vlbuf, "</target>"); }
+static enum packet_return packet_query_features(struct gdb_context* gdbctx) +{ + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; + struct dbg_process* process = gdbctx->process; + + if (!process) return packet_error; + + if (strncmp(gdbctx->qxfer_object_annex, "target.xml", MAX_QX_ANNEX_LEN) == 0) + { + struct backend_cpu *cpu = process->be_cpu; + if (!cpu) return packet_error; + + packet_query_target_xml(gdbctx, vlbuf, cpu); + + return packet_send_buffer; + } + + return packet_reply_error(gdbctx, 0); +} + +struct qxfer +{ + const char* name; + enum packet_return (*handler)(struct gdb_context* gdbctx); +} qxfer_handlers[] = +{ + {"libraries", packet_query_libraries}, + {"threads" , packet_query_threads }, + {"features" , packet_query_features }, +}; + +static BOOL parse_xfer_read(const char *ptr, char *name_buf, size_t name_buf_size, char *annex_buf, size_t annex_buf_size, unsigned int *offp, unsigned int *lenp) +{ + const char *name, *annex; + char *endptr; + size_t name_size, annex_size; + unsigned long off, len; + + if (strncmp(ptr, "Xfer:", 5) != 0) + return FALSE; + ptr += 5; + + name = ptr; + ptr = strchr(ptr, ':'); + if (!ptr || (name_size = ptr - name) >= name_buf_size) + return FALSE; + ptr++; + + if (strncmp(ptr, "read:", 5) != 0) + return FALSE; + ptr += 5; + + annex = ptr; + ptr = strchr(ptr, ':'); + if (!ptr || (annex_size = ptr - annex) >= annex_buf_size) + return FALSE; + ptr++; + + off = strtoul(ptr, &endptr, 16); + if (endptr == ptr || *endptr != ',' || off > (unsigned int)-1) + return FALSE; + ptr = endptr + 1; + + len = strtoul(ptr, &endptr, 16); + if (endptr == ptr || *endptr != '\0' || len > (unsigned int)-1) + return FALSE; + + memcpy(name_buf, name, name_size); + memset(name_buf + name_size, '\0', name_buf_size - name_size); + + memcpy(annex_buf, annex, annex_size); + memset(annex_buf + annex_size, '\0', annex_buf_size - annex_size); + + *offp = off; + *lenp = len; + + return TRUE; +} + static enum packet_return packet_query(struct gdb_context* gdbctx) { + char object_name[32], annex[MAX_QX_ANNEX_LEN]; unsigned int off, len; - struct backend_cpu *cpu;
switch (gdbctx->in_packet[0]) { @@ -2016,11 +2112,16 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) return packet_ok; if (strncmp(gdbctx->in_packet, "Supported", 9) == 0) { + size_t i; + packet_reply_open(gdbctx); packet_reply_add(gdbctx, "QStartNoAckMode+;"); - packet_reply_add(gdbctx, "qXfer:libraries:read+;"); - packet_reply_add(gdbctx, "qXfer:threads:read+;"); - packet_reply_add(gdbctx, "qXfer:features:read+;"); + for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++) + { + packet_reply_add(gdbctx, "qXfer:"); + packet_reply_add(gdbctx, qxfer_handlers[i].name); + packet_reply_add(gdbctx, ":read+;"); + } packet_reply_close(gdbctx); return packet_done; } @@ -2051,59 +2152,58 @@ static enum packet_return packet_query(struct gdb_context* gdbctx) } break; case 'X': - if (sscanf(gdbctx->in_packet, "Xfer:libraries:read::%x,%x", &off, &len) == 2) + if (parse_xfer_read(gdbctx->in_packet, + object_name, sizeof(object_name), + annex, sizeof(annex), + &off, &len)) { - BOOL more; + enum packet_return result; + int i; + + for (i = 0; ; i++) + { + if (i >= ARRAY_SIZE(qxfer_handlers)) + { + ERR("unhandled qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len); + return packet_error; + } + if (strcmp(qxfer_handlers[i].name, object_name) == 0) + break; + } + + TRACE("qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);
- if (!gdbctx->process) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_libraries(gdbctx); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); vl_empty(&gdbctx->qxfer_buffer); - return packet_done; - } - - if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2) - { - BOOL more;
- if (!gdbctx->process) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_threads(gdbctx); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); - vl_empty(&gdbctx->qxfer_buffer); - return packet_done; - } - - if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2) - { - BOOL more; - - if (!gdbctx->process) return packet_error; - if (!(cpu = gdbctx->process->be_cpu)) return packet_error; - - vl_empty(&gdbctx->qxfer_buffer); - packet_query_target_xml(gdbctx, cpu); - packet_reply_xfer( - gdbctx, - gdbctx->qxfer_buffer.base, - gdbctx->qxfer_buffer.len, - off, len, &more - ); - vl_empty(&gdbctx->qxfer_buffer); - return packet_done; + gdbctx->qxfer_object_idx = i; + memcpy(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN); + + result = (*qxfer_handlers[i].handler)(gdbctx); + TRACE("qXfer read result = %d\n", result); + + if ((result & ~packet_last_f) == packet_send_buffer) + { + BOOL more; + + packet_reply_xfer( + gdbctx, + gdbctx->qxfer_buffer.base, + gdbctx->qxfer_buffer.len, + off, len, &more + ); + if (!more) + vl_empty(&gdbctx->qxfer_buffer); + + result = (result & packet_last_f) | packet_done; + } + else + { + gdbctx->qxfer_object_idx = -1; + memset(gdbctx->qxfer_object_annex, 0, MAX_QX_ANNEX_LEN); + vl_empty(&gdbctx->qxfer_buffer); + } + + return result; } break; } @@ -2419,6 +2519,8 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne for (i = 0; i < ARRAY_SIZE(gdbctx->wine_segs); i++) gdbctx->wine_segs[i] = 0;
+ gdbctx->qxfer_object_idx = -1; + memset(gdbctx->qxfer_object_annex, 0, sizeof(gdbctx->qxfer_object_annex)); memset(&gdbctx->qxfer_buffer, 0, sizeof(gdbctx->qxfer_buffer));
/* wait for first trap */
On 11/16/21 17:51, Jinoh Kang wrote:
Replace the series of sscanf in packet_query with a dedicated qXfer request parser function and a lookup into a predefined handler table.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 218 ++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 58 deletions(-)
To be honest I find the sscanf version much more readable, especially as gdb protocol is not always easy to follow, and the number of possible qxfer variants (3) doesn't really justify to use a dispatch table imho.
Is this change solving anything in particular?
On 11/17/21 03:43, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Replace the series of sscanf in packet_query with a dedicated qXfer request parser function and a lookup into a predefined handler table.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 218 ++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 58 deletions(-)
To be honest I find the sscanf version much more readable, especially as gdb protocol is not always easy to follow,
I do too. It's just that I couldn't figure out how to keep sscanf (with its format specifiers) in the generalised parser.
The annex part of "libraries" and "threads" is always empty, but for "features" it is not. Therefore, extracting the annex part requires matching a *possibly empty* sequence of characters in a desired character set. '%[]' can no longer be used, as it requires at least one character.
Any suggestions are welcome.
and the number of possible qxfer variants (3) doesn't really justify to use a dispatch table imho.> Is this change solving anything in particular?
Apologies for not clarifying the purpose of this patch upfront. The following two patches do depend on this generalisation.
On 11/17/21 09:24, Jinoh Kang wrote:
On 11/17/21 03:43, Rémi Bernon wrote:
On 11/16/21 17:51, Jinoh Kang wrote:
Replace the series of sscanf in packet_query with a dedicated qXfer request parser function and a lookup into a predefined handler table.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com
programs/winedbg/gdbproxy.c | 218 ++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 58 deletions(-)
To be honest I find the sscanf version much more readable, especially as gdb protocol is not always easy to follow,
I do too. It's just that I couldn't figure out how to keep sscanf (with its format specifiers) in the generalised parser.
The annex part of "libraries" and "threads" is always empty, but for "features" it is not. Therefore, extracting the annex part requires matching a *possibly empty* sequence of characters in a desired character set. '%[]' can no longer be used, as it requires at least one character.
Any suggestions are welcome.
What about having two cases like this?
if (sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read::%x,%x", topic, &off, &len) == 3 || sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read:%256[^:]:%x,%x", topic, annex, &off, &len) == 4)
Or splitting the topic off, but I think having the patterns like this you can easily grasp how the protocol is supposed to be.
GDB does not retrieve the result of a qXfer command at once; instead, it issues a series of requests to obtain the result one "chunk" at a time, and concatenates those chunks internally. Each request contains offset and length variables that specify which portion of the result shall be retrieved.
Today, Winedbg handles this by generating the entire result data each time a request is received and slicing out the requested range for the response. This is not only inefficient due to repeated computation, but also prone to race condition since the result may change between successive chunk requests due to the dynamic nature of some commands such as "libraries" and "threads."
Fix this by cacheing the result into a buffer at the first request, and use the buffer to serve successive chunk requests. The cache is invalidated when the remote requests a different object, or the debugger reaches the end of the result cache buffer.
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 55c970c1d1e..19ad76910af 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -2173,13 +2173,24 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
TRACE("qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);
- vl_empty(&gdbctx->qxfer_buffer); + if (off > 0 && + gdbctx->qxfer_buffer.len > 0 && + gdbctx->qxfer_object_idx == i && + strncmp(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN) == 0) + { + result = packet_send_buffer; + TRACE("qXfer read result = %d (cached)\n", result); + } + else + { + vl_empty(&gdbctx->qxfer_buffer);
- gdbctx->qxfer_object_idx = i; - memcpy(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN); + gdbctx->qxfer_object_idx = i; + memcpy(gdbctx->qxfer_object_annex, annex, MAX_QX_ANNEX_LEN);
- result = (*qxfer_handlers[i].handler)(gdbctx); - TRACE("qXfer read result = %d\n", result); + result = (*qxfer_handlers[i].handler)(gdbctx); + TRACE("qXfer read result = %d\n", result); + }
if ((result & ~packet_last_f) == packet_send_buffer) {
Remove the necessity of using the `file` command to specify the executable.
Otherwise, RHEL's downstream GDB complains with the following message:
Remote gdbserver does not support determining executable automatically. RHEL <=6.8 and <=7.2 versions of gdbserver do not support such automatic executable detection. The following versions of gdbserver support it: - Upstream version of gdbserver (unsupported) 7.10 or later - Red Hat Developer Toolset (DTS) version of gdbserver from DTS 4.0 or later (only on x86_64) - RHEL-7.3 versions of gdbserver (on any architecture)
Signed-off-by: Jinoh Kang jinoh.kang.kr@gmail.com --- programs/winedbg/gdbproxy.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)
diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c index 19ad76910af..27f59aac259 100644 --- a/programs/winedbg/gdbproxy.c +++ b/programs/winedbg/gdbproxy.c @@ -99,7 +99,10 @@ struct gdb_context /* assume standard signal and errno values */ enum host_error { + HOST_EPERM = 1, + HOST_ENOENT = 2, HOST_ESRCH = 3, + HOST_ENOMEM = 12, HOST_EFAULT = 14, HOST_EINVAL = 22, }; @@ -1963,6 +1966,33 @@ static enum packet_return packet_query_features(struct gdb_context* gdbctx) return packet_reply_error(gdbctx, 0); }
+static enum packet_return packet_query_exec_file(struct gdb_context* gdbctx) +{ + struct vl_buffer *vlbuf = &gdbctx->qxfer_buffer; + struct dbg_process* process = gdbctx->process; + char *unix_path; + BOOL is_wow64; + char *tmp; + + if (!process) return packet_error; + + if (gdbctx->qxfer_object_annex[0] || !process->imageName) + return packet_reply_error(gdbctx, HOST_EPERM); + + if (!(unix_path = wine_get_unix_file_name(process->imageName))) + return packet_reply_error(gdbctx, GetLastError() == ERROR_NOT_ENOUGH_MEMORY ? HOST_ENOMEM : HOST_ENOENT); + + if (IsWow64Process(process->handle, &is_wow64) && + is_wow64 && (tmp = strstr(unix_path, "system32"))) + memcpy(tmp, "syswow64", 8); + + vl_append_str(vlbuf, unix_path); + + HeapFree(GetProcessHeap(), 0, unix_path); + + return packet_send_buffer; +} + struct qxfer { const char* name; @@ -1972,6 +2002,7 @@ struct qxfer {"libraries", packet_query_libraries}, {"threads" , packet_query_threads }, {"features" , packet_query_features }, + {"exec-file", packet_query_exec_file}, };
static BOOL parse_xfer_read(const char *ptr, char *name_buf, size_t name_buf_size, char *annex_buf, size_t annex_buf_size, unsigned int *offp, unsigned int *lenp)
-
Jinoh Kang -
Rémi Bernon