Re: [PATCH v3 04/20] gdbstub: move mem_buf to GDBState and use GByteArray

["Alex Bennée" <alex.bennee@linaro.org>]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 12/11/19 6:05 PM, Alex Bennée wrote:
>> This is in preparation for further re-factoring of the register API
>> with the rest of the code. Theoretically the read register function
>> could overwrite the MAX_PACKET_LENGTH buffer although currently all
>> registers are well within the size range.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
>> Tested-by: Damien Hedde <damien.hedde@greensocs.com>
>> 
>> ---
>> v3
>>   - fixed up email on Damien's tags
>> ---
>>  gdbstub.c | 56 ++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 35 insertions(+), 21 deletions(-)
>> 
>
>> @@ -2092,11 +2105,12 @@ static void handle_query_rcmd(GdbCmdContext *gdb_ctx, void *user_ctx)
>>      }
>>  
>>      len = len / 2;
>> -    hextomem(gdb_ctx->mem_buf, gdb_ctx->params[0].data, len);
>> -    gdb_ctx->mem_buf[len++] = 0;
>> -    qemu_chr_be_write(gdbserver_state.mon_chr, gdb_ctx->mem_buf, len);
>> +    g_byte_array_set_size(gdbserver_state.mem_buf, len);
>
> Hi Alex,
>
> Just found out that the g_byte_array_set_size() above should be removed.
> hextomem() will append data starting at offset [len] instead of [0] and
> we end up with an uninitialized prefix in the array.

Oops, fixed. I should assert len is 0 before we start.

>
>> +    hextomem(gdbserver_state.mem_buf, gdb_ctx->params[0].data, len);
>> +    g_byte_array_append(gdbserver_state.mem_buf, &zero, 1);
>> +    qemu_chr_be_write(gdbserver_state.mon_chr, gdbserver_state.mem_buf->data,
>> +                      gdbserver_state.mem_buf->len);
>>      put_packet("OK");
>> -
>>  }
>>  #endif
>>  
>> 
>
> I did double-checked the rest of the patch and it is it the only resize
> that passed through v2 review.
>
> Regards,
> Damien


-- 
Alex Bennée