[PATCH v6 0/6] 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
v3 -> v4:
- Fix pointer variable declaration style
- Synchronize to prior modified patches.
- Refactor the code a bit
- Revert to scanf() when parsing Xfer packet lines
- Fix indentation
- s/buffer_realloc/realloc/g
- s/vl_buffer/reply_buffer/g; s/vl_/reply_buffer_/g; s/vlbuf/reply/g
- Rewrite reply_buffer_add_uinthex(): convert for loop into while loop
- Rewrite reply_buffer_append_xmlstr(): convert for loop into while loop
- Split packet_reply_add() into packet_reply_add_data() here
- Rewrite packet_reply_add for clarity.
- Update Wine version in comment.
v4 -> v5:
- Fix pointer variable declaration style.
- Synchronize to prior modified patches.
- Split out XML escaping in a separate patch
- Don't realloc/free when emptying
- Drop reply_buffer_resize(), merge into reply_buffer_grow()
- Remove "more" variables for now
- Don't empty the reply buffer twice
- Use two sscanf for alternative patterns
- Don't always zero-initialise qxfer_object_annex
- Use strcpy (I just did what I was told to do)
- Use strcmp (I just did what I was told to do)
v5 -> v6:
- Remove a gratuitous blank line
- Edit commit messages

Jinoh Kang (6):
  winedbg: Refactor gdb_context::out_{buf*,len} into reply_buffer.
  winedbg: Buffer output of GDB qXfer commands for proper slicing.
  winedbg: Escape XML special characters in qXfer reply.
  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 | 602 ++++++++++++++++++++++++------------
 1 file changed, 404 insertions(+), 198 deletions(-)

[PATCH v6 1/6] winedbg: Refactor gdb_context::out_{buf*,len} into reply_buffer.

[Jinoh Kang]

This is required for a subsequent patch that adds buffering for
GDB qXfer reply data.

Signed-off-by: Jinoh Kang 
---

Notes:
    v5 -> v6: Ddit commit message

programs/winedbg/gdbproxy.c | 141 ++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 54 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index c1d0bda1a41..2e7e04e6c93 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -54,6 +54,13 @@ struct gdb_xpoint
     unsigned int value;
 };

+struct reply_buffer
+{
+    void *base;
+    size_t len;
+    size_t alloc;
+};
+
 struct gdb_context
 {
     /* gdb information */
@@ -66,9 +73,7 @@ struct gdb_context
     char*                       in_packet;
     int                         in_packet_len;
     /* outgoing buffer */
-    char*                       out_buf;
-    int                         out_buf_alloc;
-    int                         out_len;
+    struct reply_buffer         out_buf;
     int                         out_curr_packet;
     /* generic GDB thread information */
     int                         exec_tid; /* tid used in step & continue */
@@ -224,12 +229,62 @@ static void hex_to(char* dst, const void* src, size_t len)
     }
 }

-static unsigned char checksum(const char* ptr, int len)
+static void reply_buffer_empty(struct reply_buffer* reply)
+{
+    reply->len = 0;
+}
+
+static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
+{
+    if (reply->alloc < reply->len + size)
+    {
+        reply->alloc = ((reply->len + size) / 32 + 1) * 32;
+        reply->base = realloc(reply->base, reply->alloc);
+    }
+}
+
+static void reply_buffer_append(struct reply_buffer* reply, const void* data, size_t size)
+{
+    reply_buffer_grow(reply, size);
+    memcpy((unsigned char *)reply->base + reply->len, data, size);
+    reply->len += size;
+}
+
+static inline void reply_buffer_append_str(struct reply_buffer* reply, const char* str)
+{
+    reply_buffer_append(reply, (const void *)str, strlen(str));
+}
+
+static inline void reply_buffer_append_hex(struct reply_buffer* reply, const void* src, size_t len)
+{
+    reply_buffer_grow(reply, len * 2);
+    hex_to((char *)reply->base + reply->len, src, len);
+    reply->len += len * 2;
+}
+
+static inline void reply_buffer_append_uinthex(struct reply_buffer* reply, ULONG_PTR val, int len)
+{
+    char buf[sizeof(ULONG_PTR) * 2], *ptr;
+
+    assert(len <= sizeof(ULONG_PTR));
+
+    ptr = buf + len * 2;
+    while (ptr != buf)
+    {
+        *--ptr = hex_to0(val & 0x0F);
+        val >>= 4;
+    }
+
+    reply_buffer_append(reply, ptr, len * 2);
+}
+
+static unsigned char checksum(const void* data, int len)
 {
     unsigned cksum = 0;
+    const unsigned char* ptr = data;

while (len-- > 0)
-        cksum += (unsigned char)*ptr++;
+        cksum += *ptr++;
     return cksum;
 }

@@ -705,20 +760,9 @@ 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 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 = realloc(gdbctx->out_buf, gdbctx->out_buf_alloc);
-    }
-}
-
 static void packet_reply_hex_to(struct gdb_context* gdbctx, const void* src, int len)
 {
-    packet_reply_grow(gdbctx, len * 2);
-    hex_to(&gdbctx->out_buf[gdbctx->out_len], src, len);
-    gdbctx->out_len += len * 2;
+    reply_buffer_append_hex(&gdbctx->out_buf, src, len);
 }

static inline void packet_reply_hex_to_str(struct gdb_context* gdbctx, const char* src)
@@ -728,15 +772,7 @@ static inline void packet_reply_hex_to_str(struct gdb_context* gdbctx, const cha

static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len)
 {
-    int i, shift;
-
-    shift = (len - 1) * 8;
-    packet_reply_grow(gdbctx, len * 2);
-    for (i = 0; i < len; i++, shift -= 8)
-    {
-        gdbctx->out_buf[gdbctx->out_len++] = hex_to0((val >> (shift + 4)) & 0x0F);
-        gdbctx->out_buf[gdbctx->out_len++] = hex_to0((val >>  shift     ) & 0x0F);
-    }
+    reply_buffer_append_uinthex(&gdbctx->out_buf, val, len);
 }

