diff mbox

[v2] gdbstub.c: fix GDB connection segfault caused by empty machines

Message ID 1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com
State New
Headers show

Commit Message

skiver.cloud.yzy@gmail.com Jan. 18, 2017, 1:14 a.m. UTC 
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 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

--
2.7.4

Comments

no-reply@patchew.org Jan. 18, 2017, 1:19 a.m. UTC | #1 
Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com
Type: series
Subject: [Qemu-devel] [PATCH v2] gdbstub.c: fix GDB connection segfault caused by empty machines

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com -> patchew/1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com
Switched to a new branch 'test'
4daee16 gdbstub.c: fix GDB connection segfault caused by empty machines

=== OUTPUT BEGIN ===
Checking PATCH 1/1: gdbstub.c: fix GDB connection segfault caused by empty machines...
ERROR: Error messages should not contain newlines
#48: FILE: gdbstub.c:640:
+            error_report("Error: Bad gdb register numbering for '%s'\n"

ERROR: Error messages should not contain newlines
#49: FILE: gdbstub.c:641:
+                         "Expected %d got %d\n", xml, g_pos, s->base_reg);

ERROR: Error messages should not contain newlines
#58: FILE: gdbstub.c:893:
+        error_report("\nQEMU: Terminated via GDBstub\n");

ERROR: Error messages should not contain newlines
#68: FILE: gdbstub.c:1361:
+                error_report("gdbstub: Bad syscall format string '%s'\n",

ERROR: Error messages should not contain newlines
#79: FILE: gdbstub.c:1737:
+                     "machine without any CPU.\n");

total: 5 errors, 0 warnings, 47 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
skiver.cloud.yzy@gmail.com Jan. 18, 2017, 1:34 a.m. UTC | #2 
Oops seems I forgot to check the patch before submitting. Sorry for that.
The new patch is tagged as v3 now, but I'm not sure whether it should
be tagged as v2 since the origin v2 failed. Is there any specification
about this? Thanks!

2017-01-18 9:19 GMT+08:00  <no-reply@patchew.org>:
> Hi,
>
> Your series seems to have some coding style problems. See output below for
> more information:
>
> Message-id: 1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com
> Type: series
> Subject: [Qemu-devel] [PATCH v2] gdbstub.c: fix GDB connection segfault caused by empty machines
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
>
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
>
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
>
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
>
> exit $failed
> === TEST SCRIPT END ===
>
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]         patchew/1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com -> patchew/1484702052-7900-1-git-send-email-skiver.cloud.yzy@gmail.com
> Switched to a new branch 'test'
> 4daee16 gdbstub.c: fix GDB connection segfault caused by empty machines
>
> === OUTPUT BEGIN ===
> Checking PATCH 1/1: gdbstub.c: fix GDB connection segfault caused by empty machines...
> ERROR: Error messages should not contain newlines
> #48: FILE: gdbstub.c:640:
> +            error_report("Error: Bad gdb register numbering for '%s'\n"
>
> ERROR: Error messages should not contain newlines
> #49: FILE: gdbstub.c:641:
> +                         "Expected %d got %d\n", xml, g_pos, s->base_reg);
>
> ERROR: Error messages should not contain newlines
> #58: FILE: gdbstub.c:893:
> +        error_report("\nQEMU: Terminated via GDBstub\n");
>
> ERROR: Error messages should not contain newlines
> #68: FILE: gdbstub.c:1361:
> +                error_report("gdbstub: Bad syscall format string '%s'\n",
>
> ERROR: Error messages should not contain newlines
> #79: FILE: gdbstub.c:1737:
> +                     "machine without any CPU.\n");
>
> total: 5 errors, 0 warnings, 47 lines checked
>
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
>
> === OUTPUT END ===
>
> Test command exited with code: 1
>
>
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
Fam Zheng Jan. 18, 2017, 2:13 a.m. UTC | #3 
On Wed, 01/18 09:34, Yang Ziyue wrote:
> Oops seems I forgot to check the patch before submitting. Sorry for that.
> The new patch is tagged as v3 now, but I'm not sure whether it should
> be tagged as v2 since the origin v2 failed. Is there any specification
> about this? Thanks!

Hi Ziyue, thanks for submitting the patch. When you repost, use v3 please, a
(even failed or mistaken) version shouldn't be reused, so that people looking at
the subjects will know which one is newer.

Fam
skiver.cloud.yzy@gmail.com Jan. 18, 2017, 3:40 p.m. UTC | #4 
Got it. Thanks!

2017-01-18 10:13 GMT+08:00 Fam Zheng <famz@redhat.com>:
> On Wed, 01/18 09:34, Yang Ziyue wrote:
>> Oops seems I forgot to check the patch before submitting. Sorry for that.
>> The new patch is tagged as v3 now, but I'm not sure whether it should
>> be tagged as v2 since the origin v2 failed. Is there any specification
>> about this? Thanks!
>
> Hi Ziyue, thanks for submitting the patch. When you repost, use v3 please, a
> (even failed or mistaken) version shouldn't be reused, so that people looking at
> the subjects will know which one is newer.
>
> Fam
diff mbox

Patch

diff --git a/gdbstub.c b/gdbstub.c
index de62d26..3a22ce3 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
@@ -636,8 +637,8 @@  void gdb_register_coprocessor(CPUState *cpu,
     *p = s;
     if (g_pos) {
         if (g_pos != s->base_reg) {
-            fprintf(stderr, "Error: Bad gdb register numbering for '%s'\n"
-                    "Expected %d got %d\n", xml, g_pos, s->base_reg);
+            error_report("Error: Bad gdb register numbering for '%s'\n"
+                         "Expected %d got %d\n", xml, g_pos, s->base_reg);
         } else {
             cpu->gdb_num_g_regs = cpu->gdb_num_regs;
         }
@@ -889,7 +890,7 @@  static int gdb_handle_packet(GDBState *s, const char *line_buf)
         }
     case 'k':
         /* Kill the target */
-        fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
+        error_report("\nQEMU: Terminated via GDBstub\n");
         exit(0);
     case 'D':
         /* Detach packet */
@@ -1357,8 +1358,8 @@  void gdb_do_syscallv(gdb_syscall_complete_cb cb, const char *fmt, va_list va)
                 break;
             default:
             bad_format:
-                fprintf(stderr, "gdbstub: Bad syscall format string '%s'\n",
-                        fmt - 1);
+                error_report("gdbstub: Bad syscall format string '%s'\n",
+                             fmt - 1);
                 break;
             }
         } else {
@@ -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.\n");
+        return -1;
+    }
+
     if (!device)
         return -1;
     if (strcmp(device, "none") != 0) {