[PATCH] gdb/remote: fix qRcmd error handling

[PATCH] gdb/remote: fix qRcmd error handling

[Andrew Burgess]

This commit:

  commit 3623271997a5c0d79609aa6a1f35ef61b4469054
  Date:   Tue Jan 30 15:55:47 2024 +0100

      remote.c: Use packet_check_result

Introduced a bug in the error handling of the qRcmd packet.  Prior to
this commit if a packet had status PACKET_OK then, if the packet
contained the text "OK" we considered the packet handled.  But, if the
packet contained any other content (that was not an error message)
then the content was printed to the user.

After the above commit this was no longer the case, any non-error
packet that didn't contain "OK" would be treated as an error.

Currently, gdbserver doesn't exercise this path so it's not possible
to write a simple test for this case.  When gdbserver wishes to print
output it sends back an 'O' string output packet, these packets are
handled earlier in the process.  Then once gdbserver has finished
sending output an 'OK' packet is sent.
---
 gdb/remote.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index cfb54de157d..ee4b76e290b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11954,18 +11954,23 @@ remote_target::rcmd (const char *command, struct ui_file *outbuf)
 	  continue;
 	}
       packet_result result = packet_check_result (buf, false);
-      if (strcmp (buf, "OK") == 0)
-	break;
-      else if (result.status () == PACKET_UNKNOWN)
-	error (_("Target does not support this command."));
-      else
-	error (_("Protocol error with Rcmd: %s."), result.err_msg ());
-
-      for (p = buf; p[0] != '\0' && p[1] != '\0'; p += 2)
+      switch (result.status ())
 	{
-	  char c = (fromhex (p[0]) << 4) + fromhex (p[1]);
+	case PACKET_UNKNOWN:
+	  error (_("Target does not support this command."));
+	case PACKET_ERROR:
+	  error (_("Protocol error with Rcmd: %s."), result.err_msg ());
+	case PACKET_OK:
+	  break;
+	}
 
-	  gdb_putc (c, outbuf);
+      if (strcmp (buf, "OK") != 0)
+	{
+	  for (p = buf; p[0] != '\0' && p[1] != '\0'; p += 2)
+	    {
+	      char c = (fromhex (p[0]) << 4) + fromhex (p[1]);
+	      gdb_putc (c, outbuf);
+	    }
 	}
       break;
     }

base-commit: 1f984aabf17f558d04d3cf1c1b643fd44e8348e8
-- 
2.25.4

Re: [PATCH] gdb/remote: fix qRcmd error handling

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Introduced a bug in the error handling of the qRcmd packet.  Prior to
> Andrew> this commit if a packet had status PACKET_OK then, if the packet
> Andrew> contained the text "OK" we considered the packet handled.  But, if the
> Andrew> packet contained any other content (that was not an error message)
> Andrew> then the content was printed to the user.
>
> Andrew> After the above commit this was no longer the case, any non-error
> Andrew> packet that didn't contain "OK" would be treated as an error.
>
> The patch itself seems reasonable to me, but at the same time, isn't the
> behavior you describe also fine?
>
> The docs describe "OK", other "O" output packets, the "E" response, and
> the empty response.  It seems to me that any other response could
> reasonably be treated as an error.

The behaviour I'm restoring is documented, see the 'OUTPUT' entry in the
'Reply:' list:

  'qRcmd,COMMAND'
       COMMAND (hex encoded) is passed to the local interpreter for
       execution.  Invalid commands should be reported using the output
       string.  Before the final result packet, the target may also
       respond with a number of intermediate 'OOUTPUT' console output
       packets.  _Implementors should note that providing access to a
       stubs's interpreter may have security implications_.
  
       Reply:
       'OK'
            A command response with no output.
       'OUTPUT'
            A command response with the hex encoded output string OUTPUT.
       'E NN'
            Indicate a badly formed request.  The error number NN is given
            as hex digits.
       ''
            An empty reply indicates that 'qRcmd' is not recognized.