static const unsigned char gdb_special_chars_lookup_table[4] = {
@@ -764,6 +800,7 @@ static inline BOOL is_gdb_special_char(unsigned char val)
 static void packet_reply_add(struct gdb_context* gdbctx, const char* str)
 {
     const unsigned char *ptr = (unsigned char *)str, *curr;
+    unsigned char esc_seq[2];

while (*ptr)
     {
@@ -772,23 +809,20 @@ static void packet_reply_add(struct gdb_context* gdbctx, const char* str)
         while (*ptr && !is_gdb_special_char(*ptr))
             ptr++;

-        packet_reply_grow(gdbctx, ptr - curr);
-        memcpy(&gdbctx->out_buf[gdbctx->out_len], curr, ptr - curr);
-        gdbctx->out_len += ptr - curr;
+        reply_buffer_append(&gdbctx->out_buf, curr, ptr - curr);
         if (!*ptr) break;

-        packet_reply_grow(gdbctx, 2);
-        gdbctx->out_buf[gdbctx->out_len++] = 0x7D;
-        gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++;
+        esc_seq[0] = 0x7D;
+        esc_seq[1] = 0x20 ^ *ptr++;
+        reply_buffer_append(&gdbctx->out_buf, esc_seq, 2);
     }
 }

static void packet_reply_open(struct gdb_context* gdbctx)
 {
     assert(gdbctx->out_curr_packet == -1);
-    packet_reply_grow(gdbctx, 1);
-    gdbctx->out_buf[gdbctx->out_len++] = '$';
-    gdbctx->out_curr_packet = gdbctx->out_len;
+    reply_buffer_append(&gdbctx->out_buf, "$", 1);
+    gdbctx->out_curr_packet = gdbctx->out_buf.len;
 }

static void packet_reply_close(struct gdb_context* gdbctx)
@@ -796,10 +830,9 @@ static void packet_reply_close(struct gdb_context* gdbctx)
     unsigned char       cksum;
     int plen;

-    plen = gdbctx->out_len - gdbctx->out_curr_packet;
-    packet_reply_grow(gdbctx, 1);
-    gdbctx->out_buf[gdbctx->out_len++] = '#';
-    cksum = checksum(&gdbctx->out_buf[gdbctx->out_curr_packet], plen);
+    plen = gdbctx->out_buf.len - gdbctx->out_curr_packet;
+    reply_buffer_append(&gdbctx->out_buf, "#", 1);
+    cksum = checksum((unsigned char *)gdbctx->out_buf.base + gdbctx->out_curr_packet, plen);
     packet_reply_hex_to(gdbctx, &cksum, 1);
     gdbctx->out_curr_packet = -1;
 }
@@ -815,20 +848,22 @@ static void packet_reply_close_xfer(struct gdb_context* gdbctx, unsigned int off
     int begin = gdbctx->out_curr_packet + 1;
     int plen;

-    if (begin + off < gdbctx->out_len)
+    if (begin + off < gdbctx->out_buf.len)
     {
-        gdbctx->out_len -= off;
-        memmove(gdbctx->out_buf + begin, gdbctx->out_buf + begin + off, gdbctx->out_len);
+        gdbctx->out_buf.len -= off;
+        memmove((unsigned char *)gdbctx->out_buf.base + begin,
+                (unsigned char *)gdbctx->out_buf.base + begin + off,
+                gdbctx->out_buf.len);
     }
     else
     {
-        gdbctx->out_buf[gdbctx->out_curr_packet] = 'l';
-        gdbctx->out_len = gdbctx->out_curr_packet + 1;
+        *((unsigned char *)gdbctx->out_buf.base + gdbctx->out_curr_packet) = 'l';
+        gdbctx->out_buf.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';
+    plen = gdbctx->out_buf.len - begin;
+    if (plen > len) gdbctx->out_buf.len -= (plen - len);
+    else *((unsigned char *)gdbctx->out_buf.base + gdbctx->out_curr_packet) = 'l';

packet_reply_close(gdbctx);
 }
@@ -2098,10 +2133,10 @@ static BOOL extract_packets(struct gdb_context* gdbctx)
             case packet_done:   break;
             }

-            TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf, gdbctx->out_len));
-            i = send(gdbctx->sock, gdbctx->out_buf, gdbctx->out_len, 0);
-            assert(i == gdbctx->out_len);
-            gdbctx->out_len = 0;
+            TRACE("Reply: %s\n", debugstr_an(gdbctx->out_buf.base, gdbctx->out_buf.len));
+            i = send(gdbctx->sock, gdbctx->out_buf.base, gdbctx->out_buf.len, 0);
+            assert(i == gdbctx->out_buf.len);
+            reply_buffer_empty(&gdbctx->out_buf);
         }
         else
             WARN("Ignoring: %s (checksum: %d != %d)\n", debugstr_an(ptr, sum - ptr),
@@ -2255,9 +2290,7 @@ static BOOL gdb_init_context(struct gdb_context* gdbctx, unsigned flags, unsigne
     gdbctx->in_buf = NULL;
     gdbctx->in_buf_alloc = 0;
     gdbctx->in_len = 0;
-    gdbctx->out_buf = NULL;
-    gdbctx->out_buf_alloc = 0;
-    gdbctx->out_len = 0;
+    memset(&gdbctx->out_buf, 0, sizeof(gdbctx->out_buf));
     gdbctx->out_curr_packet = -1;

gdbctx->exec_tid = -1;

[PATCH v6 1/6] winedbg: Refactor gdb_context::out_{buf*, len} into reply_buffer.

[Rémi Bernon]

Hi Jinoh!

A few nitpicks, otherwise it looks good.

On 11/22/21 15:59, Jinoh Kang wrote:
> This is required for a subsequent patch that adds buffering for
> GDB qXfer reply data.
> Signed-off-by: Jinoh Kang
>
> Notes:
>      v5 -> v6: Ddit commit message
>
> programs/winedbg/gdbproxy.c | 141 ++++++++++++++++++++++--------------
>   1 file changed, 87 insertions(+), 54 deletions(-)
>
> diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
> index c1d0bda1a41..2e7e04e6c93 100644
> --- a/programs/winedbg/gdbproxy.c
> +++ b/programs/winedbg/gdbproxy.c
> @@ -54,6 +54,13 @@ struct gdb_xpoint
>       unsigned int value;
>   };
>
> +struct reply_buffer
> +{
> - void *base;
> - size_t len;
> - size_t alloc;
>
> +};
> -

I think you can save a few casts by making base an unsigned char*. I 
don't think it's really useful to keep void* here (also note that this 
file seems to use left pointer alignment everywhere).
> struct gdb_context
>   {
>       /* gdb information */
> @@ -66,9 +73,7 @@ struct gdb_context
>       char*                       in_packet;
>       int                         in_packet_len;
>       /* outgoing buffer */
> - char*                       out_buf;
> - int                         out_buf_alloc;
> - int                         out_len;
> - struct reply_buffer         out_buf;
>  int                         out_curr_packet;
>  /* generic GDB thread information */
>  int                         exec_tid; /* tid used in step & continue */
>
> @@ -224,12 +229,62 @@ static void hex_to(char* dst, const void* src, size_t len)
>       }
>   }
>
> -static unsigned char checksum(const char* ptr, int len)
> +static void reply_buffer_empty(struct reply_buffer* reply)
> +{
> - reply->len = 0;
>
> +}
> -

