[PATCH] gdbstub: Fix client Ctrl-C handling

[PATCH] gdbstub: Fix client Ctrl-C handling

[Nicholas Piggin]

The gdb remote protocol has a special interrupt character (0x03) that is
transmitted outside the regular packet processing, and represents a
Ctrl-C pressed in the client. Despite not being a regular packet, it
does expect a regular stop response if the stub successfully stops the
running program.

See: https://sourceware.org/gdb/onlinedocs/gdb/Interrupts.html

Inhibiting the stop reply packet can lead to gdb client hang. So permit
a stop response when receiving a character from gdb that stops the vm.
Additionally, add a warning if that was not a 0x03 character, because
the gdb session is likely to end up getting confused if this happens.

Cc: qemu-stable@nongnu.org
Cc: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Taylor Simpson <tsimpson@quicinc.com>
Reported-by: Frederic Barrat <fbarrat@linux.ibm.com>
Fixes: 758370052fb ("gdbstub: only send stop-reply packets when allowed to")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
Hey Fred, I'm not sure if this is the gdb hang you were seeing, but it
is the one I could reproduce. Could be worth checking there are no more
corner case hangs after this.

Thanks,
Nick

 gdbstub/gdbstub.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 6911b73c07..ce8b42eb15 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
             return;
     }
     if (runstate_is_running()) {
-        /* when the CPU is running, we cannot do anything except stop
-           it when receiving a char */
+        /*
+         * When the CPU is running, we cannot do anything except stop
+         * it when receiving a char. This is expected on a Ctrl-C in the
+         * gdb client. Because we are in all-stop mode, gdb sends a
+         * 0x03 byte which is not a usual packet, so we handle it specially
+         * here, but it does expect a stop reply.
+         */
+        if (ch != 0x03) {
+            warn_report("gdbstub: client sent packet while target running\n");
+        }
+        gdbserver_state.allow_stop_reply = true;
         vm_stop(RUN_STATE_PAUSED);
     } else
 #endif
-- 
2.40.1

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Matheus Tavares Bernardino]

> Nicholas Piggin <npiggin@gmail.com> wrote:
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 6911b73c07..ce8b42eb15 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
>              return;
>      }
>      if (runstate_is_running()) {
> -        /* when the CPU is running, we cannot do anything except stop
> -           it when receiving a char */
> +        /*
> +         * When the CPU is running, we cannot do anything except stop
> +         * it when receiving a char. This is expected on a Ctrl-C in the
> +         * gdb client. Because we are in all-stop mode, gdb sends a
> +         * 0x03 byte which is not a usual packet, so we handle it specially
> +         * here, but it does expect a stop reply.
> +         */
> +        if (ch != 0x03) {
> +            warn_report("gdbstub: client sent packet while target running\n");
> +        }
> +        gdbserver_state.allow_stop_reply = true;
>          vm_stop(RUN_STATE_PAUSED);
>      } else
>  #endif

Makes sense to me, but shouldn't we send the stop-reply packet only for
Ctrl+C/0x03?

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Nicholas Piggin]

On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index 6911b73c07..ce8b42eb15 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> >              return;
> >      }
> >      if (runstate_is_running()) {
> > -        /* when the CPU is running, we cannot do anything except stop
> > -           it when receiving a char */
> > +        /*
> > +         * When the CPU is running, we cannot do anything except stop
> > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > +         * here, but it does expect a stop reply.
> > +         */
> > +        if (ch != 0x03) {
> > +            warn_report("gdbstub: client sent packet while target running\n");
> > +        }
> > +        gdbserver_state.allow_stop_reply = true;
> >          vm_stop(RUN_STATE_PAUSED);
> >      } else
> >  #endif
>
> Makes sense to me, but shouldn't we send the stop-reply packet only for
> Ctrl+C/0x03?

Good question.

I think if we get a character here that's not a 3, we're already in
trouble, and we eat it so even worse. Since we only send a stop packet
back when the vm stops, then if we don't send one now we might never
send it. At least if we send one then the client might have some chance
to get back to a sane state. And this does at least take us back to
behaviour before the stop filtering patch.

