[PATCH v4 0/7] 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.

Jinoh Kang (7):
  winedbg: Replace packet_realloc() with 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 | 621 ++++++++++++++++++++++++++----------
 1 file changed, 456 insertions(+), 165 deletions(-)

-- 
2.31.1

[PATCH v4 1/7] winedbg: Replace packet_realloc() with realloc().

[Jinoh Kang]

winedbg is now built with msvcrt, so just use realloc() directly
instead of Win32 heap functions.

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---
 programs/winedbg/gdbproxy.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index cd736532dcf..d3055340ead 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -705,20 +705,12 @@ 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)
-{
-    if (!buf)
-        return HeapAlloc(GetProcessHeap(), 0, size);
-    return HeapReAlloc(GetProcessHeap(), 0, buf, size);
-
-}
-
 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 = realloc(gdbctx->out_buf, gdbctx->out_buf_alloc);
     }
 }
 
@@ -2096,7 +2088,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 = 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 v4 1/7] winedbg: Replace packet_realloc() with realloc().

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 2/7] 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 d3055340ead..727b24c9adf 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -772,7 +772,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;
@@ -789,7 +789,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);
@@ -1762,7 +1762,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 v4 2/7] winedbg: Use unsigned int for offset/length in GDB qXfer handler.

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 3/7] 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>
---

Notes:
    v3 -> v4:
    - Rewrite packet_reply_add for clarity.
    - Fix pointer variable declaration style.
    - Update Wine version in comment.

 programs/winedbg/gdbproxy.c | 60 +++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 727b24c9adf..69fcbdb51d9 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -739,18 +739,61 @@ static void packet_reply_val(struct gdb_context* gdbctx, ULONG_PTR val, int len)
     }
 }
 
-static inline void packet_reply_add(struct gdb_context* gdbctx, const char* str)
+static const unsigned char gdb_special_chars_lookup_table[4] = {
+    /* The characters should be indexed 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)
 {
-    int len = strlen(str);
-    packet_reply_grow(gdbctx, len);
-    memcpy(&gdbctx->out_buf[gdbctx->out_len], str, len);
-    gdbctx->out_len += len;
+    /* 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.21
+     * 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(struct gdb_context* gdbctx, const char* str)
+{
+    const unsigned char *ptr = (unsigned char *)str, *curr;
+
+    while (*ptr)
+    {
+        curr = ptr;
+
+        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;
+        if (!*ptr) break;
+
+        packet_reply_grow(gdbctx, 2);
+        gdbctx->out_buf[gdbctx->out_len++] = 0x7D;
+        gdbctx->out_buf[gdbctx->out_len++] = 0x20 ^ *ptr++;
+    }
 }
 
 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;
 }
 
@@ -760,7 +803,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;
@@ -799,8 +843,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 v4 3/7] winedbg: Escape special characters in GDB packet reply.

[Rémi Bernon]

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
---

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

 programs/winedbg/gdbproxy.c | 388 ++++++++++++++++++++++++------------
 1 file changed, 262 insertions(+), 126 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 69fcbdb51d9..4ed99c4c06c 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 */
@@ -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 reply_buffer         qxfer_buffer;
 };
 
 /* assume standard signal and errno values */
@@ -224,6 +232,107 @@ static void hex_to(char* dst, const void* src, size_t len)
     }
 }
 