I know C++ made it canonical to use "empty" as a verb to clear 
containers but I feel like "reset" or "clear" is better.
> +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
> +{
> - if (reply->alloc < reply->len + size)
> - {
> -
>    reply->alloc = ((reply->len + size) / 32 + 1) * 32;
> -
>    reply->base = realloc(reply->base, reply->alloc);
> - }
>
> +}

Could we maybe grow by more than that? Something like:

(max(reply->alloc * 3 / 2, reply->len + size) + 0x1f) & ~0x1f

(I would even prefer to drop the alignment, but I don't remember why or 
if it was actually needed)

Or maybe we don't want to do that in this patch, which should not change 
the logic... As you prefer.
> -
>
> +static void reply_buffer_append(struct reply_buffer* reply, const void* data, size_t size)
> +{
> - reply_buffer_grow(reply, size);
> - memcpy((unsigned char *)reply->base + reply->len, data, size);
> - reply->len += size;
>
> +}
> -
>
> +static inline void reply_buffer_append_str(struct reply_buffer* reply, const char* str)
> +{
> - reply_buffer_append(reply, (const void *)str, strlen(str));
>
> +}
> -

This function is unused in this patch, it should be added in the next 
one, where it is. You probably don't need the inline by the way, here or 
elsewhere, and you don't need to cast to const void* either.
> +static inline void reply_buffer_append_hex(struct reply_buffer* reply, const void* src, size_t len)
> +{
> - reply_buffer_grow(reply, len * 2);
> - hex_to((char *)reply->base + reply->len, src, len);
> - reply->len += len * 2;
>
> +}
> -

Cheers,

[PATCH v6 1/6] winedbg: Refactor gdb_context::out_{buf*, len} into reply_buffer.

[Jinoh Kang]

On 11/23/21 20:20, Rémi Bernon wrote:
> Hi Jinoh!
>
> A few nitpicks, otherwise it looks good.
>
> On 11/22/21 15:59, Jinoh Kang wrote:
> > This is required for a subsequent patch that adds buffering for
> > GDB qXfer reply data.
> > Signed-off-by: Jinoh Kang
> >
> > Notes:
> >      v5 -> v6: Ddit commit message
> >
> > programs/winedbg/gdbproxy.c | 141 ++++++++++++++++++++++--------------
> >   1 file changed, 87 insertions(+), 54 deletions(-)
> >
> > diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
> > index c1d0bda1a41..2e7e04e6c93 100644
> > --- a/programs/winedbg/gdbproxy.c
> > +++ b/programs/winedbg/gdbproxy.c
> > @@ -54,6 +54,13 @@ struct gdb_xpoint
> >       unsigned int value;
> >   };
> >   +struct reply_buffer
> > +{
> > +    void *base;
> > +    size_t len;
> > +    size_t alloc;
> > +};
> > -
>
> I think you can save a few casts by making base an unsigned char*. I don't think it's really useful to keep void* here (also note that this file seems to use left pointer alignment everywhere).
> > struct gdb_context
> >   {
> >       /* gdb information */
> > @@ -66,9 +73,7 @@ struct gdb_context
> >       char*                       in_packet;
> >       int                         in_packet_len;
> >       /* outgoing buffer */
> > -    char*                       out_buf;
> > -    int                         out_buf_alloc;
> > -    int                         out_len;
> > +    struct reply_buffer         out_buf;
> >       int                         out_curr_packet;
> >       /* generic GDB thread information */
> >       int                         exec_tid; /* tid used in step & continue */
> > @@ -224,12 +229,62 @@ static void hex_to(char* dst, const void* src, size_t len)
> >       }
> >   }
> >   -static unsigned char checksum(const char* ptr, int len)
> > +static void reply_buffer_empty(struct reply_buffer* reply)
> > +{
> > +    reply->len = 0;
> > +}
> > -
>
> I know C++ made it canonical to use "empty" as a verb to clear containers but I feel like "reset" or "clear" is better.
> > +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
> > +{
> > +    if (reply->alloc < reply->len + size)
> > +    {
> > +        reply->alloc = ((reply->len + size) / 32 + 1) * 32;
> > +        reply->base = realloc(reply->base, reply->alloc);
> > +    }
> > +}
>
> Could we maybe grow by more than that? Something like:
>
> (max(reply->alloc * 3 / 2, reply->len + size) + 0x1f) & ~0x1f
>
> (I would even prefer to drop the alignment, but I don't remember why or if it was actually needed)
>
> Or maybe we don't want to do that in this patch, which should not change the logic... As you prefer.
> > -
> >
> > +static void reply_buffer_append(struct reply_buffer* reply, const void* data, size_t size)
> > +{
> > +    reply_buffer_grow(reply, size);
> > +    memcpy((unsigned char *)reply->base + reply->len, data, size);
> > +    reply->len += size;
> > +}
> > -
> >
> > +static inline void reply_buffer_append_str(struct reply_buffer* reply, const char* str)
> > +{
> > +    reply_buffer_append(reply, (const void *)str, strlen(str));
> > +}
> > -
>
> This function is unused in this patch, it should be added in the next one, where it is. You probably don't need the inline by the way, here or elsewhere, and you don't need to cast to const void* either.

I just realised I forgot to take the inline keyword out.  The reason why inline
is there was simply because the old code was using it; nothing more.  I don't
think its presence or absence would make a big difference, though.
> > +static inline void reply_buffer_append_hex(struct reply_buffer* reply, const void* src, size_t len)
> > +{
> > +    reply_buffer_grow(reply, len * 2);
> > +    hex_to((char *)reply->base + reply->len, src, len);
> > +    reply->len += len * 2;
> > +}
> > -
>
> Cheers,

[PATCH v6 1/6] winedbg: Refactor gdb_context::out_{buf*, len} into reply_buffer.

[Rémi Bernon]

On 11/23/21 16:15, Jinoh Kang wrote:
> > > +static inline void reply_buffer_append_str(struct reply_buffer* reply, const char* str)
> > > +{
> > > +    reply_buffer_append(reply, (const void *)str, strlen(str));
> > > +}
> > > -
> >
> > This function is unused in this patch, it should be added in the next one, where it is. You probably don't need the inline by the way, here or elsewhere, and you don't need to cast to const void* either.
>
> I just realised I forgot to take the inline keyword out.  The reason why inline
> is there was simply because the old code was using it; nothing more.  I don't
> think its presence or absence would make a big difference, though.

Yes, it doesn't really matter. It silents the unused function warning 
with GCC though, Clang still emits it.

[PATCH v6 2/6] winedbg: Buffer output of GDB qXfer commands for proper slicing.

[Jinoh Kang]

Today, gdbproxy reuses the same buffer for both the qXfer reply and the
actual GDB packet reply.  This worked well, since each byte in the qXfer
reply buffer matched 1:1 to each byte in the actual GDB reply packet.

Since we escape special characters now, this property no longer holds
and a single byte in qXfer reply will take up to two bytes in the GDB
reply packet.  This causes offsets to shift, preventing the
offset/length response slicing (part of GDB protocol) from working
correctly.

Fix this by writing the qXfer reply data in a separate buffer, and
performing slicing out of it.

Signed-off-by: Jinoh Kang 
---

Notes:
    v3 -> v4:
    - Fix pointer variable declaration style
    - Fix indentation
    - s/buffer_realloc/realloc/g
    - s/vl_buffer/reply_buffer/g; s/vl_/reply_buffer_/g; s/vlbuf/reply/g
    - Rewrite reply_buffer_add_uinthex(): convert for loop into while loop
    - Rewrite reply_buffer_append_xmlstr(): convert for loop into while loop
    - Split packet_reply_add() into packet_reply_add_data() here

v4 -> v5:
    - Split out XML escaping in a separate patch
    - Don't realloc/free when emptying
    - Drop reply_buffer_resize(), merge into reply_buffer_grow()
    - Remove "more" variables for now
    - Don't empty buffer before and after

v5 -> v6:
    - Edit commit message
    - Remove gratuitous blank line

programs/winedbg/gdbproxy.c | 271 +++++++++++++++++++-----------------
 1 file changed, 142 insertions(+), 129 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 2e7e04e6c93..8cc6fac65a6 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -87,6 +87,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 reply_buffer         qxfer_buffer;
 };