Could go further and only stop the machine if it was a 3, or send a
stop packet even if we were stopped, etc. but all that get further from
a minimal fix.

Thanks,
Nick

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Joel Stanley]

On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > >
> > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > index 6911b73c07..ce8b42eb15 100644
> > > --- a/gdbstub/gdbstub.c
> > > +++ b/gdbstub/gdbstub.c
> > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > >              return;
> > >      }
> > >      if (runstate_is_running()) {
> > > -        /* when the CPU is running, we cannot do anything except stop
> > > -           it when receiving a char */
> > > +        /*
> > > +         * When the CPU is running, we cannot do anything except stop
> > > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > > +         * here, but it does expect a stop reply.
> > > +         */
> > > +        if (ch != 0x03) {
> > > +            warn_report("gdbstub: client sent packet while target running\n");
> > > +        }
> > > +        gdbserver_state.allow_stop_reply = true;
> > >          vm_stop(RUN_STATE_PAUSED);
> > >      } else
> > >  #endif
> >
> > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > Ctrl+C/0x03?
>
> Good question.
>
> I think if we get a character here that's not a 3, we're already in
> trouble, and we eat it so even worse. Since we only send a stop packet
> back when the vm stops, then if we don't send one now we might never
> send it. At least if we send one then the client might have some chance
> to get back to a sane state. And this does at least take us back to
> behaviour before the stop filtering patch.
>
> Could go further and only stop the machine if it was a 3, or send a
> stop packet even if we were stopped, etc. but all that get further from
> a minimal fix.

I was taking a look at -rc1 and it looks like this hasn't made it in.
Is it something we want to propose including?

As a user of qemu I'd vote for it to go in.

Cheers,

Joel

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Nicholas Piggin]

On Wed Jul 26, 2023 at 4:35 PM AEST, Joel Stanley wrote:
> On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > > >
> > > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > > index 6911b73c07..ce8b42eb15 100644
> > > > --- a/gdbstub/gdbstub.c
> > > > +++ b/gdbstub/gdbstub.c
> > > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > > >              return;
> > > >      }
> > > >      if (runstate_is_running()) {
> > > > -        /* when the CPU is running, we cannot do anything except stop
> > > > -           it when receiving a char */
> > > > +        /*
> > > > +         * When the CPU is running, we cannot do anything except stop
> > > > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > > > +         * here, but it does expect a stop reply.
> > > > +         */
> > > > +        if (ch != 0x03) {
> > > > +            warn_report("gdbstub: client sent packet while target running\n");
> > > > +        }
> > > > +        gdbserver_state.allow_stop_reply = true;
> > > >          vm_stop(RUN_STATE_PAUSED);
> > > >      } else
> > > >  #endif
> > >
> > > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > > Ctrl+C/0x03?
> >
> > Good question.
> >
> > I think if we get a character here that's not a 3, we're already in
> > trouble, and we eat it so even worse. Since we only send a stop packet
> > back when the vm stops, then if we don't send one now we might never
> > send it. At least if we send one then the client might have some chance
> > to get back to a sane state. And this does at least take us back to
> > behaviour before the stop filtering patch.
> >
> > Could go further and only stop the machine if it was a 3, or send a
> > stop packet even if we were stopped, etc. but all that get further from
> > a minimal fix.
>
> I was taking a look at -rc1 and it looks like this hasn't made it in.
> Is it something we want to propose including?
>
> As a user of qemu I'd vote for it to go in.

I think it should, gdb is hardly usable without it.

Thanks,
Nick

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Joel Stanley]

