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

[Akihiko Odaki]

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