/* assume standard signal and errno values */
@@ -797,20 +798,20 @@ static inline BOOL is_gdb_special_char(unsigned char val)
     return gdb_special_chars_lookup_table[val % length] == val;
 }

-static void packet_reply_add(struct gdb_context* gdbctx, const char* str)
+static void packet_reply_add_data(struct gdb_context* gdbctx, const void* data, size_t len)
 {
-    const unsigned char *ptr = (unsigned char *)str, *curr;
+    const unsigned char *ptr = data, *end = ptr + len, *curr;
     unsigned char esc_seq[2];

-    while (*ptr)
+    while (ptr != end)
     {
         curr = ptr;

-        while (*ptr && !is_gdb_special_char(*ptr))
+        while (ptr != end && !is_gdb_special_char(*ptr))
             ptr++;

reply_buffer_append(&gdbctx->out_buf, curr, ptr - curr);
-        if (!*ptr) break;
+        if (ptr == end) break;

esc_seq[0] = 0x7D;
         esc_seq[1] = 0x20 ^ *ptr++;
@@ -818,6 +819,11 @@ static void packet_reply_add(struct gdb_context* gdbctx, const char* str)
     }
 }

+static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str)
+{
+    packet_reply_add_data(gdbctx, str, strlen(str));
+}
+
 static void packet_reply_open(struct gdb_context* gdbctx)
 {
     assert(gdbctx->out_curr_packet == -1);
@@ -837,37 +843,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_buf.len)
-    {
-        gdbctx->out_buf.len -= off;
-        memmove((unsigned char *)gdbctx->out_buf.base + begin,
-                (unsigned char *)gdbctx->out_buf.base + begin + off,
-                gdbctx->out_buf.len);
-    }
-    else
-    {
-        *((unsigned char *)gdbctx->out_buf.base + gdbctx->out_curr_packet) = 'l';
-        gdbctx->out_buf.len = gdbctx->out_curr_packet + 1;
-    }
-
-    plen = gdbctx->out_buf.len - begin;
-    if (plen > len) gdbctx->out_buf.len -= (plen - len);
-    else *((unsigned char *)gdbctx->out_buf.base + gdbctx->out_curr_packet) = 'l';
-
-    packet_reply_close(gdbctx);
-}
-
 static enum packet_return packet_reply(struct gdb_context* gdbctx, const char* packet)
 {
     packet_reply_open(gdbctx);
@@ -897,6 +872,29 @@ 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 nonempty;
+    size_t trunc_len;
+
+    packet_reply_open(gdbctx);
+
+    nonempty = (size_t)off < datalen;
+    if (nonempty && (size_t)off + len < datalen)
+        packet_reply_add(gdbctx, "m");
+    else
+        packet_reply_add(gdbctx, "l");
+
+    if (nonempty)
+    {
+        trunc_len = min((size_t)len, datalen - off);
+        packet_reply_add_data(gdbctx, (const unsigned char *)data + off, trunc_len);
+    }
+
+    packet_reply_close(gdbctx);
+}
+
 /* =============================================== *
  *          P A C K E T   H A N D L E R S          *
  * =============================================== *
@@ -1604,6 +1602,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 reply_buffer* reply = &gdbctx->qxfer_buffer;
     MEMORY_BASIC_INFORMATION mbi;
     IMAGE_SECTION_HEADER *sec;
     IMAGE_DOS_HEADER *dos = NULL;
@@ -1616,11 +1615,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="");
+    reply_buffer_append_str(reply, "<library name="");
     if (strcmp(mod.LoadedImageName, "[vdso].so") == 0)
-        packet_reply_add(gdbctx, "linux-vdso.so.1");
+        reply_buffer_append_str(reply, "linux-vdso.so.1");
     else if (mod.LoadedImageName[0] == '/')
-        packet_reply_add(gdbctx, mod.LoadedImageName);
+        reply_buffer_append_str(reply, mod.LoadedImageName);
     else
     {
         UNICODE_STRING nt_name;
@@ -1635,15 +1634,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);
+            reply_buffer_append_str(reply, unix_path);
         }
         else
-            packet_reply_add(gdbctx, mod.LoadedImageName);
+            reply_buffer_append_str(reply, mod.LoadedImageName);

HeapFree(GetProcessHeap(), 0, unix_path);
         RtlFreeUnicodeString(&nt_name);
     }
-    packet_reply_add(gdbctx, "">");
+    reply_buffer_append_str(reply, "">");

size = sizeof(buffer);
     if (VirtualQueryEx(gdbctx->process->handle, (void *)(UINT_PTR)mod.BaseOfImage, &mbi, sizeof(mbi)) >= sizeof(mbi) &&
@@ -1674,72 +1673,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, ""/>");
+        reply_buffer_append_str(reply, "<segment address="0x");
+        reply_buffer_append_uinthex(reply, mod.BaseOfImage + sec[i].VirtualAddress, sizeof(ULONG_PTR));
+        reply_buffer_append_str(reply, ""/>");
     }

-    packet_reply_add(gdbctx, "</library>");
+    reply_buffer_append_str(reply, "</library>");

return TRUE;
 }

static void packet_query_libraries(struct gdb_context* gdbctx)
 {
+    struct reply_buffer* reply = &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>");
+    reply_buffer_append_str(reply, "<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>");
+    reply_buffer_append_str(reply, "</library-list>");
 }

static void packet_query_threads(struct gdb_context* gdbctx)
 {
+    struct reply_buffer* reply = &gdbctx->qxfer_buffer;
     struct dbg_process* process = gdbctx->process;
     struct dbg_thread* thread;

-    packet_reply_add(gdbctx, "<threads>");
+    reply_buffer_append_str(reply, "<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, ""/>");
+        reply_buffer_append_str(reply, "<thread ");
+        reply_buffer_append_str(reply, "id="");
+        reply_buffer_append_uinthex(reply, thread->tid, 4);
+        reply_buffer_append_str(reply, "" name="");
+        reply_buffer_append_str(reply, thread->name);
+        reply_buffer_append_str(reply, ""/>");
     }
-    packet_reply_add(gdbctx, "</threads>");
+    reply_buffer_append_str(reply, "</threads>");
 }

static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_cpu* cpu)
 {
+    struct reply_buffer* reply = &gdbctx->qxfer_buffer;
     const char* feature_prefix = NULL;
     const char* feature = NULL;
     char buffer[256];
     int i;

-    packet_reply_add(gdbctx, "<target>");
+    reply_buffer_append_str(reply, "<target>");
     switch (cpu->machine)
     {
     case IMAGE_FILE_MACHINE_AMD64:
-        packet_reply_add(gdbctx, "<architecture>i386:x86-64</architecture>");
+        reply_buffer_append_str(reply, "<architecture>i386:x86-64</architecture>");
         feature_prefix = "org.gnu.gdb.i386.";
         break;
     case IMAGE_FILE_MACHINE_I386:
-        packet_reply_add(gdbctx, "<architecture>i386</architecture>");
+        reply_buffer_append_str(reply, "<architecture>i386</architecture>");
         feature_prefix = "org.gnu.gdb.i386.";
         break;
     case IMAGE_FILE_MACHINE_ARMNT:
-        packet_reply_add(gdbctx, "<architecture>arm</architecture>");
+        reply_buffer_append_str(reply, "<architecture>arm</architecture>");
         feature_prefix = "org.gnu.gdb.arm.";
         break;
     case IMAGE_FILE_MACHINE_ARM64:
-        packet_reply_add(gdbctx, "<architecture>aarch64</architecture>");
+        reply_buffer_append_str(reply, "<architecture>aarch64</architecture>");
         feature_prefix = "org.gnu.gdb.aarch64.";
         break;
     }
@@ -1748,87 +1750,87 @@ 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) reply_buffer_append_str(reply, "</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, "">");
+            reply_buffer_append_str(reply, "<feature name="");
+            if (feature_prefix) reply_buffer_append_str(reply, feature_prefix);
+            reply_buffer_append_str(reply, feature);
+            reply_buffer_append_str(reply, "">");

if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 &&
                 strcmp(feature, "core") == 0)
-                packet_reply_add(gdbctx, "<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"/>"
-                                         "<field name="AF" start="4" end="4"/>"
-                                         "<field name="ZF" start="6" end="6"/>"
-                                         "<field name="SF" start="7" end="7"/>"
-                                         "<field name="TF" start="8" end="8"/>"
-                                         "<field name="IF" start="9" end="9"/>"
-                                         "<field name="DF" start="10" end="10"/>"
-                                         "<field name="OF" start="11" end="11"/>"
-                                         "<field name="NT" start="14" end="14"/>"
-                                         "<field name="RF" start="16" end="16"/>"
-                                         "<field name="VM" start="17" end="17"/>"
-                                         "<field name="AC" start="18" end="18"/>"
-                                         "<field name="VIF" start="19" end="19"/>"
-                                         "<field name="VIP" start="20" end="20"/>"
-                                         "<field name="ID" start="21" end="21"/>"
-                                         "</flags>");
+                reply_buffer_append_str(reply, "<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"/>"
+                                               "<field name="AF" start="4" end="4"/>"
+                                               "<field name="ZF" start="6" end="6"/>"
+                                               "<field name="SF" start="7" end="7"/>"
+                                               "<field name="TF" start="8" end="8"/>"
+                                               "<field name="IF" start="9" end="9"/>"
+                                               "<field name="DF" start="10" end="10"/>"
+                                               "<field name="OF" start="11" end="11"/>"
+                                               "<field name="NT" start="14" end="14"/>"
+                                               "<field name="RF" start="16" end="16"/>"
+                                               "<field name="VM" start="17" end="17"/>"
+                                               "<field name="AC" start="18" end="18"/>"
+                                               "<field name="VIF" start="19" end="19"/>"
+                                               "<field name="VIP" start="20" end="20"/>"
+                                               "<field name="ID" start="21" end="21"/>"
+                                               "</flags>");

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"/>"
-                                         "<vector id="v2d" type="ieee_double" count="2"/>"
-                                         "<vector id="v16i8" type="int8" count="16"/>"
-                                         "<vector id="v8i16" type="int16" count="8"/>"
-                                         "<vector id="v4i32" type="int32" count="4"/>"
-                                         "<vector id="v2i64" type="int64" count="2"/>"
-                                         "<union id="vec128">"
-                                         "<field name="v4_float" type="v4f"/>"
-                                         "<field name="v2_double" type="v2d"/>"
-                                         "<field name="v16_int8" type="v16i8"/>"
-                                         "<field name="v8_int16" type="v8i16"/>"
-                                         "<field name="v4_int32" type="v4i32"/>"
-                                         "<field name="v2_int64" type="v2i64"/>"
-                                         "<field name="uint128" type="uint128"/>"
-                                         "</union>"
-                                         "<flags id="i386_mxcsr" size="4">"
-                                         "<field name="IE" start="0" end="0"/>"
-                                         "<field name="DE" start="1" end="1"/>"
-                                         "<field name="ZE" start="2" end="2"/>"
-                                         "<field name="OE" start="3" end="3"/>"
-                                         "<field name="UE" start="4" end="4"/>"
-                                         "<field name="PE" start="5" end="5"/>"
-                                         "<field name="DAZ" start="6" end="6"/>"
-                                         "<field name="IM" start="7" end="7"/>"
-                                         "<field name="DM" start="8" end="8"/>"
-                                         "<field name="ZM" start="9" end="9"/>"
-                                         "<field name="OM" start="10" end="10"/>"
-                                         "<field name="UM" start="11" end="11"/>"
-                                         "<field name="PM" start="12" end="12"/>"
-                                         "<field name="FZ" start="15" end="15"/>"
-                                         "</flags>");
+                reply_buffer_append_str(reply, "<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"/>"
+                                               "<vector id="v4i32" type="int32" count="4"/>"
+                                               "<vector id="v2i64" type="int64" count="2"/>"
+                                               "<union id="vec128">"
+                                               "<field name="v4_float" type="v4f"/>"
+                                               "<field name="v2_double" type="v2d"/>"
+                                               "<field name="v16_int8" type="v16i8"/>"
+                                               "<field name="v8_int16" type="v8i16"/>"
+                                               "<field name="v4_int32" type="v4i32"/>"
+                                               "<field name="v2_int64" type="v2i64"/>"
+                                               "<field name="uint128" type="uint128"/>"
+                                               "</union>"
+                                               "<flags id="i386_mxcsr" size="4">"
+                                               "<field name="IE" start="0" end="0"/>"
+                                               "<field name="DE" start="1" end="1"/>"
+                                               "<field name="ZE" start="2" end="2"/>"
+                                               "<field name="OE" start="3" end="3"/>"
+                                               "<field name="UE" start="4" end="4"/>"
+                                               "<field name="PE" start="5" end="5"/>"
+                                               "<field name="DAZ" start="6" end="6"/>"
+                                               "<field name="IM" start="7" end="7"/>"
+                                               "<field name="DM" start="8" end="8"/>"
+                                               "<field name="ZM" start="9" end="9"/>"
+                                               "<field name="OM" start="10" end="10"/>"
+                                               "<field name="UM" start="11" end="11"/>"
+                                               "<field name="PM" start="12" end="12"/>"
+                                               "<field name="FZ" start="15" end="15"/>"
+                                               "</flags>");
         }

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);
+        reply_buffer_append_str(reply, 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, """);
+            reply_buffer_append_str(reply, " type="");
+            reply_buffer_append_str(reply, cpu->gdb_register_map[i].type);
+            reply_buffer_append_str(reply, """);
         }

-        packet_reply_add(gdbctx, "/>");
+        reply_buffer_append_str(reply, "/>");
     }

-    if (feature) packet_reply_add(gdbctx, "</feature>");
-    packet_reply_add(gdbctx, "</target>");
+    if (feature) reply_buffer_append_str(reply, "</feature>");
+    reply_buffer_append_str(reply, "</target>");
 }

static enum packet_return packet_query(struct gdb_context* gdbctx)
@@ -1960,9 +1962,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
         {
             if (!gdbctx->process) return packet_error;

-            packet_reply_open_xfer(gdbctx);
             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);
+            reply_buffer_empty(&gdbctx->qxfer_buffer);
             return packet_done;
         }

@@ -1970,9 +1975,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
         {
             if (!gdbctx->process) return packet_error;

-            packet_reply_open_xfer(gdbctx);
             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);
+            reply_buffer_empty(&gdbctx->qxfer_buffer);
             return packet_done;
         }

@@ -1981,9 +1989,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
             if (!gdbctx->process) return packet_error;
             if (!(cpu = gdbctx->process->be_cpu)) return packet_error;

-            packet_reply_open_xfer(gdbctx);
             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);
+            reply_buffer_empty(&gdbctx->qxfer_buffer);
             return packet_done;
         }
         break;
@@ -2301,6 +2312,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))
     {

[PATCH v6 2/6] winedbg: Buffer output of GDB qXfer commands for proper slicing.

[Rémi Bernon]

This looks almost good too.

On 11/22/21 15:59, Jinoh Kang wrote:
> +static void packet_reply_xfer(struct gdb_context* gdbctx, const void *data, size_t datalen,
> -
>                          unsigned int off, unsigned int len)
>
> +{
> - BOOL nonempty;
> - size_t trunc_len;
> -
> - packet_reply_open(gdbctx);
> -
> - nonempty = (size_t)off < datalen;
> - if (nonempty && (size_t)off + len < datalen)
> -
>    packet_reply_add(gdbctx, "m");
> - else
> -
>    packet_reply_add(gdbctx, "l");
> -
> - if (nonempty)
> - {
> -
>    trunc_len = min((size_t)len, datalen - off);
> -
>    packet_reply_add_data(gdbctx, (const unsigned char *)data + off, trunc_len);
> - }
> -
> - packet_reply_close(gdbctx);
>
> +}
> -

With base an unsigned char* you could save a cast there too.

I also don't think you need to cast unsigned int to size_t, you could 
even make the off and len params, size_t directly.

(The local variables also are a bit unnecessary IMHO but probably a 
matter of taste)
> static enum packet_return packet_query(struct gdb_context* gdbctx)
> @@ -1960,9 +1962,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>           {
>               if (!gdbctx->process) return packet_error;
> -
>        packet_reply_open_xfer(gdbctx);
>         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);
> -
>        reply_buffer_empty(&gdbctx->qxfer_buffer);
>         return packet_done;
>     }
>
> @@ -1970,9 +1975,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>           {
>               if (!gdbctx->process) return packet_error;
> -
>        packet_reply_open_xfer(gdbctx);
>         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);
> -
>        reply_buffer_empty(&gdbctx->qxfer_buffer);
>         return packet_done;
>     }
>
> @@ -1981,9 +1989,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
>               if (!gdbctx->process) return packet_error;
>               if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
> -
>        packet_reply_open_xfer(gdbctx);
>         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);
> -
>        reply_buffer_empty(&gdbctx->qxfer_buffer);
>         return packet_done;
>     }
>     break;

Maybe you could directly use gdbctx->qxfer_buffer in packet_reply_xfer, 
instead of passing base and len here?

[PATCH v6 3/6] winedbg: Escape XML special characters in qXfer reply.

[Jinoh Kang]

Some dynamic strings (e.g. loaded image paths) may contain XML special
characters which breaks parsing.

Fix this by escaping all dynamic strings (i.e. character data and
attribute values) that go into the XML replies.

Signed-off-by: Jinoh Kang 
---

Notes:
    On XML special character escaping code:

strcspn() uses lookup tables as well, but as of Wine 6.22
    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.

v5 -> v6: Edit commit message

programs/winedbg/gdbproxy.c | 63 ++++++++++++++++++++++++++++++++-----
 1 file changed, 56 insertions(+), 7 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 8cc6fac65a6..025f8ed2ffc 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -279,6 +279,55 @@ static inline void reply_buffer_append_uinthex(struct reply_buffer* reply, ULONG
     reply_buffer_append(reply, 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)
+{
+    const size_t length = ARRAY_SIZE(xml_special_chars_lookup_table);
+    return xml_special_chars_lookup_table[val % length] == val;
+}
+
+static void reply_buffer_append_xmlstr(struct reply_buffer* reply, const char* str)
+{
+    const char *ptr = str, *curr;
+
+    for (;;)
+    {
+        curr = ptr;
+
+        while (!is_nul_or_xml_special_char((unsigned char)*ptr))
+            ptr++;
+
+        reply_buffer_append(reply, curr, ptr - curr);
+
+        switch (*ptr++)
+        {
+        case '"': reply_buffer_append_str(reply, "&quot;"); break;
+        case '&': reply_buffer_append_str(reply, "&amp;"); break;
+        case ''': reply_buffer_append_str(reply, "&apos;"); break;
+        case '<': reply_buffer_append_str(reply, "&lt;"); break;
+        case '>': reply_buffer_append_str(reply, "&gt;"); break;
+        case '\0':
+        default:
+            return;
+        }
+    }
+}
+
 static unsigned char checksum(const void* data, int len)
 {
     unsigned cksum = 0;
@@ -1617,9 +1666,9 @@ static BOOL CALLBACK packet_query_libraries_cb(PCSTR mod_name, DWORD64 base, PVO

reply_buffer_append_str(reply, "<library name="");
     if (strcmp(mod.LoadedImageName, "[vdso].so") == 0)
-        reply_buffer_append_str(reply, "linux-vdso.so.1");
+        reply_buffer_append_xmlstr(reply, "linux-vdso.so.1");
     else if (mod.LoadedImageName[0] == '/')
-        reply_buffer_append_str(reply, mod.LoadedImageName);
+        reply_buffer_append_xmlstr(reply, mod.LoadedImageName);
     else
     {
         UNICODE_STRING nt_name;
@@ -1634,10 +1683,10 @@ 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);
-            reply_buffer_append_str(reply, unix_path);
+            reply_buffer_append_xmlstr(reply, unix_path);
         }
         else
-            reply_buffer_append_str(reply, mod.LoadedImageName);
+            reply_buffer_append_xmlstr(reply, mod.LoadedImageName);

HeapFree(GetProcessHeap(), 0, unix_path);
         RtlFreeUnicodeString(&nt_name);
@@ -1754,8 +1803,8 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
             feature = cpu->gdb_register_map[i].feature;

reply_buffer_append_str(reply, "<feature name="");
-            if (feature_prefix) reply_buffer_append_str(reply, feature_prefix);
-            reply_buffer_append_str(reply, feature);
+            if (feature_prefix) reply_buffer_append_xmlstr(reply, feature_prefix);
+            reply_buffer_append_xmlstr(reply, feature);
             reply_buffer_append_str(reply, "">");

if (strcmp(feature_prefix, "org.gnu.gdb.i386.") == 0 &&
@@ -1822,7 +1871,7 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
         if (cpu->gdb_register_map[i].type)
         {
             reply_buffer_append_str(reply, " type="");
-            reply_buffer_append_str(reply, cpu->gdb_register_map[i].type);
+            reply_buffer_append_xmlstr(reply, cpu->gdb_register_map[i].type);
             reply_buffer_append_str(reply, """);
         }

[PATCH v6 3/6] winedbg: Escape XML special characters in qXfer reply.

[Rémi Bernon]

Looks good and the next ones too.

[PATCH v6 4/6] winedbg: Define table for GDB qXfer command handlers.

[Jinoh Kang]

Define a handler lookup table for qXfer commands and use it.

This facilitates implementing more qXfer commands and cacheing reply
data.

Signed-off-by: Jinoh Kang 
---

Notes:
    v3 -> v4:
    - Refactor the code a bit
    - Revert to scanf() when parsing Xfer packet lines
    - Fix pointer variable declaration style

v4 -> v5:
    - Remove "more" variables for now
    - Don't empty the reply buffer twice
    - Use two sscanf for alternative patterns
    - Don't always zero-initialise qxfer_object_annex
    - Use strcpy (I just did what I was told to do)
    - Use strcmp (I just did what I was told to do)

programs/winedbg/gdbproxy.c | 141 +++++++++++++++++++++++++-----------
 1 file changed, 100 insertions(+), 41 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 025f8ed2ffc..c9c7656f2a0 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -61,6 +61,9 @@ struct reply_buffer
     size_t alloc;
 };

+#define QX_NAME_SIZE 32
+#define QX_ANNEX_SIZE MAX_PATH
+
 struct gdb_context
 {
     /* gdb information */
