gdbstub: Add a missing case of signal number translation in gdbstub

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>


__Martin

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

>>>>> 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

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