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

[Qemu-devel] [PATCH v4 1/2] 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.

Also some updates from fprintf(stderr, ...) to error_report.

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

diff --git a/gdbstub.c b/gdbstub.c
index de62d26..426d55e 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -18,6 +18,7 @@
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/cutils.h"
 #include "cpu.h"
 #ifdef CONFIG_USER_ONLY
@@ -1731,6 +1732,12 @@ int gdbserver_start(const char *device)
     CharDriverState *mon_chr;
     ChardevCommon common = { 0 };

+    if (!first_cpu) {
+        error_report("gdbstub: meaningless to attach gdb to a "
+                     "machine without any CPU.");
+        return -1;
+    }
+
     if (!device)
         return -1;
     if (strcmp(device, "none") != 0) {
--
2.7.4

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

[Thomas Huth]

On 18.01.2017 04:21, 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.
> 
> Also some updates from fprintf(stderr, ...) to error_report.

Seems like that last sentence is a left-over from your other patch and
should be removed from this patch description here.

> Signed-off-by: Ziyue Yang <skiver.cloud.yzy@gmail.com>
> ---
>  gdbstub.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index de62d26..426d55e 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -18,6 +18,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/cutils.h"
>  #include "cpu.h"
>  #ifdef CONFIG_USER_ONLY
> @@ -1731,6 +1732,12 @@ int gdbserver_start(const char *device)
>      CharDriverState *mon_chr;
>      ChardevCommon common = { 0 };
> 
> +    if (!first_cpu) {
> +        error_report("gdbstub: meaningless to attach gdb to a "
> +                     "machine without any CPU.");
> +        return -1;
> +    }
> +
>      if (!device)
>          return -1;
>      if (strcmp(device, "none") != 0) {
> --
> 2.7.4
> 

With the "fprintf" sentence removed:

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Yang Ziyue]

Hi Thomas,

I'm not quite sure about that, but did you mean that I should mention
the second patch before the Sign-off line instead of in the commit
message?
Thanks!

Ziyue Yang

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

[Thomas Huth]

 Hi,

On 18.01.2017 08:43, Yang Ziyue wrote:
> Hi Thomas,
> 
> I'm not quite sure about that, but did you mean that I should mention
> the second patch before the Sign-off line instead of in the commit
> message?

No, I meant: You still got the sentence "Also some updates from
fprintf(stderr, ...) to error_report." in the description of your first
patch. But indeed this is now done in the second patch instead. So
simply remove that sentence from the description of your first patch,
and everything is fine :-)

 Thomas

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

[Yang Ziyue]

Got it. Thanks!

2017-01-18 15:48 GMT+08:00 Thomas Huth <thuth@redhat.com>:
>  Hi,
>
> On 18.01.2017 08:43, Yang Ziyue wrote:
>> Hi Thomas,
>>
>> I'm not quite sure about that, but did you mean that I should mention
>> the second patch before the Sign-off line instead of in the commit
>> message?
>
> No, I meant: You still got the sentence "Also some updates from
> fprintf(stderr, ...) to error_report." in the description of your first
> patch. But indeed this is now done in the second patch instead. So
> simply remove that sentence from the description of your first patch,
> and everything is fine :-)
>
>  Thomas
>