[Qemu-devel] [PATCH] gdbstub.c: fix GDB connection segfault caused by empty machines

[Qemu-devel] [PATCH] gdbstub.c: fix GDB connection segfault caused by empty machines

[Ziyue Yang]

From: Ziyue Yang <yzylivezh@hotmail.com>

This patch is to fix the segmentation fault caused by attaching
GDB to a QEMU instance initialized with "-M none" option.

The bug can be reproduced by

> ./qemu-system-x86_64 -M none -nographic -S -s

and attach a GDB to it by

> gdb -ex 'target remote :1234

The segmentation fault was originally caused by trying to read
the information about CPU when communicating with GDB. However,
it's impossible for any control flow to exist on an empty machine,
nor can CPU's be hot plugged to an empty machine later by QOM
commands. So I think simply disabling GDB connections on empty
machines makes sense.

Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
---
 gdbstub.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index de62d26..413e817 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1731,6 +1731,12 @@ int gdbserver_start(const char *device)
     CharDriverState *mon_chr;
     ChardevCommon common = { 0 };
 
+    if (!first_cpu) {
+        fprintf(stderr, "gdbstub: meaningless to attach gdb to a "
+                "machine without any CPU.\n");
+        return -1;
+    }
+
     if (!device)
         return -1;
     if (strcmp(device, "none") != 0) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH] gdbstub.c: fix GDB connection segfault caused by empty machines

[Thomas Huth]

On 17.01.2017 16:19, Ziyue Yang wrote:
> From: Ziyue Yang <yzylivezh@hotmail.com>
> 
> This patch is to fix the segmentation fault caused by attaching
> GDB to a QEMU instance initialized with "-M none" option.
> 
> The bug can be reproduced by
> 
>> ./qemu-system-x86_64 -M none -nographic -S -s
> 
> and attach a GDB to it by
> 
>> gdb -ex 'target remote :1234
> 
> The segmentation fault was originally caused by trying to read
> the information about CPU when communicating with GDB. However,
> it's impossible for any control flow to exist on an empty machine,
> nor can CPU's be hot plugged to an empty machine later by QOM
> commands. So I think simply disabling GDB connections on empty
> machines makes sense.

Yes, this sounds like a proper and easy fix for the problem.

> Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
> ---
>  gdbstub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index de62d26..413e817 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1731,6 +1731,12 @@ int gdbserver_start(const char *device)
>      CharDriverState *mon_chr;
>      ChardevCommon common = { 0 };
>  
> +    if (!first_cpu) {
> +        fprintf(stderr, "gdbstub: meaningless to attach gdb to a "
> +                "machine without any CPU.\n");
> +        return -1;
> +    }

Could you maybe rather use error_report() instead of fprintf()? I think
that's the preferred way to print out an error in QEMU nowadays.

 Thomas

Re: [Qemu-devel] [PATCH] gdbstub.c: fix GDB connection segfault caused by empty machines

[Yang Ziyue]

2017-01-18 1:27 GMT+08:00 Thomas Huth <thuth@redhat.com>:
>
> On 17.01.2017 16:19, Ziyue Yang wrote:
> > From: Ziyue Yang <yzylivezh@hotmail.com>
> >
> > This patch is to fix the segmentation fault caused by attaching
> > GDB to a QEMU instance initialized with "-M none" option.
> >
> > The bug can be reproduced by
> >
> >> ./qemu-system-x86_64 -M none -nographic -S -s
> >
> > and attach a GDB to it by
> >
> >> gdb -ex 'target remote :1234
> >
> > The segmentation fault was originally caused by trying to read
> > the information about CPU when communicating with GDB. However,
> > it's impossible for any control flow to exist on an empty machine,
> > nor can CPU's be hot plugged to an empty machine later by QOM
> > commands. So I think simply disabling GDB connections on empty
> > machines makes sense.
>
> Yes, this sounds like a proper and easy fix for the problem.
>
> > Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
> > ---
> >  gdbstub.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index de62d26..413e817 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -1731,6 +1731,12 @@ int gdbserver_start(const char *device)
> >      CharDriverState *mon_chr;
> >      ChardevCommon common = { 0 };
> >
> > +    if (!first_cpu) {
> > +        fprintf(stderr, "gdbstub: meaningless to attach gdb to a "
> > +                "machine without any CPU.\n");
> > +        return -1;
> > +    }
>
> Could you maybe rather use error_report() instead of fprintf()? I think
> that's the preferred way to print out an error in QEMU nowadays.

Sure. I've sent a v2 version patch fixing that.

>
>
>  Thomas
>