@@ -87,6 +90,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[QX_ANNEX_SIZE];
     struct reply_buffer         qxfer_buffer;
 };

@@ -808,7 +813,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 packet_reply_hex_to(struct gdb_context* gdbctx, const void* src, int len)
 {
@@ -1732,11 +1737,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 reply_buffer* reply = &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);

@@ -1745,14 +1755,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);
     reply_buffer_append_str(reply, "</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 reply_buffer* reply = &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);
+
     reply_buffer_append_str(reply, "<threads>");
     LIST_FOR_EACH_ENTRY(thread, &process->threads, struct dbg_thread, entry)
     {
@@ -1764,11 +1781,12 @@ static void packet_query_threads(struct gdb_context* gdbctx)
         reply_buffer_append_str(reply, ""/>");
     }
     reply_buffer_append_str(reply, "</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 reply_buffer* reply, struct backend_cpu* cpu)
 {
-    struct reply_buffer* reply = &gdbctx->qxfer_buffer;
     const char* feature_prefix = NULL;
     const char* feature = NULL;
     char buffer[256];
@@ -1882,10 +1900,41 @@ static void packet_query_target_xml(struct gdb_context* gdbctx, struct backend_c
     reply_buffer_append_str(reply, "</target>");
 }

+static enum packet_return packet_query_features(struct gdb_context* gdbctx)
+{
+    struct reply_buffer* reply = &gdbctx->qxfer_buffer;
+    struct dbg_process* process = gdbctx->process;
+
+    if (!process) return packet_error;
+
+    if (strncmp(gdbctx->qxfer_object_annex, "target.xml", QX_ANNEX_SIZE) == 0)
+    {
+        struct backend_cpu *cpu = process->be_cpu;
+        if (!cpu) return packet_error;
+
+        packet_query_target_xml(gdbctx, reply, 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 enum packet_return packet_query(struct gdb_context* gdbctx)
 {
+    char object_name[QX_NAME_SIZE], annex[QX_ANNEX_SIZE];
     unsigned int off, len;
-    struct backend_cpu *cpu;

switch (gdbctx->in_packet[0])
     {
@@ -1972,11 +2021,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;
         }
@@ -2007,44 +2061,47 @@ 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)
+        annex[0] = '\0';
+        if (sscanf(gdbctx->in_packet, "Xfer:%31[^:]:read::%x,%x", object_name, &off, &len) == 3 ||
+            sscanf(gdbctx->in_packet, "Xfer:%31[^:]:read:%255[^:]:%x,%x", object_name, annex, &off, &len) == 4)
         {
-            if (!gdbctx->process) return packet_error;
+            enum packet_return result;
+            int i;

-            packet_query_libraries(gdbctx);
-            packet_reply_xfer(gdbctx,
-                              gdbctx->qxfer_buffer.base,
-                              gdbctx->qxfer_buffer.len,
-                              off, len);
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
-            return packet_done;
-        }
+            for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
+            {
+                if (strcmp(qxfer_handlers[i].name, object_name) == 0)
+                    break;
+            }

-        if (sscanf(gdbctx->in_packet, "Xfer:threads:read::%x,%x", &off, &len) == 2)
-        {
-            if (!gdbctx->process) return packet_error;
+            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;
+            }

-            packet_query_threads(gdbctx);
-            packet_reply_xfer(gdbctx,
-                              gdbctx->qxfer_buffer.base,
-                              gdbctx->qxfer_buffer.len,
-                              off, len);
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
-            return packet_done;
-        }
+            TRACE("qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);

-        if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
-        {
-            if (!gdbctx->process) return packet_error;
-            if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
-
-            packet_query_target_xml(gdbctx, cpu);
-            packet_reply_xfer(gdbctx,
-                              gdbctx->qxfer_buffer.base,
-                              gdbctx->qxfer_buffer.len,
-                              off, len);
+            gdbctx->qxfer_object_idx = i;
+            strcpy(gdbctx->qxfer_object_annex, annex);
+
+            result = (*qxfer_handlers[i].handler)(gdbctx);
+            TRACE("qXfer read result = %d\n", result);
+
+            if ((result & ~packet_last_f) == packet_send_buffer)
+            {
+                packet_reply_xfer(gdbctx,
+                                  gdbctx->qxfer_buffer.base,
+                                  gdbctx->qxfer_buffer.len,
+                                  off, len);
+                result = (result & packet_last_f) | packet_done;
+            }
+
+            gdbctx->qxfer_object_idx = -1;
+            gdbctx->qxfer_object_annex[0] = '\0';
             reply_buffer_empty(&gdbctx->qxfer_buffer);
-            return packet_done;
+
+            return result;
         }
         break;
     }
@@ -2361,6 +2418,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 */

[PATCH v6 5/6] 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 
---

Notes:
    v3 -> v4:
    - Refactor the code a bit
    - Revert to scanf() when parsing Xfer packet lines
    - Fix pointer variable declaration style

v3 -> v4: Synchronize to prior modified patches.

v4 -> v5:
    - Synchronize to prior modified patches.
    - Fix pointer variable declaration style.

programs/winedbg/gdbproxy.c | 43 +++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index c9c7656f2a0..bdbfbe029f8 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -927,15 +927,16 @@ static inline void packet_reply_register_hex_to(struct gdb_context* gdbctx, dbg_
 }

static void packet_reply_xfer(struct gdb_context* gdbctx, const void *data, size_t datalen,
-                              unsigned int off, unsigned int len)
+                              unsigned int off, unsigned int len, BOOL* more_p)
 {
-    BOOL nonempty;
+    BOOL nonempty, more;
     size_t trunc_len;

packet_reply_open(gdbctx);

nonempty = (size_t)off < datalen;
-    if (nonempty && (size_t)off + len < datalen)
+    more = nonempty && (size_t)off + len < datalen;
+    if (more)
         packet_reply_add(gdbctx, "m");
     else
         packet_reply_add(gdbctx, "l");
@@ -947,6 +948,8 @@ static void packet_reply_xfer(struct gdb_context* gdbctx, const void *data, size
     }

packet_reply_close(gdbctx);
+
+    *more_p = more;
 }

/* =============================================== *
@@ -2067,6 +2070,7 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
         {
             enum packet_return result;
             int i;
+            BOOL more;

for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
             {
@@ -2082,24 +2086,41 @@ 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);

-            gdbctx->qxfer_object_idx = i;
-            strcpy(gdbctx->qxfer_object_annex, annex);
+            if (off > 0 &&
+                gdbctx->qxfer_buffer.len > 0 &&
+                gdbctx->qxfer_object_idx == i &&
+                strcmp(gdbctx->qxfer_object_annex, annex) == 0)
+            {
+                result = packet_send_buffer;
+                TRACE("qXfer read result = %d (cached)\n", result);
+            }
+            else
+            {
+                reply_buffer_empty(&gdbctx->qxfer_buffer);

-            result = (*qxfer_handlers[i].handler)(gdbctx);
-            TRACE("qXfer read result = %d\n", result);
+                gdbctx->qxfer_object_idx = i;
+                strcpy(gdbctx->qxfer_object_annex, annex);

+                result = (*qxfer_handlers[i].handler)(gdbctx);
+                TRACE("qXfer read result = %d\n", result);
+            }
+
+            more = FALSE;
             if ((result & ~packet_last_f) == packet_send_buffer)
             {
                 packet_reply_xfer(gdbctx,
                                   gdbctx->qxfer_buffer.base,
                                   gdbctx->qxfer_buffer.len,
-                                  off, len);
+                                  off, len, &more);
                 result = (result & packet_last_f) | packet_done;
             }

-            gdbctx->qxfer_object_idx = -1;
-            gdbctx->qxfer_object_annex[0] = '\0';
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
+            if (!more)
+            {
+                gdbctx->qxfer_object_idx = -1;
+                gdbctx->qxfer_object_annex[0] = '\0';
+                reply_buffer_empty(&gdbctx->qxfer_buffer);
+            }

return result;
         }

[PATCH v6 6/6] winedbg: Implement GDB qXfer object exec-file.

[Jinoh Kang]

Today, when gdbproxy is started with --no-start mode, GDB fails to
recognise the symbol file unless the `file` command or the
`sharedlibrary` command is explicitly issued.

Also, 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)

Fix this by implementing the qXfer object "exec-file".

Signed-off-by: Jinoh Kang 
---

Notes:
    v3 -> v4: Fix pointer variable declaration style
    v4 -> v5: (no changes)
    v5 -> v6: Edit commit message

programs/winedbg/gdbproxy.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index bdbfbe029f8..a09fc4dbe3e 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -98,7 +98,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,
 };
@@ -1923,6 +1926,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 reply_buffer* reply = &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);
+
+    reply_buffer_append_str(reply, unix_path);
+
+    HeapFree(GetProcessHeap(), 0, unix_path);
+
+    return packet_send_buffer;
+}
+
 struct qxfer
 {
     const char*        name;
@@ -1932,6 +1962,7 @@ struct qxfer
     {"libraries", packet_query_libraries},
     {"threads"  , packet_query_threads  },
     {"features" , packet_query_features },
+    {"exec-file", packet_query_exec_file},
 };

static enum packet_return packet_query(struct gdb_context* gdbctx)