+static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
+{
+    reply->alloc = alloc;
+    reply->base = realloc(reply->base, reply->alloc);
+}
+
+static void reply_buffer_empty(struct reply_buffer* reply)
+{
+    reply->len = 0;
+    reply_buffer_resize(reply, 0);
+}
+
+static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
+{
+    if (reply->alloc < reply->len + size)
+        reply_buffer_resize(reply, ((reply->len + size) / 32 + 1) * 32);
+}
+
+static void reply_buffer_append(struct reply_buffer* reply, const void* data, size_t size)
+{
+    reply_buffer_grow(reply, size);
+    memcpy((void *)((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_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 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.21
+     * 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 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, """); break;
+        case '&': reply_buffer_append_str(reply, "&"); break;
+        case '\'': reply_buffer_append_str(reply, "'"); break;
+        case '<': reply_buffer_append_str(reply, "<"); break;
+        case '>': reply_buffer_append_str(reply, ">"); break;
+        case '\0':
+        default:
+            return;
+        }
+    }
+}
+
 static unsigned char checksum(const char* ptr, int len)
 {
     unsigned cksum = 0;
@@ -767,21 +876,21 @@ 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;
 
-    while (*ptr)
+    while (ptr != end)
     {
         curr = ptr;
 
-        while (*ptr && !is_gdb_special_char(*ptr))
+        while (ptr != end && !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;
-        if (!*ptr) break;
+        if (ptr == end) break;
 
         packet_reply_grow(gdbctx, 2);
         gdbctx->out_buf[gdbctx->out_len++] = 0x7D;
@@ -789,6 +898,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);
@@ -810,34 +924,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)
 {
@@ -868,6 +954,32 @@ 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_data(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          *
  * =============================================== *
@@ -1575,6 +1687,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;
@@ -1587,11 +1700,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_xmlstr(reply, "linux-vdso.so.1");
     else if (mod.LoadedImageName[0] == '/')
-        packet_reply_add(gdbctx, mod.LoadedImageName);
+        reply_buffer_append_xmlstr(reply, mod.LoadedImageName);
     else
     {
         UNICODE_STRING nt_name;
@@ -1606,15 +1719,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_xmlstr(reply, unix_path);
         }
         else
-            packet_reply_add(gdbctx, mod.LoadedImageName);
+            reply_buffer_append_xmlstr(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) &&
@@ -1645,72 +1758,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;
     }
@@ -1719,87 +1835,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)
@@ -1929,32 +2045,50 @@ 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);
+            reply_buffer_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);
+            reply_buffer_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);
+            reply_buffer_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);
+            reply_buffer_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);
+            reply_buffer_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);
+            reply_buffer_empty(&gdbctx->qxfer_buffer);
             return packet_done;
         }
         break;
@@ -2274,6 +2408,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 v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

Hi Jinoh!

A few comments:

On 11/19/21 14:41, Jinoh Kang wrote:
> Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> ---

Overall I'd say this one is probably worth splitting. Maybe consider a 
bit of prior refactoring to use the same "reply_buffer" both for the 
packet output buffer and the qxfer buffer?

At least it'd be nice to have the escaping done separately from the 
qxfer buffer addition, if possible.
> +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
> +{
> +    reply->alloc = alloc;
> +    reply->base = realloc(reply->base, reply->alloc);
> +}
> +
> +static void reply_buffer_empty(struct reply_buffer* reply)
> +{
> +    reply->len = 0;
> +    reply_buffer_resize(reply, 0);
> +}
> +

Do we need to realloc when emptying? I think the less alloc call we do 
the better. I don't think we should worry too much about memory 
consumption here?
> +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
> +{
> +    if (reply->alloc < reply->len + size)
> +        reply_buffer_resize(reply, ((reply->len + size) / 32 + 1) * 32);
> +}
> +

Maybe just one grow helper is enough and we can drop the resize? 
Otherwise I'd also expect it to work the other way, where resize only 
realloc when needed, calling grow.

[...]
> static enum packet_return packet_query(struct gdb_context* gdbctx)
> @@ -1929,32 +2045,50 @@ 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);
> +            reply_buffer_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);
> +            reply_buffer_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);
> +            reply_buffer_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);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               return packet_done;
>           }
>
> if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
>           {
> +            BOOL more;
> +

I think these more variables are not even used at this point, you could 
probably only add them later.
> if (!gdbctx->process) return packet_error;
>               if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
>
> -            packet_reply_open_xfer(gdbctx);
> +            reply_buffer_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);
> +            reply_buffer_empty(&gdbctx->qxfer_buffer);
>               return packet_done;
>           }
>           break;

Do we really need to empty the buffer before and after?

Cheers,
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

On 11/19/21 23:54, Rémi Bernon wrote:
> Hi Jinoh!
>
> A few comments:
>
> On 11/19/21 14:41, Jinoh Kang wrote:
> > Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> > ---
>
> Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?

De-duplicating the buffer management code sounds like a good idea.

My intention was to minimise touching existing code and do refactoring in
a separate patch serie.  Turns out this extra step is unnecessary, I guess.
> At least it'd be nice to have the escaping done separately from the qxfer buffer addition, if possible.

I agree.  In this case I would say that the qXfer buffer addition logically
precedes the XML escaping.  The reason is that the qXfer buffer is required to
correctly calculate the offset and length of the reply data slice that should
be transmitted to the debugger client.

Currently gdbproxy reuses the same buffer for both qXfer reply and actual GDB
packet reply.  This works well since a character in the qXfer reply buffer
matches 1:1 to the actual GDB reply packet.  However, if special characters are
escaped, this property will no longer hold 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 from working correctly.

I'll copy the above in the commit message.
> >   +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
> > +{
> > +    reply->alloc = alloc;
> > +    reply->base = realloc(reply->base, reply->alloc);
> > +}
> > +
> > +static void reply_buffer_empty(struct reply_buffer* reply)
> > +{
> > +    reply->len = 0;
> > +    reply_buffer_resize(reply, 0);
> > +}
> > +
>
> Do we need to realloc when emptying? I think the less alloc call we do the better.

I just noticed that gdbproxy does not free the old packet reply after transmission.
There's no reason reply_buffer shall deviate from this behaviour.  Thanks!
> I don't think we should worry too much about memory consumption here?

In fact, if memory consumption isn't a concern, why don't we straight out just
pre-allocate buffers in fixed sizes?  This way we don't have to worry about
realloc() entirely (we would still need to look out for overflows though).
This is also what some GDB stubs (e.g. the Linux kernel) does.
Perhaps fifteen pages followed by a guard to catch overlooked memory corruption
issues would suffice?
> > +static void reply_buffer_grow(struct reply_buffer* reply, size_t size)
> > +{
> > +    if (reply->alloc < reply->len + size)
> > +        reply_buffer_resize(reply, ((reply->len + size) / 32 + 1) * 32);
> > +}
> > +
>
> Maybe just one grow helper is enough and we can drop the resize? Otherwise I'd also expect it to work the other way, where resize only realloc when needed, calling grow.

Yes, helper merging will also cover this case.
> [...]
> >   static enum packet_return packet_query(struct gdb_context* gdbctx)
> > @@ -1929,32 +2045,50 @@ 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);
> > +            reply_buffer_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);
> > +            reply_buffer_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);
> > +            reply_buffer_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);
> > +            reply_buffer_empty(&gdbctx->qxfer_buffer);
> >               return packet_done;
> >           }
> >             if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
> >           {
> > +            BOOL more;
> > +
>
> I think these more variables are not even used at this point, you could probably only add them later.

Ack.
> >               if (!gdbctx->process) return packet_error;
> >               if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
> >   -            packet_reply_open_xfer(gdbctx);
> > +            reply_buffer_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);
> > +            reply_buffer_empty(&gdbctx->qxfer_buffer);
> >               return packet_done;
> >           }
> >           break;
>
> Do we really need to empty the buffer before and after?

Since we haven't implemented the cache here, not really.
> Cheers,

-- 
Sincerely,
Jinoh Kang

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

On 11/19/21 17:33, Jinoh Kang wrote:
> On 11/19/21 23:54, Rémi Bernon wrote:
> > Hi Jinoh!
> >
> > A few comments:
> >
> > On 11/19/21 14:41, Jinoh Kang wrote:
> > > Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> > > ---
> >
> > Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?
>
> De-duplicating the buffer management code sounds like a good idea.
>
> My intention was to minimise touching existing code and do refactoring in
> a separate patch serie.  Turns out this extra step is unnecessary, I guess.

I think winegdb in general and gdbproxy in particular aren't used very 
much and definitely need more love. Hence I'd not be too worried about 
changing the code, I don't think anything actually depends on it, and 
improvements are probably welcome.
> > At least it'd be nice to have the escaping done separately from the qxfer buffer addition, if possible.
>
> I agree.  In this case I would say that the qXfer buffer addition logically
> precedes the XML escaping.  The reason is that the qXfer buffer is required to
> correctly calculate the offset and length of the reply data slice that should
> be transmitted to the debugger client.
>
> Currently gdbproxy reuses the same buffer for both qXfer reply and actual GDB
> packet reply.  This works well since a character in the qXfer reply buffer
> matches 1:1 to the actual GDB reply packet.  However, if special characters are
> escaped, this property will no longer hold 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 from working correctly.
>
> I'll copy the above in the commit message.

Sounds good.
> > >   +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
> > > +{
> > > +    reply->alloc = alloc;
> > > +    reply->base = realloc(reply->base, reply->alloc);
> > > +}
> > > +
> > > +static void reply_buffer_empty(struct reply_buffer* reply)
> > > +{
> > > +    reply->len = 0;
> > > +    reply_buffer_resize(reply, 0);
> > > +}
> > > +
> >
> > Do we need to realloc when emptying? I think the less alloc call we do the better.
>
> I just noticed that gdbproxy does not free the old packet reply after transmission.
> There's no reason reply_buffer shall deviate from this behaviour.  Thanks!

Yes, I think it's just cleaned up on exit, there's no real use to do the 
cleanup properly here.
> > I don't think we should worry too much about memory consumption here?
>
> In fact, if memory consumption isn't a concern, why don't we straight out just
> pre-allocate buffers in fixed sizes?  This way we don't have to worry about
> realloc() entirely (we would still need to look out for overflows though).
> This is also what some GDB stubs (e.g. the Linux kernel) does.
> Perhaps fifteen pages followed by a guard to catch overlooked memory corruption
> issues would suffice?

I guess we could, or at least allocate a bigger buffer and reallocate it 
by *3/2 steps when needed. I'm not completely sure why I only added 
small increments, maybe it was already like this before.

It could be static too, of course if we can assume that gdb will only 
ever ask limited size chunks (even if you try to dump large portion of 
memory for instance).

-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

On 11/20/21 01:42, Rémi Bernon wrote:
> On 11/19/21 17:33, Jinoh Kang wrote:
> > On 11/19/21 23:54, Rémi Bernon wrote:
> > > Hi Jinoh!
> > >
> > > A few comments:
> > >
> > > On 11/19/21 14:41, Jinoh Kang wrote:
> > > > Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> > > > ---
> > >
> > > Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?
> >
> > De-duplicating the buffer management code sounds like a good idea.
> >
> > My intention was to minimise touching existing code and do refactoring in
> > a separate patch serie.  Turns out this extra step is unnecessary, I guess.
>
> I think winegdb in general and gdbproxy in particular aren't used very much

Wait, there were any alternatives?  Bare GDB was always cumbersome to me, since
it never detects all the DLL images automatically.

Maybe I should take a look at MinGW build of gdbserver.
> and definitely need more love.

Especially since its use is infrequent, test coverage should be what needs most
improvement.  We can even get free conformance tests if we get rid of all
Wine-dependent code from WineDbg (sounds weird, I know).

(The major part of Wine dependence is the Unix path resolution.  Removing this
 would mean that gdbproxy shall implement the file I/O protocol, so that
 GDB can read files from Windows paths.  Sounds like a lot of work, though.)
> Hence I'd not be too worried about changing the code, I don't think anything actually depends on it, and improvements are probably welcome.
> > > At least it'd be nice to have the escaping done separately from the qxfer buffer addition, if possible.
> >
> > I agree.  In this case I would say that the qXfer buffer addition logically
> > precedes the XML escaping.  The reason is that the qXfer buffer is required to
> > correctly calculate the offset and length of the reply data slice that should
> > be transmitted to the debugger client.
> >
> > Currently gdbproxy reuses the same buffer for both qXfer reply and actual GDB
> > packet reply.  This works well since a character in the qXfer reply buffer
> > matches 1:1 to the actual GDB reply packet.  However, if special characters are
> > escaped, this property will no longer hold 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 from working correctly.
> >
> > I'll copy the above in the commit message.
>
> Sounds good.
> > > >    +static void reply_buffer_resize(struct reply_buffer* reply, size_t alloc)
> > > > +{
> > > > +    reply->alloc = alloc;
> > > > +    reply->base = realloc(reply->base, reply->alloc);
> > > > +}
> > > > +
> > > > +static void reply_buffer_empty(struct reply_buffer* reply)
> > > > +{
> > > > +    reply->len = 0;
> > > > +    reply_buffer_resize(reply, 0);
> > > > +}
> > > > +
> > >
> > > Do we need to realloc when emptying? I think the less alloc call we do the better.
> >
> > I just noticed that gdbproxy does not free the old packet reply after transmission.
> > There's no reason reply_buffer shall deviate from this behaviour.  Thanks!
>
> Yes, I think it's just cleaned up on exit, there's no real use to do the cleanup properly here.
> > > I don't think we should worry too much about memory consumption here?
> >
> > In fact, if memory consumption isn't a concern, why don't we straight out just
> > pre-allocate buffers in fixed sizes?  This way we don't have to worry about
> > realloc() entirely (we would still need to look out for overflows though).
> > This is also what some GDB stubs (e.g. the Linux kernel) does.
> > Perhaps fifteen pages followed by a guard to catch overlooked memory corruption
> > issues would suffice?
>
> I guess we could, or at least allocate a bigger buffer and reallocate it by *3/2 steps when needed. I'm not completely sure why I only added small increments, maybe it was already like this before.

Good.
> It could be static too, of course if we can assume that gdb will only ever ask limited size chunks (even if you try to dump large portion of memory for instance).

GDB seems to do just that.  For LLDB (which uses a compatible, albeit subtly different protocol), I believe it doesn't.
>

-- 
Sincerely,
Jinoh Kang

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Eric Pouech]

> (The major part of Wine dependence is the Unix path resolution.  Removing this
>   would mean that gdbproxy shall implement the file I/O protocol, so that
>   GDB can read files from Windows paths.  Sounds like a lot of work, though.)

you'd better use the PE port of gdb for that

(but last time I checked it didn't work in wine; didn't investigate though)

A+

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Rémi Bernon]

On 11/19/21 18:05, Jinoh Kang wrote:
> On 11/20/21 01:42, Rémi Bernon wrote:
> > On 11/19/21 17:33, Jinoh Kang wrote:
> > > On 11/19/21 23:54, Rémi Bernon wrote:
> > > > Hi Jinoh!
> > > >
> > > > A few comments:
> > > >
> > > > On 11/19/21 14:41, Jinoh Kang wrote:
> > > > > Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> > > > > ---
> > > >
> > > > Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?
> > >
> > > De-duplicating the buffer management code sounds like a good idea.
> > >
> > > My intention was to minimise touching existing code and do refactoring in
> > > a separate patch serie.  Turns out this extra step is unnecessary, I guess.
> >
> > I think winegdb in general and gdbproxy in particular aren't used very much
>
> Wait, there were any alternatives?  Bare GDB was always cumbersome to me, since
> it never detects all the DLL images automatically.
>
> Maybe I should take a look at MinGW build of gdbserver.

Maybe I shouldn't say anything if you're only interested in fixing it 
because it doesn't work ;)

But I've personally moved to using GDB directly, with a python script to 
generate load-symbol-file commands from /proc/self/maps [1], which I 
find more usable.
> > and definitely need more love.
>
> Especially since its use is infrequent, test coverage should be what needs most
> improvement.  We can even get free conformance tests if we get rid of all
> Wine-dependent code from WineDbg (sounds weird, I know).
>
> (The major part of Wine dependence is the Unix path resolution.  Removing this
>   would mean that gdbproxy shall implement the file I/O protocol, so that
>   GDB can read files from Windows paths.  Sounds like a lot of work, though.)

Although tests could be interesting to have I don't think we want to get 
rid of the non-GDB code, it still has its use. Making it better could be 
nice though.

[1] https://www.winehq.org/pipermail/wine-devel/2021-June/189134.html
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Jinoh Kang]

On 11/20/21 03:09, Rémi Bernon wrote:
> On 11/19/21 18:05, Jinoh Kang wrote:
> > On 11/20/21 01:42, Rémi Bernon wrote:
> > > On 11/19/21 17:33, Jinoh Kang wrote:
> > > > On 11/19/21 23:54, Rémi Bernon wrote:
> > > > > Hi Jinoh!
> > > > >
> > > > > A few comments:
> > > > >
> > > > > On 11/19/21 14:41, Jinoh Kang wrote:
> > > > > > Signed-off-by: Jinoh Kang <jinoh.kang.kr(a)gmail.com>
> > > > > > ---
> > > > >
> > > > > Overall I'd say this one is probably worth splitting. Maybe consider a bit of prior refactoring to use the same "reply_buffer" both for the packet output buffer and the qxfer buffer?
> > > >
> > > > De-duplicating the buffer management code sounds like a good idea.
> > > >
> > > > My intention was to minimise touching existing code and do refactoring in
> > > > a separate patch serie.  Turns out this extra step is unnecessary, I guess.
> > >
> > > I think winegdb in general and gdbproxy in particular aren't used very much
> >
> > Wait, there were any alternatives?  Bare GDB was always cumbersome to me, since
> > it never detects all the DLL images automatically.
> >
> > Maybe I should take a look at MinGW build of gdbserver.
>
> Maybe I shouldn't say anything if you're only interested in fixing it because it doesn't work ;)

Why fix a working program?  "if it works, don't fix it."  ;) ;)

Perhaps you meant I wouldn't have worked on it had I known alternatives in the
first place.  But I didn't know them.  So I fixed it.  Knowing other ways around
now doesn't change something that already happened.  :p

Actually the fixes have been hanging around for months now.  I could've kept it;
Winedbg doesn't really get very frequent updates anyway.  But I decided that
they are better off on the list.  So here we are.
> But I've personally moved to using GDB directly, with a python script to generate load-symbol-file commands from /proc/self/maps [1], which I find more usable.

Having both PE and ELF symbols was exactly what I needed.  In fact I've wrote a
script that does a similar thing.  With plain GDB though, I never got "handle
SIGxxx pass" to work, so there's that.
> > > and definitely need more love.
> >
> > Especially since its use is infrequent, test coverage should be what needs most
> > improvement.  We can even get free conformance tests if we get rid of all
> > Wine-dependent code from WineDbg (sounds weird, I know).
> >
> > (The major part of Wine dependence is the Unix path resolution.  Removing this
> >   would mean that gdbproxy shall implement the file I/O protocol, so that
> >   GDB can read files from Windows paths.  Sounds like a lot of work, though.)
>
> Although tests could be interesting to have I don't think we want to get rid of the non-GDB code, it still has its use. Making it better could be nice though.

The Unix path resolution part only resides in gdbproxy.  There's no need to touch
non-GDB code at all.
> [1] https://www.winehq.org/pipermail/wine-devel/2021-June/189134.html

-- 
Sincerely,
Jinoh Kang

[PATCH v4 4/7] winedbg: Buffer and escape output of GDB qXfer commands.

[Eric Pouech]

> It could be static too, of course if we can assume that gdb will only 
> ever ask limited size chunks (even if you try to dump large portion of 
> memory for instance).

I don't like the static storage... all the rest is attached to the 
process instance

A+

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

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

 programs/winedbg/gdbproxy.c | 167 ++++++++++++++++++++++++++----------
 1 file changed, 121 insertions(+), 46 deletions(-)

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 4ed99c4c06c..ace3c71492c 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -61,6 +61,14 @@ struct reply_buffer
     size_t alloc;
 };
 
+#define STRINGIFY2(x) #x
+#define STRINGIFY(x) STRINGIFY2(x)
+
+#define MAX_QX_NAME_LEN 31
+#define MAX_QX_ANNEX_LEN 31
+#define QX_NAME_SIZE (MAX_QX_NAME_LEN+1)
+#define QX_ANNEX_SIZE (MAX_QX_ANNEX_LEN+1)
+
 struct gdb_context
 {
     /* gdb information */
@@ -89,6 +97,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;
 };
 
@@ -812,7 +822,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_grow(struct gdb_context* gdbctx, size_t size)
 {
@@ -1768,11 +1778,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);
 
@@ -1781,14 +1796,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)
     {
@@ -1800,11 +1822,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];
@@ -1918,10 +1941,61 @@ 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 BOOL parse_xfer_read(const char* ptr, char* name_buf, char* annex_buf, unsigned int* offp, unsigned int* lenp)
+{
+    int n;
+
+    /* Xfer:<object>:read:<annex>:<offset-hex>,<length-hex> */
+
+    if (sscanf(ptr, "Xfer:%" STRINGIFY(MAX_QX_NAME_LEN) "[^:]:read:%n", name_buf, &n) != 1)
+        return FALSE;
+    ptr += (unsigned int)n;
+
+    if (sscanf(ptr, "%" STRINGIFY(MAX_QX_ANNEX_LEN) "[^:]%n", annex_buf, &n) != 1)
+    {
+        annex_buf[0] = '\0';
+        n = 0;
+    }
+    ptr += (unsigned int)n;
+
+    return sscanf(ptr, ":%x,%x", offp, lenp) == 2;
+}
+
 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])
     {
@@ -2008,11 +2082,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;
         }
@@ -2043,53 +2122,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)
+        if (parse_xfer_read(gdbctx->in_packet, object_name, annex, &off, &len))
         {
+            enum packet_return result;
+            int i;
             BOOL more;
 
-            if (!gdbctx->process) return packet_error;
+            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 (strncmp(qxfer_handlers[i].name, object_name, QX_NAME_SIZE) == 0)
+                    break;
+            }
 
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
-            packet_query_libraries(gdbctx);
-            packet_reply_xfer(gdbctx,
-                              gdbctx->qxfer_buffer.base,
-                              gdbctx->qxfer_buffer.len,
-                              off, len, &more);
-            reply_buffer_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;
+            TRACE("qXfer %s read %s %u,%u\n", debugstr_a(object_name), debugstr_a(annex), off, len);
 
             reply_buffer_empty(&gdbctx->qxfer_buffer);
-            packet_query_threads(gdbctx);
-            packet_reply_xfer(gdbctx,
-                              gdbctx->qxfer_buffer.base,
-                              gdbctx->qxfer_buffer.len,
-                              off, len, &more);
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
-            return packet_done;
-        }
 
-        if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
-        {
-            BOOL more;
+            gdbctx->qxfer_object_idx = i;
+            lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);
 
-            if (!gdbctx->process) return packet_error;
-            if (!(cpu = gdbctx->process->be_cpu)) return packet_error;
+            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, &more);
+                result = (result & packet_last_f) | packet_done;
+            }
+
+            gdbctx->qxfer_object_idx = -1;
+            memset(gdbctx->qxfer_object_annex, 0, QX_ANNEX_SIZE);
             reply_buffer_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);
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
-            return packet_done;
+
+            return result;
         }
         break;
     }
@@ -2408,6 +2481,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 v4 5/7] winedbg: Define table for GDB qXfer command handlers.

[Rémi Bernon]

On 11/19/21 14:41, Jinoh Kang wrote:
> +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, char* annex_buf, unsigned int* offp, unsigned int* lenp)
> +{
> +    int n;
> +
> +    /* Xfer:<object>:read:<annex>:<offset-hex>,<length-hex> */
> +
> +    if (sscanf(ptr, "Xfer:%" STRINGIFY(MAX_QX_NAME_LEN) "[^:]:read:%n", name_buf, &n) != 1)
> +        return FALSE;
> +    ptr += (unsigned int)n;
> +
> +    if (sscanf(ptr, "%" STRINGIFY(MAX_QX_ANNEX_LEN) "[^:]%n", annex_buf, &n) != 1)
> +    {
> +        annex_buf[0] = '\0';
> +        n = 0;
> +    }
> +    ptr += (unsigned int)n;
> +
> +    return sscanf(ptr, ":%x,%x", offp, lenp) == 2;
> +}
> +

I feel that this is much more complicated than it needs to be, 
expecially with the stringify to get buffer lengths. We usually just use 
MAX_PATH uness we really need something bigger, and then hardcoding the 
length in sscanf doesn't seem too bad.

Something like this below instead of this helper should be more readable 
IMHO:
> char object_name[MAX_PATH] = {0}, annex[MAX_PATH] = {0};
>
> /* [...] */
>
> if (sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read:%256[^:]:%x,%x", object_name, annex, &off, &len) == 4 ||
>         sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read::%x,%x", object_name, &off, &len) == 3)
> 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])
>       {
> @@ -2008,11 +2082,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;
>           }
> @@ -2043,53 +2122,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)
> +        if (parse_xfer_read(gdbctx->in_packet, object_name, annex, &off, &len))
>           {
> +            enum packet_return result;
> +            int i;
>               BOOL more;
>
> -            if (!gdbctx->process) return packet_error;
> +            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 (strncmp(qxfer_handlers[i].name, object_name, QX_NAME_SIZE) == 0)
> +                    break;
> +            }

I usually prefer the pattern where the loop does lookup and break, and 
the outcome is checked outside, like:
> for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
>     if (!strcmp(qxfer_handlers[i].name, object_name))
>         break;
> 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 (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
> -        {
> -            BOOL more;
> +            gdbctx->qxfer_object_idx = i;
> +            lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);

I think you can probably assume zero-terminated strings, especially if 
the buffers are zero initialized and a smaller size is passed to sscanf 
(but I think it's supposed to add a zero terminator anyway), so you can 
use strcpy / strcmp everywhere. There's a couple of other calls 
elsewhere in this patch.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>

[PATCH v4 5/7] winedbg: Define table for GDB qXfer command handlers.

[Jinoh Kang]

On 11/20/21 00:15, Rémi Bernon wrote:
> On 11/19/21 14:41, Jinoh Kang wrote:
> > +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, char* annex_buf, unsigned int* offp, unsigned int* lenp)
> > +{
> > +    int n;
> > +
> > +    /* Xfer:<object>:read:<annex>:<offset-hex>,<length-hex> */
> > +
> > +    if (sscanf(ptr, "Xfer:%" STRINGIFY(MAX_QX_NAME_LEN) "[^:]:read:%n", name_buf, &n) != 1)
> > +        return FALSE;
> > +    ptr += (unsigned int)n;
> > +
> > +    if (sscanf(ptr, "%" STRINGIFY(MAX_QX_ANNEX_LEN) "[^:]%n", annex_buf, &n) != 1)
> > +    {
> > +        annex_buf[0] = '\0';
> > +        n = 0;
> > +    }
> > +    ptr += (unsigned int)n;
> > +
> > +    return sscanf(ptr, ":%x,%x", offp, lenp) == 2;
> > +}
> > +
>
> I feel that this is much more complicated than it needs to be, expecially with the stringify to get buffer lengths.

Ack.
> We usually just use MAX_PATH uness we really need something bigger, and then hardcoding the length in sscanf doesn't seem too bad.

Ack.  MAX_PATH sounds plenty.
> Something like this below instead of this helper should be more readable IMHO:
> > char object_name[MAX_PATH] = {0}, annex[MAX_PATH] = {0};
> >
> > /* [...] */
> >
> >     if (sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read:%256[^:]:%x,%x", object_name, annex, &off, &len) == 4 ||
> >         sscanf(gdbctx->in_packet, "Xfer:%256[^:]:read::%x,%x", object_name, &off, &len) == 3)

I tend to prefer avoiding duplication of strings, since it makes me scan
the code twice for any differences from seemingly similar code.
But this is just two lines of code, it shouldn't really matter...
> >   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])
> >       {
> > @@ -2008,11 +2082,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;
> >           }
> > @@ -2043,53 +2122,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)
> > +        if (parse_xfer_read(gdbctx->in_packet, object_name, annex, &off, &len))
> >           {
> > +            enum packet_return result;
> > +            int i;
> >               BOOL more;
> >   -            if (!gdbctx->process) return packet_error;
> > +            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 (strncmp(qxfer_handlers[i].name, object_name, QX_NAME_SIZE) == 0)
> > +                    break;
> > +            }
> >  
>
> I usually prefer the pattern where the loop does lookup and break, and the outcome is checked outside, like:
> > for (i = 0; i < ARRAY_SIZE(qxfer_handlers); i++)
> >     if (!strcmp(qxfer_handlers[i].name, object_name))
> >         break;
> > 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;
> > }

Ack.
> > -        if (sscanf(gdbctx->in_packet, "Xfer:features:read:target.xml:%x,%x", &off, &len) == 2)
> > -        {
> > -            BOOL more;
> > +            gdbctx->qxfer_object_idx = i;
> > +            lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);
>
> I think you can probably assume zero-terminated strings,

Sounds reasonable.
> especially if the buffers are zero initialized and a smaller size is passed to sscanf (but I think it's supposed to add a zero terminator anyway),

Yes, looks like scanf always adds the \0 terminator.
> so you can use strcpy / strcmp everywhere. There's a couple of other calls elsewhere in this patch.

I'm okay with strcmp.  For strcpy with dynamic source though,
let's say perhaps I'm going to think about it for a bit...

-- 
Sincerely,
Jinoh Kang

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

Notes:
    v3 -> v4: Synchronize to prior modified patches.

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

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index ace3c71492c..743bd95a5e7 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -2141,14 +2141,26 @@ 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);
 
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
+            if (off > 0 &&
+                gdbctx->qxfer_buffer.len > 0 &&
+                gdbctx->qxfer_object_idx == i &&
+                strncmp(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE) == 0)
+            {
+                result = packet_send_buffer;
+                TRACE("qXfer read result = %d (cached)\n", result);
+            }
+            else
+            {
+                reply_buffer_empty(&gdbctx->qxfer_buffer);
 
-            gdbctx->qxfer_object_idx = i;
-            lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);
+                gdbctx->qxfer_object_idx = i;
+                lstrcpynA(gdbctx->qxfer_object_annex, annex, QX_ANNEX_SIZE);
 
-            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);
+            }
 
+            more = FALSE;
             if ((result & ~packet_last_f) == packet_send_buffer)
             {
                 packet_reply_xfer(gdbctx,
@@ -2158,9 +2170,12 @@ static enum packet_return packet_query(struct gdb_context* gdbctx)
                 result = (result & packet_last_f) | packet_done;
             }
 
-            gdbctx->qxfer_object_idx = -1;
-            memset(gdbctx->qxfer_object_annex, 0, QX_ANNEX_SIZE);
-            reply_buffer_empty(&gdbctx->qxfer_buffer);
+            if (!more)
+            {
+                gdbctx->qxfer_object_idx = -1;
+                memset(gdbctx->qxfer_object_annex, 0, QX_ANNEX_SIZE);
+                reply_buffer_empty(&gdbctx->qxfer_buffer);
+            }
 
             return result;
         }
-- 
2.31.1

[PATCH v4 7/7] 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>
---

Notes:
    v3 -> v4: Fix pointer variable declaration style

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

diff --git a/programs/winedbg/gdbproxy.c b/programs/winedbg/gdbproxy.c
index 743bd95a5e7..17a0afa885c 100644
--- a/programs/winedbg/gdbproxy.c
+++ b/programs/winedbg/gdbproxy.c
@@ -105,7 +105,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,
 };
@@ -1961,6 +1964,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;
@@ -1970,6 +2000,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, char* annex_buf, unsigned int* offp, unsigned int* lenp)
-- 
2.31.1