Re: [Qemu-devel] [PATCH v9 13/27] gdbstub: Implement write all registers (G pkt) with new infra

[Alex Bennée]

Jon Doron <address@hidden> writes:

> Signed-off-by: Jon Doron <address@hidden>
> ---
>  gdbstub.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index daa602edc3..adfe39b3a3 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1734,6 +1734,29 @@ static void handle_read_mem(GdbCmdContext *gdb_ctx, 
> void *user_ctx)
>      put_packet(gdb_ctx->s, gdb_ctx->str_buf);
>  }
>
> +static void handle_write_all_regs(GdbCmdContext *gdb_ctx, void *user_ctx)
> +{
> +    target_ulong addr, len;
> +    uint8_t *registers;
> +    int reg_size;
> +
> +    if (!gdb_ctx->num_params) {
> +        return;
> +    }
> +
> +    cpu_synchronize_state(gdb_ctx->s->g_cpu);
> +    registers = gdb_ctx->mem_buf;
> +    len = strlen(gdb_ctx->params[0].data) / 2;
> +    hextomem(registers, gdb_ctx->params[0].data, len);

Admittedly the legacy code didn't check either but we are assuming that
registers (i.e. gdb_ctx->mem_buf) won't overflow. It's probably still
safe as the incoming packets are limited in size. I was trying to find
where MAX_PACKET_LENGTH came from and AFAICT it's an arbitrary number we
made up.

I wonder if it would make sense in another series to convert the various
buffers to GString and GBytes so we can dynamically handle longer
packets without all this inconsistent application of bounds checking.

Anyway:

Reviewed-by: Alex Bennée <address@hidden>


> +    for (addr = 0; addr < gdb_ctx->s->g_cpu->gdb_num_g_regs && len > 0;
> +         addr++) {
> +        reg_size = gdb_write_register(gdb_ctx->s->g_cpu, registers, addr);
> +        len -= reg_size;
> +        registers += reg_size;
> +    }
> +    put_packet(gdb_ctx->s, "OK");
> +}
> +
>  static int gdb_handle_packet(GDBState *s, const char *line_buf)
>  {
>      CPUState *cpu;
> @@ -1745,7 +1768,6 @@ static int gdb_handle_packet(GDBState *s, const char 
> *line_buf)
>      uint8_t mem_buf[MAX_PACKET_LENGTH];
>      char buf[sizeof(mem_buf) + 1 /* trailing NUL */];
>      char thread_id[16];
> -    uint8_t *registers;
>      target_ulong addr, len;
>      const GdbCmdParseEntry *cmd_parser = NULL;
>
> @@ -1911,16 +1933,15 @@ static int gdb_handle_packet(GDBState *s, const char 
> *line_buf)
>          put_packet(s, buf);
>          break;
>      case 'G':
> -        cpu_synchronize_state(s->g_cpu);
> -        registers = mem_buf;
> -        len = strlen(p) / 2;
> -        hextomem((uint8_t *)registers, p, len);
> -        for (addr = 0; addr < s->g_cpu->gdb_num_g_regs && len > 0; addr++) {
> -            reg_size = gdb_write_register(s->g_cpu, registers, addr);
> -            len -= reg_size;
> -            registers += reg_size;
> +        {
> +            static const GdbCmdParseEntry write_all_regs_cmd_desc = {
> +                .handler = handle_write_all_regs,
> +                .cmd = "G",
> +                .cmd_startswith = 1,
> +                .schema = "s0"
> +            };
> +            cmd_parser = &write_all_regs_cmd_desc;
>          }
> -        put_packet(s, "OK");
>          break;
>      case 'm':
>          {


--
Alex Bennée