[Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Martin Simmons]

While using qemu with gdb "target remote" to debug an application that uses
fork and exec, the qemu process receives SIGSTOP every time the forked process
terminates (sending SIGCHLD).

This is caused by a missing call to gdb_signal_to_target in gdbstub.c, which
is fixed by this patch:

Signed-off-by: Martin Simmons <martin@lispworks.com>

diff --git a/gdbstub.c b/gdbstub.c
index d1b5afd..6a73a35 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -823,7 +823,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
                 action = *p++;
                 signal = 0;
                 if (action == 'C' || action == 'S') {
-                    signal = strtoul(p, (char **)&p, 16);
+                    signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
+                    if (signal == -1)
+                        signal = 0;
                 } else if (action != 'c' && action != 's') {
                     res = 0;
                     break;

__Martin

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Peter Maydell]

On 4 November 2014 17:51, Martin Simmons <martin@lispworks.com> wrote:
> While using qemu with gdb "target remote" to debug an application that uses
> fork and exec, the qemu process receives SIGSTOP every time the forked process
> terminates (sending SIGCHLD).
>
> This is caused by a missing call to gdb_signal_to_target in gdbstub.c, which
> is fixed by this patch:
>
> Signed-off-by: Martin Simmons <martin@lispworks.com>
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d1b5afd..6a73a35 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -823,7 +823,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>                  action = *p++;
>                  signal = 0;
>                  if (action == 'C' || action == 'S') {
> -                    signal = strtoul(p, (char **)&p, 16);
> +                    signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
> +                    if (signal == -1)
> +                        signal = 0;
>                  } else if (action != 'c' && action != 's') {
>                      res = 0;
>                      break;

The if() statement should have braces for our coding style,
and no space before the '(' in function calls; otherwise this
looks good to me.

I notice that gdb_signal_to_target() doesn't check for being
passed negative numbers, which means a malicious gdb could
make us crash here, but I assume nobody actually treats the
gdbstub as a security boundary... Anyway, that's a separate
issue for a different patch.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Martin Simmons]

>>>>> On Tue, 4 Nov 2014 19:09:43 +0000, Peter Maydell said:
> 
> On 4 November 2014 17:51, Martin Simmons <martin@lispworks.com> wrote:
> > While using qemu with gdb "target remote" to debug an application that uses
> > fork and exec, the qemu process receives SIGSTOP every time the forked process
> > terminates (sending SIGCHLD).
> >
> > This is caused by a missing call to gdb_signal_to_target in gdbstub.c, which
> > is fixed by this patch:
> >
> > Signed-off-by: Martin Simmons <martin@lispworks.com>
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index d1b5afd..6a73a35 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -823,7 +823,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> >                  action = *p++;
> >                  signal = 0;
> >                  if (action == 'C' || action == 'S') {
> > -                    signal = strtoul(p, (char **)&p, 16);
> > +                    signal = gdb_signal_to_target (strtoul(p, (char **)&p, 16));
> > +                    if (signal == -1)
> > +                        signal = 0;
> >                  } else if (action != 'c' && action != 's') {
> >                      res = 0;
> >                      break;
> 
> The if() statement should have braces for our coding style,
> and no space before the '(' in function calls; otherwise this
> looks good to me.

Do you want a new patch with it like that?  I was following the style of the
rest of that file, in particular the other 'C' case :-(

__Martin

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Peter Maydell]

On 5 November 2014 13:50, Martin Simmons <martin@lispworks.com> wrote:
>>>>>> On Tue, 4 Nov 2014 19:09:43 +0000, Peter Maydell said:
>> The if() statement should have braces for our coding style,
>> and no space before the '(' in function calls; otherwise this
>> looks good to me.
>
> Do you want a new patch with it like that?  I was following the style of the
> rest of that file, in particular the other 'C' case :-(

Yes, if you could respin it that would be nice. Unfortunately there
are still areas of our codebase which don't follow the coding
style; we update bits of code as we change them, so new patches
follow the style guide for the lines they touch. You can use
scripts/checkpatch.pl to check your patch for the most common
issues.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Martin Simmons]

>>>>> On Wed, 5 Nov 2014 14:17:36 +0000, Peter Maydell said:
> 
> On 5 November 2014 13:50, Martin Simmons <martin@lispworks.com> wrote:
> >>>>>> On Tue, 4 Nov 2014 19:09:43 +0000, Peter Maydell said:
> >> The if() statement should have braces for our coding style,
> >> and no space before the '(' in function calls; otherwise this
> >> looks good to me.
> >
> > Do you want a new patch with it like that?  I was following the style of the
> > rest of that file, in particular the other 'C' case :-(
> 
> Yes, if you could respin it that would be nice. Unfortunately there
> are still areas of our codebase which don't follow the coding
> style; we update bits of code as we change them, so new patches
> follow the style guide for the lines they touch. You can use
> scripts/checkpatch.pl to check your patch for the most common
> issues.

OK, here it is.

Signed-off-by: Martin Simmons <martin@lispworks.com>

diff --git a/gdbstub.c b/gdbstub.c
index d1b5afd..0faca56 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -823,7 +823,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
                 action = *p++;
                 signal = 0;
                 if (action == 'C' || action == 'S') {
-                    signal = strtoul(p, (char **)&p, 16);
+                    signal = gdb_signal_to_target(strtoul(p, (char **)&p, 16));
+                    if (signal == -1) {
+                        signal = 0;
+                    }
                 } else if (action != 'c' && action != 's') {
                     res = 0;
                     break;

__Martin

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Peter Maydell]

On 5 November 2014 14:47, Martin Simmons <martin@lispworks.com> wrote:
>>>>>> On Wed, 5 Nov 2014 14:17:36 +0000, Peter Maydell said:
>>
>> On 5 November 2014 13:50, Martin Simmons <martin@lispworks.com> wrote:
>> >>>>>> On Tue, 4 Nov 2014 19:09:43 +0000, Peter Maydell said:
>> >> The if() statement should have braces for our coding style,
>> >> and no space before the '(' in function calls; otherwise this
>> >> looks good to me.
>> >
>> > Do you want a new patch with it like that?  I was following the style of the
>> > rest of that file, in particular the other 'C' case :-(
>>
>> Yes, if you could respin it that would be nice. Unfortunately there
>> are still areas of our codebase which don't follow the coding
>> style; we update bits of code as we change them, so new patches
>> follow the style guide for the lines they touch. You can use
>> scripts/checkpatch.pl to check your patch for the most common
>> issues.
>
> OK, here it is.
>
> Signed-off-by: Martin Simmons <martin@lispworks.com>
>
> diff --git a/gdbstub.c b/gdbstub.c
> index d1b5afd..0faca56 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -823,7 +823,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>                  action = *p++;
>                  signal = 0;
>                  if (action == 'C' || action == 'S') {
> -                    signal = strtoul(p, (char **)&p, 16);
> +                    signal = gdb_signal_to_target(strtoul(p, (char **)&p, 16));
> +                    if (signal == -1) {
> +                        signal = 0;
> +                    }
>                  } else if (action != 'c' && action != 's') {
>                      res = 0;
>                      break;
>
> __Martin
>

This version Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
and cc'd trivial. (Somebody's going to have to manually put the
patch together with the commit message now, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: Add a missing case of signal number translation in gdbstub

[Michael Tokarev]

05.11.2014 18:06, Peter Maydell wrote:
[]
> This version Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> and cc'd trivial. (Somebody's going to have to manually put the
> patch together with the commit message now, though.)

Applied to -trivial, putting all parts together.

Thanks,

/mjt