On Sun, 30 Jul 2023 at 09:43, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed Jul 26, 2023 at 4:35 PM AEST, Joel Stanley wrote:
> > On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:
> > >
> > > On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > >
> > > > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > > > index 6911b73c07..ce8b42eb15 100644
> > > > > --- a/gdbstub/gdbstub.c
> > > > > +++ b/gdbstub/gdbstub.c
> > > > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > > > >              return;
> > > > >      }
> > > > >      if (runstate_is_running()) {
> > > > > -        /* when the CPU is running, we cannot do anything except stop
> > > > > -           it when receiving a char */
> > > > > +        /*
> > > > > +         * When the CPU is running, we cannot do anything except stop
> > > > > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > > > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > > > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > > > > +         * here, but it does expect a stop reply.
> > > > > +         */
> > > > > +        if (ch != 0x03) {
> > > > > +            warn_report("gdbstub: client sent packet while target running\n");
> > > > > +        }
> > > > > +        gdbserver_state.allow_stop_reply = true;
> > > > >          vm_stop(RUN_STATE_PAUSED);
> > > > >      } else
> > > > >  #endif
> > > >
> > > > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > > > Ctrl+C/0x03?
> > >
> > > Good question.
> > >
> > > I think if we get a character here that's not a 3, we're already in
> > > trouble, and we eat it so even worse. Since we only send a stop packet
> > > back when the vm stops, then if we don't send one now we might never
> > > send it. At least if we send one then the client might have some chance
> > > to get back to a sane state. And this does at least take us back to
> > > behaviour before the stop filtering patch.
> > >
> > > Could go further and only stop the machine if it was a 3, or send a
> > > stop packet even if we were stopped, etc. but all that get further from
> > > a minimal fix.
> >
> > I was taking a look at -rc1 and it looks like this hasn't made it in.
> > Is it something we want to propose including?
> >
> > As a user of qemu I'd vote for it to go in.
>
> I think it should, gdb is hardly usable without it.

I think I hit this issue when debugging u-boot in the aspeed arm
machines. Your patch fixed things:

Tested-by: Joel Stanley <joel@jms.id.au>

Alex, Philippe, can we get this queued for 8.1?

Cheers,

Joel

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Peter Maydell]

On Mon, 31 Jul 2023 at 07:59, Joel Stanley <joel@jms.id.au> wrote:
>
> On Sun, 30 Jul 2023 at 09:43, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed Jul 26, 2023 at 4:35 PM AEST, Joel Stanley wrote:
> > > On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:
> > > >
> > > > On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > > > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > > > > >
> > > > > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > > > > index 6911b73c07..ce8b42eb15 100644
> > > > > > --- a/gdbstub/gdbstub.c
> > > > > > +++ b/gdbstub/gdbstub.c
> > > > > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > > > > >              return;
> > > > > >      }
> > > > > >      if (runstate_is_running()) {
> > > > > > -        /* when the CPU is running, we cannot do anything except stop
> > > > > > -           it when receiving a char */
> > > > > > +        /*
> > > > > > +         * When the CPU is running, we cannot do anything except stop
> > > > > > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > > > > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > > > > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > > > > > +         * here, but it does expect a stop reply.
> > > > > > +         */
> > > > > > +        if (ch != 0x03) {
> > > > > > +            warn_report("gdbstub: client sent packet while target running\n");
> > > > > > +        }
> > > > > > +        gdbserver_state.allow_stop_reply = true;
> > > > > >          vm_stop(RUN_STATE_PAUSED);
> > > > > >      } else
> > > > > >  #endif
> > > > >
> > > > > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > > > > Ctrl+C/0x03?
> > > >
> > > > Good question.
> > > >
> > > > I think if we get a character here that's not a 3, we're already in
> > > > trouble, and we eat it so even worse. Since we only send a stop packet
> > > > back when the vm stops, then if we don't send one now we might never
> > > > send it. At least if we send one then the client might have some chance
> > > > to get back to a sane state. And this does at least take us back to
> > > > behaviour before the stop filtering patch.
> > > >
> > > > Could go further and only stop the machine if it was a 3, or send a
> > > > stop packet even if we were stopped, etc. but all that get further from
> > > > a minimal fix.
> > >
> > > I was taking a look at -rc1 and it looks like this hasn't made it in.
> > > Is it something we want to propose including?
> > >
> > > As a user of qemu I'd vote for it to go in.
> >
> > I think it should, gdb is hardly usable without it.
>
> I think I hit this issue when debugging u-boot in the aspeed arm
> machines. Your patch fixed things:
>
> Tested-by: Joel Stanley <joel@jms.id.au>
>
> Alex, Philippe, can we get this queued for 8.1?

