[PATCH v3 0/9] winedbg(gdbproxy): don't misbehave on module names w/ special characters; modernise

[Jinoh Kang]

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(-)

-- 
2.31.1

[PATCH v3 1/9] winedbg: Crash when allocation fails in packet_realloc().

[Jinoh Kang]

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(a)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)
-- 
2.31.1

[PATCH v3 2/9] winedbg: Free buffer and return NULL if size==0 in packet_realloc().

[Jinoh Kang]

packet_realloc() does not return on error, so we can use NULL for normal
circumstances.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)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);
-- 
2.31.1

[PATCH v3 3/9] winedbg: Generalise packet_realloc(), rename to buffer_realloc().

[Jinoh Kang]

Make it return void * and accept size_t as the size.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)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;
-- 
2.31.1

[PATCH v3 3/9] winedbg: Generalise packet_realloc(), rename to buffer_realloc().

[Rémi Bernon]

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(a)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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 3/9] winedbg: Generalise packet_realloc(), rename to buffer_realloc().

[Jinoh Kang]

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(a)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)?

-- 
Sincerely,
Jinoh Kang

[PATCH v3 3/9] winedbg: Generalise packet_realloc(), rename to buffer_realloc().

[Rémi Bernon]

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(a)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:
>
> 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)?

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,
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 4/9] winedbg: Use unsigned int for offset/length in GDB qXfer handler.

[Jinoh Kang]

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(a)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])
-- 
2.31.1

[PATCH v3 5/9] winedbg: Escape special characters in GDB packet reply.

[Jinoh Kang]

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(a)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);
-- 
2.31.1

[PATCH v3 5/9] winedbg: Escape special characters in GDB packet reply.

[Rémi Bernon]

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(a)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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 5/9] winedbg: Escape special characters in GDB packet reply.

[Jinoh Kang]

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(a)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.

-- 
Sincerely,
Jinoh Kang

[PATCH v3 5/9] winedbg: Escape special characters in GDB packet reply.

[Rémi Bernon]

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:
>
> 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.

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?
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)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))
     {
-- 
2.31.1

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

On 11/16/21 17:51, Jinoh Kang wrote:
> Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

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(a)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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

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(a)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.

-- 
Sincerely,
Jinoh Kang

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

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)

-- 
Sincerely,
Jinoh Kang

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 6/9] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

On 11/16/21 17:51, Jinoh Kang wrote:
> Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)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).
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 7/9] winedbg: Define table for GDB qXfer command handlers.

[Jinoh Kang]

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(a)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 */
-- 
2.31.1

[PATCH v3 7/9] winedbg: Define table for GDB qXfer command handlers.

[Rémi Bernon]

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(a)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?
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 7/9] winedbg: Define table for GDB qXfer command handlers.

[Jinoh Kang]

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(a)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.

-- 
Sincerely,
Jinoh Kang

[PATCH v3 7/9] winedbg: Define table for GDB qXfer command handlers.

[Rémi Bernon]

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(a)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.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v3 8/9] winedbg: Cache GDB qXfer command result for chunked fetching.

[Jinoh Kang]

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(a)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)
             {
-- 
2.31.1

[PATCH v3 9/9] winedbg: Implement GDB qXfer object exec-file.

[Jinoh Kang]

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(a)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)
-- 
2.31.1