The main problem was that we were treating a guest address
as a host address with a mere cast.

Use the correct interface for accessing guest memory.  Do not
allow offset == auxv_len, which would result in an empty packet.

Fixes: 51c623b0de1 ("gdbstub: add support to Xfer:auxv:read: packet")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Fixes a number of check-tcg failures, which are transformed from
proper failures to "SKIP: PC not set" by the test harness (bug?).
But the qemu executable has in fact crashed.

I'm embarrased to note that I reviewed the original patch, and
should have noticed this.


r~

---
 gdbstub.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index c7ca7e9f88..759bb00bcf 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2245,7 +2245,6 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
 {
     TaskState *ts;
     unsigned long offset, len, saved_auxv, auxv_len;
-    const char *mem;
 
     if (gdb_ctx->num_params < 2) {
         put_packet("E22");
@@ -2257,8 +2256,8 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
     ts = gdbserver_state.c_cpu->opaque;
     saved_auxv = ts->info->saved_auxv;
     auxv_len = ts->info->auxv_len;
-    mem = (const char *)(saved_auxv + offset);
-    if (offset > auxv_len) {
+
+    if (offset >= auxv_len) {
         put_packet("E00");
         return;
     }
@@ -2269,12 +2268,20 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
 
     if (len < auxv_len - offset) {
         g_string_assign(gdbserver_state.str_buf, "m");
-        memtox(gdbserver_state.str_buf, mem, len);
     } else {
         g_string_assign(gdbserver_state.str_buf, "l");
-        memtox(gdbserver_state.str_buf, mem, auxv_len - offset);
+        len = auxv_len - offset;
     }
 
+    g_byte_array_set_size(gdbserver_state.mem_buf, len);
+    if (target_memory_rw_debug(gdbserver_state.g_cpu, saved_auxv + offset,
+                               gdbserver_state.mem_buf->data, len, false)) {
+        put_packet("E14");
+        return;
+    }
+
+    memtox(gdbserver_state.str_buf,
+           (const char *)gdbserver_state.mem_buf->data, len);
     put_packet_binary(gdbserver_state.str_buf->str,
                       gdbserver_state.str_buf->len, true);
 }
-- 
2.25.1

Richard Henderson <richard.henderson@linaro.org> writes:

> The main problem was that we were treating a guest address
> as a host address with a mere cast.
>
> Use the correct interface for accessing guest memory.  Do not
> allow offset == auxv_len, which would result in an empty packet.
>
> Fixes: 51c623b0de1 ("gdbstub: add support to Xfer:auxv:read: packet")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>
> Fixes a number of check-tcg failures, which are transformed from
> proper failures to "SKIP: PC not set" by the test harness (bug?).
> But the qemu executable has in fact crashed.

I wonder if I can tighten that up to detect the failed remote end in the
scripts.

Anyway snarfed into my queue. Thanks!

>
> I'm embarrased to note that I reviewed the original patch, and
> should have noticed this.
>
>
> r~
>
> ---
>  gdbstub.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index c7ca7e9f88..759bb00bcf 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2245,7 +2245,6 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
>  {
>      TaskState *ts;
>      unsigned long offset, len, saved_auxv, auxv_len;
> -    const char *mem;
>  
>      if (gdb_ctx->num_params < 2) {
>          put_packet("E22");
> @@ -2257,8 +2256,8 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
>      ts = gdbserver_state.c_cpu->opaque;
>      saved_auxv = ts->info->saved_auxv;
>      auxv_len = ts->info->auxv_len;
> -    mem = (const char *)(saved_auxv + offset);
> -    if (offset > auxv_len) {
> +
> +    if (offset >= auxv_len) {
>          put_packet("E00");
>          return;
>      }
> @@ -2269,12 +2268,20 @@ static void handle_query_xfer_auxv(GdbCmdContext *gdb_ctx, void *user_ctx)
>  
>      if (len < auxv_len - offset) {
>          g_string_assign(gdbserver_state.str_buf, "m");
> -        memtox(gdbserver_state.str_buf, mem, len);
>      } else {
>          g_string_assign(gdbserver_state.str_buf, "l");
> -        memtox(gdbserver_state.str_buf, mem, auxv_len - offset);
> +        len = auxv_len - offset;
>      }
>  
> +    g_byte_array_set_size(gdbserver_state.mem_buf, len);
> +    if (target_memory_rw_debug(gdbserver_state.g_cpu, saved_auxv + offset,
> +                               gdbserver_state.mem_buf->data, len, false)) {
> +        put_packet("E14");
> +        return;
> +    }
> +
> +    memtox(gdbserver_state.str_buf,
> +           (const char *)gdbserver_state.mem_buf->data, len);
>      put_packet_binary(gdbserver_state.str_buf->str,
>                        gdbserver_state.str_buf->len, true);
>  }


-- 
Alex Bennée