I'm doing a pullreq today anyway, so I can grab it.

thanks
-- PMM

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Philippe Mathieu-Daudé]

On 31/7/23 15:59, Peter Maydell wrote:
> On Mon, 31 Jul 2023 at 07:59, Joel Stanley <joel@jms.id.au> wrote:
>>
>> On Sun, 30 Jul 2023 at 09:43, Nicholas Piggin <npiggin@gmail.com> wrote:
>>>
>>> On Wed Jul 26, 2023 at 4:35 PM AEST, Joel Stanley wrote:
>>>> On Wed, 12 Jul 2023 at 02:12, Nicholas Piggin <npiggin@gmail.com> wrote:


>>>> I was taking a look at -rc1 and it looks like this hasn't made it in.
>>>> Is it something we want to propose including?
>>>>
>>>> As a user of qemu I'd vote for it to go in.
>>>
>>> I think it should, gdb is hardly usable without it.
>>
>> I think I hit this issue when debugging u-boot in the aspeed arm
>> machines. Your patch fixed things:
>>
>> Tested-by: Joel Stanley <joel@jms.id.au>
>>
>> Alex, Philippe, can we get this queued for 8.1?
> 
> I'm doing a pullreq today anyway, so I can grab it.

Thank you Peter!

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Matheus Tavares Bernardino]

Hi, Nick.

> Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > Nicholas Piggin <npiggin@gmail.com> wrote:
> > >
> > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > index 6911b73c07..ce8b42eb15 100644
> > > --- a/gdbstub/gdbstub.c
> > > +++ b/gdbstub/gdbstub.c
> > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > >              return;
> > >      }
> > >      if (runstate_is_running()) {
> > > -        /* when the CPU is running, we cannot do anything except stop
> > > -           it when receiving a char */
> > > +        /*
> > > +         * When the CPU is running, we cannot do anything except stop
> > > +         * it when receiving a char. This is expected on a Ctrl-C in the
> > > +         * gdb client. Because we are in all-stop mode, gdb sends a
> > > +         * 0x03 byte which is not a usual packet, so we handle it specially
> > > +         * here, but it does expect a stop reply.
> > > +         */
> > > +        if (ch != 0x03) {
> > > +            warn_report("gdbstub: client sent packet while target running\n");
> > > +        }
> > > +        gdbserver_state.allow_stop_reply = true;
> > >          vm_stop(RUN_STATE_PAUSED);
> > >      } else
> > >  #endif
> >
> > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > Ctrl+C/0x03?
> 
> Good question.
> 
> I think if we get a character here that's not a 3, we're already in
> trouble, and we eat it so even worse. Since we only send a stop packet
> back when the vm stops, then if we don't send one now we might never
> send it. At least if we send one then the client might have some chance
> to get back to a sane state.

I just noticed now (as I was integrating the latest upstream patches
with our downstream qemu-system-hexagon) that this causes the
gdbstub-untimely-packet tcg test to fail.