This behaviour has existed since commit 96baa820df8126 the "initial
import" commit, and was only broken by accident in commit 3623271997a5 2
weeks ago.

So I think we should restore this behaviour, ideally before the next
release branches.

Thanks,
Andrew

Re: [PATCH] gdb/remote: fix qRcmd error handling

[Tom Tromey]

>> The patch itself seems reasonable to me, but at the same time, isn't the
>> behavior you describe also fine?
>> 
>> The docs describe "OK", other "O" output packets, the "E" response, and
>> the empty response.  It seems to me that any other response could
>> reasonably be treated as an error.

Andrew> The behaviour I'm restoring is documented, see the 'OUTPUT' entry in the
Andrew> 'Reply:' list:

Andrew>        'OUTPUT'
Andrew>             A command response with the hex encoded output string OUTPUT.

I thought your patch was concerned with changing the output of responses
other than 'E' or 'O'... did I misunderstand that?

Tom

Re: [PATCH] gdb/remote: fix qRcmd error handling

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>> The patch itself seems reasonable to me, but at the same time, isn't the
>>> behavior you describe also fine?
>>> 
>>> The docs describe "OK", other "O" output packets, the "E" response, and
>>> the empty response.  It seems to me that any other response could
>>> reasonably be treated as an error.
>
> Andrew> The behaviour I'm restoring is documented, see the 'OUTPUT' entry in the
> Andrew> 'Reply:' list:
>
> Andrew>        'OUTPUT'
> Andrew>             A command response with the hex encoded output string OUTPUT.
>
> I thought your patch was concerned with changing the output of responses
> other than 'E' or 'O'... did I misunderstand that?

I don't think so.  I guess I didn't explain myself very well.

After GDB sends a qRcmd packet to the remote, the remote can reply with
zero or more 'O' packets.  This patch is not about these replies; they
are still working fine.

The remote can also reply with an 'E' packet.  This patch is not about
these error reply packets; they are still working fine.

The remote can also reply with an empty packet indicating the qRcmd is
not supported.  This patch is not about this type of reply; this is
still working fine.

The remote can also reply with a packet containing the string "OK", this
is different from a general 'O' packet, this indicates that the remote
is done handling the qRcmd and so GDB is done too.  This patch is not
about this reply; this is still working fine.

The remote can also reply with a random hex encoded string, of the kind
that would appear after the 'O' in an 'O' packet, but without the adding
the leading 'O' character.  When GDB gets one of these packets GDB
decodes the string and prints it, after which the qRcmd is done.  This
patch is about this behaviour only.

This feature was broken in commit 3623271997a5c0d, after this the hex
encoded string would be treated as an error.  In fact, what happens is
that GDB calls packet_result::err_msg() which will then throw an
assertion error because the packet status is PACKET_OK, not
PACKET_ERROR.

Thanks,
Andrew
  

Re: [PATCH] gdb/remote: fix qRcmd error handling

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Currently, gdbserver doesn't exercise this path so it's not possible
Andrew> to write a simple test for this case.  When gdbserver wishes to print
Andrew> output it sends back an 'O' string output packet, these packets are
Andrew> handled earlier in the process.  Then once gdbserver has finished
Andrew> sending output an 'OK' packet is sent.

I think I must have misread this patch earlier.  Thanks for your
patience & explanations.  I think it is ok.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] gdb/remote: fix qRcmd error handling

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Currently, gdbserver doesn't exercise this path so it's not possible
> Andrew> to write a simple test for this case.  When gdbserver wishes to print
> Andrew> output it sends back an 'O' string output packet, these packets are
> Andrew> handled earlier in the process.  Then once gdbserver has finished
> Andrew> sending output an 'OK' packet is sent.
>
> I think I must have misread this patch earlier.  Thanks for your
> patience & explanations.

Not a problem.

>                           I think it is ok.
>
> Approved-By: Tom Tromey <tom@tromey.com>

I pushed this.

Thanks,
Andrew