[Qemu-devel] gdb run / pause broken for some debuggers

[Qemu-devel] gdb run / pause broken for some debuggers

[Lucien Anti-Spam]

Hi gdbstub maintainers (CC: Luc Michel as he wrote the patch that probably caused this issue)
I have found an issue with IDA Pro when moving to QEMU3.x where RUN/PAUSE has stopped working.
I am pretty sure its to do with this debugger sending no thread-id with the vCont packet action
That is that IDA Pro sends "vCont;c" (action=c, thread-id=none).  
This is defined in the RSP document as "An action with no thread-id matches all threads."
(https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet )
In commit e40e5204af8388e605df2325d9b562c05919350e, which added multiprocess support for vCont packets, the new code errors right away if it finds no ":" action\thread-id separator as shown below:
 if (*p++ != ':') {     res = -ENOTSUP;...
The upper function calling gdb_handle_vcont when -EINVAL or -ERANGE will spit out an "E22" error message, but for -ENOTSUP  we do goto-unknown_command label results in IDA Pro thinking the gdbstub is dead.
            res = gdb_handle_vcont(s, p);            if (res) {                if ((res == -EINVAL) || (res == -ERANGE)) {                    put_packet(s, "E22");                ... else goto unknown_command:
Maybe we should extend this to add a "-ENOTSUP" clause so the "E22" is also returned?
This "unknown_command:" branch should probably send a "E nn" packet rather than just a blank "+" ack although here I do admit the spec is very unclear but in most cases they do spec out "E nn" when some part of the command turns to garbage. 
...        unknown_command:            /* put empty packet */           buf[0] = '\0';           put_packet(s, buf);


If people more experienced in this agree I will go ahead and try at a patch for both of these issues.
Cheers,Luc

Re: [Qemu-devel] gdb run / pause broken for some debuggers

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 2777 bytes --]

Hi,

On 1/24/19 3:25 PM, Lucien Anti-Spam wrote:
> Hi gdbstub maintainers (CC: Luc Michel as he wrote the patch that
> probably caused this issue)
> 
> I have found an issue with IDA Pro when moving to QEMU3.x where
> RUN/PAUSE has stopped working.
> 
> I am pretty sure its to do with this debugger sending no thread-id with
> the vCont packet action
> That is that IDA Pro sends "vCont;c" (action=c, thread-id=none).  
> 
> This is defined in the RSP document as "An action with
> no thread-id matches all threads."
> (https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet )
> 
> In commit e40e5204af8388e605df2325d9b562c05919350e, which added
> multiprocess support for vCont packets, the new code errors right away
> if it finds no ":" action\thread-id separator as shown below:
> 
>  if (*p++ != ':') {
>      res = -ENOTSUP;
> ...
Yes, I think you are right, we should fall into the GDB_ALL_PROCESSES
case here.

> 
> The upper function calling gdb_handle_vcont when -EINVAL or -ERANGE will
> spit out an "E22" error message, but for -ENOTSUP  we do
> goto-unknown_command label results in IDA Pro thinking the gdbstub is dead.
> 
>             res = gdb_handle_vcont(s, p);
>             if (res) {
>                 if ((res == -EINVAL) || (res == -ERANGE)) {
>                     put_packet(s, "E22");
>                 ... else goto unknown_command:
> 
> Maybe we should extend this to add a "-ENOTSUP" clause so the "E22" is
> also returned?
> 
> This "unknown_command:" branch should probably send a "E nn" packet
> rather than just a blank "+" ack although here I do admit the spec is
> very unclear but in most cases they do spec out "E nn" when some part of
> the command turns to garbage. 
> 
> ...
>         unknown_command:
>             /* put empty packet */
>            buf[0] = '\0';
>            put_packet(s, buf);

From the spec:

"For any command not supported by the stub, an empty response (‘$#00’)
should be returned. That way it is possible to extend the protocol. A
newer GDB can tell if a packet is supported based on that response."

And looking into gdbserver GDB sources, they do the same thing:

    default:
      /* It is a request we don't understand.  Respond with an empty
	 packet so that gdb knows that we don't support this
	 request.  */
      own_buf[0] = '\0';

So this is probably IDA here which does not correctly handle this "not
supported" response (which should not happen anyway).

Cheers,
Luc

> 
> 
> 
> If people more experienced in this agree I will go ahead and try at a
> patch for both of these issues.
> 
> Cheers,
> Luc
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]