My first thought was that, if 0x3 is the only valid case where we will
read a char when the cpu is running, perhaps not issuing the stop-reply
isn't that bad as GDB would ignore it anyways. E.g. from a `set debug
remote 1` output:

  Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;
                   fork-events+;vfork-events+;exec-events+;vContSupported+;
		   QThreadEvents+;no-resumed+;
		   xmlRegisters=i386#6a...
  Packet instead of Ack, ignoring it

So, perhaps, we could do:

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index f123b40ce7..8af066301a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2055,8 +2055,9 @@ void gdb_read_byte(uint8_t ch)
          */
         if (ch != 0x03) {
             warn_report("gdbstub: client sent packet while target running\n");
+        } else {
+            gdbserver_state.allow_stop_reply = true;
         }
-        gdbserver_state.allow_stop_reply = true;
         vm_stop(RUN_STATE_PAUSED);
     } else
 #endif
-- >8 --

Alternatively, since GDB ignores the packet anyways, should we just let
this be and refactor/remove the test?

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Richard Henderson]

On 8/1/23 11:40, Matheus Tavares Bernardino wrote:
> Hi, Nick.
> 
>> Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
>>>> Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>
>>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>>> index 6911b73c07..ce8b42eb15 100644
>>>> --- a/gdbstub/gdbstub.c
>>>> +++ b/gdbstub/gdbstub.c
>>>> @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
>>>>               return;
>>>>       }
>>>>       if (runstate_is_running()) {
>>>> -        /* when the CPU is running, we cannot do anything except stop
>>>> -           it when receiving a char */
>>>> +        /*
>>>> +         * When the CPU is running, we cannot do anything except stop
>>>> +         * it when receiving a char. This is expected on a Ctrl-C in the
>>>> +         * gdb client. Because we are in all-stop mode, gdb sends a
>>>> +         * 0x03 byte which is not a usual packet, so we handle it specially
>>>> +         * here, but it does expect a stop reply.
>>>> +         */
>>>> +        if (ch != 0x03) {
>>>> +            warn_report("gdbstub: client sent packet while target running\n");
>>>> +        }
>>>> +        gdbserver_state.allow_stop_reply = true;
>>>>           vm_stop(RUN_STATE_PAUSED);
>>>>       } else
>>>>   #endif
>>>
>>> Makes sense to me, but shouldn't we send the stop-reply packet only for
>>> Ctrl+C/0x03?
>>
>> Good question.
>>
>> I think if we get a character here that's not a 3, we're already in
>> trouble, and we eat it so even worse. Since we only send a stop packet
>> back when the vm stops, then if we don't send one now we might never
>> send it. At least if we send one then the client might have some chance
>> to get back to a sane state.
> 
> I just noticed now (as I was integrating the latest upstream patches
> with our downstream qemu-system-hexagon) that this causes the
> gdbstub-untimely-packet tcg test to fail.
> 
> My first thought was that, if 0x3 is the only valid case where we will
> read a char when the cpu is running, perhaps not issuing the stop-reply
> isn't that bad as GDB would ignore it anyways. E.g. from a `set debug
> remote 1` output:
> 
>    Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;
>                     fork-events+;vfork-events+;exec-events+;vContSupported+;
> 		   QThreadEvents+;no-resumed+;
> 		   xmlRegisters=i386#6a...
>    Packet instead of Ack, ignoring it
> 
> So, perhaps, we could do:
> 
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index f123b40ce7..8af066301a 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -2055,8 +2055,9 @@ void gdb_read_byte(uint8_t ch)
>            */
>           if (ch != 0x03) {
>               warn_report("gdbstub: client sent packet while target running\n");
> +        } else {
> +            gdbserver_state.allow_stop_reply = true;
>           }
> -        gdbserver_state.allow_stop_reply = true;
>           vm_stop(RUN_STATE_PAUSED);
>       } else
>   #endif
> -- >8 --
> 
> Alternatively, since GDB ignores the packet anyways, should we just let
> this be and refactor/remove the test?

Ping, Alex and Nick.

I can confirm that Matheus' patch fixes the problem I am seeing.
But I'm also open to removing the test.


r~

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Alex Bennée]

Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes:

> Hi, Nick.
>
>> Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
>> > > Nicholas Piggin <npiggin@gmail.com> wrote:
>> > >
>> > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>> > > index 6911b73c07..ce8b42eb15 100644
>> > > --- a/gdbstub/gdbstub.c
>> > > +++ b/gdbstub/gdbstub.c
>> > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
>> > >              return;
>> > >      }
>> > >      if (runstate_is_running()) {
>> > > -        /* when the CPU is running, we cannot do anything except stop
>> > > -           it when receiving a char */
>> > > +        /*
>> > > +         * When the CPU is running, we cannot do anything except stop
>> > > +         * it when receiving a char. This is expected on a Ctrl-C in the
>> > > +         * gdb client. Because we are in all-stop mode, gdb sends a
>> > > +         * 0x03 byte which is not a usual packet, so we handle it specially
>> > > +         * here, but it does expect a stop reply.
>> > > +         */
>> > > +        if (ch != 0x03) {
>> > > +            warn_report("gdbstub: client sent packet while target running\n");
>> > > +        }
>> > > +        gdbserver_state.allow_stop_reply = true;
>> > >          vm_stop(RUN_STATE_PAUSED);
>> > >      } else
>> > >  #endif
>> >
>> > Makes sense to me, but shouldn't we send the stop-reply packet only for
>> > Ctrl+C/0x03?
>> 
>> Good question.
>> 
>> I think if we get a character here that's not a 3, we're already in
>> trouble, and we eat it so even worse. Since we only send a stop packet
>> back when the vm stops, then if we don't send one now we might never
>> send it. At least if we send one then the client might have some chance
>> to get back to a sane state.
>
> I just noticed now (as I was integrating the latest upstream patches
> with our downstream qemu-system-hexagon) that this causes the
> gdbstub-untimely-packet tcg test to fail.
>
> My first thought was that, if 0x3 is the only valid case where we will
> read a char when the cpu is running, perhaps not issuing the stop-reply
> isn't that bad as GDB would ignore it anyways. E.g. from a `set debug
> remote 1` output:
>
>   Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;
>                    fork-events+;vfork-events+;exec-events+;vContSupported+;
> 		   QThreadEvents+;no-resumed+;
> 		   xmlRegisters=i386#6a...
>   Packet instead of Ack, ignoring it
>
> So, perhaps, we could do:
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index f123b40ce7..8af066301a 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -2055,8 +2055,9 @@ void gdb_read_byte(uint8_t ch)
>           */
>          if (ch != 0x03) {
>              warn_report("gdbstub: client sent packet while target
> running\n");

This warning seems to be triggering either way, investigating now.

> +        } else {
> +            gdbserver_state.allow_stop_reply = true;
>          }
> -        gdbserver_state.allow_stop_reply = true;
>          vm_stop(RUN_STATE_PAUSED);
>      } else
>  #endif
> -- >8 --
>
> Alternatively, since GDB ignores the packet anyways, should we just let
> this be and refactor/remove the test?


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Nicholas Piggin]

On Tue Jul 11, 2023 at 6:59 PM AEST, Nicholas Piggin wrote:
> The gdb remote protocol has a special interrupt character (0x03) that is
> transmitted outside the regular packet processing, and represents a
> Ctrl-C pressed in the client. Despite not being a regular packet, it
> does expect a regular stop response if the stub successfully stops the
> running program.
>
> See: https://sourceware.org/gdb/onlinedocs/gdb/Interrupts.html
>
> Inhibiting the stop reply packet can lead to gdb client hang. So permit
> a stop response when receiving a character from gdb that stops the vm.
> Additionally, add a warning if that was not a 0x03 character, because
> the gdb session is likely to end up getting confused if this happens.
>
> Cc: qemu-stable@nongnu.org

Oh, I should note that this doesn't apply to any stable
branches I'm sorry. Will be more careful with the tag...

Thanks,
Nick

Re: [PATCH] gdbstub: Fix client Ctrl-C handling

[Michael Tokarev]

12.07.2023 05:13, Nicholas Piggin wrote:
> On Tue Jul 11, 2023 at 6:59 PM AEST, Nicholas Piggin wrote:
>> The gdb remote protocol has a special interrupt character (0x03) that is
>> transmitted outside the regular packet processing, and represents a
>> Ctrl-C pressed in the client. Despite not being a regular packet, it
>> does expect a regular stop response if the stub successfully stops the
>> running program.
>>
>> See: https://sourceware.org/gdb/onlinedocs/gdb/Interrupts.html
>>
>> Inhibiting the stop reply packet can lead to gdb client hang. So permit
>> a stop response when receiving a character from gdb that stops the vm.
>> Additionally, add a warning if that was not a 0x03 character, because
>> the gdb session is likely to end up getting confused if this happens.
>>
>> Cc: qemu-stable@nongnu.org
> 
> Oh, I should note that this doesn't apply to any stable
> branches I'm sorry. Will be more careful with the tag...

That's entirely Okay, since the Fixes: tag helps to determine if it fits
or not, and 758370052fb is v8.0.0-803-g758370052f.  It's worse to miss
something important :)

Thank you!

/mjt