Re: [PULL 19/25] gdbstub: Replace gdb_regs with an array

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/10/17 23:05, Fabiano Rosas wrote:
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> An array is a more appropriate data structure than a list for gdb_regs
>> since it is initialized only with append operation and read-only after
>> initialization.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Message-Id: <20230912224107.29669-13-akihiko.odaki@daynix.com>
>> [AJB: fixed a checkpatch violation]
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-Id: <20231009164104.369749-20-alex.bennee@linaro.org>
>>
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 7b8347ed5a..3968369554 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -502,7 +502,7 @@ struct CPUState {
>>   
>>       CPUJumpCache *tb_jmp_cache;
>>   
>> -    struct GDBRegisterState *gdb_regs;
>> +    GArray *gdb_regs;
>>       int gdb_num_regs;
>>       int gdb_num_g_regs;
>>       QTAILQ_ENTRY(CPUState) node;
>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> index 62608a5389..b1532118d1 100644
>> --- a/gdbstub/gdbstub.c
>> +++ b/gdbstub/gdbstub.c
>> @@ -51,7 +51,6 @@ typedef struct GDBRegisterState {
>>       gdb_get_reg_cb get_reg;
>>       gdb_set_reg_cb set_reg;
>>       const char *xml;
>> -    struct GDBRegisterState *next;
>>   } GDBRegisterState;
>>   
>>   GDBState gdbserver_state;
>> @@ -386,7 +385,8 @@ static const char *get_feature_xml(const char *p, const char **newp,
>>                   xml,
>>                   g_markup_printf_escaped("<xi:include href=\"%s\"/>",
>>                                           cc->gdb_core_xml_file));
>> -            for (r = cpu->gdb_regs; r; r = r->next) {
>> +            for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> 
> It seems we can reach here before having initialized gdb_regs at
> gdb_register_coprocessor():
> 
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x0000555555e5310b in get_feature_xml (p=0x555556a99118
> <gdbserver_state+56> "target.xml:0,ffb", newp=0x7fffffffc6b0,
> process=0x555557a21dd0) at ../gdbstub/gdbstub.c:388
> 
> (gdb) p/x cpu->gdb_regs
> $1 = 0x0
> 
> 
> Using:
> qemu-system-x86 ... -s -s
> 
> just connect GDB and it crashes.

Hi,

Sorry for trouble and thank you for reporting.

I have just posted a fix "[PATCH v4 1/5] gdbstub: Check if gdb_regs is 
NULL" as part of series "[PATCH v4 0/5] gdbstub and TCG plugin 
improvements". Please test it if possible.

Alex, you may pick the patch early since the bug is quite a serious. 
Please add "Reported-by: Fabiano Rosas <farosas@suse.de>" when you do so 
since I forgot it. You may skip applying target/riscv patches though 
since the maintainers may still have something to comment.

Regards,
Akihiko Odaki