Bug 9665 - valid response packet can be treated as 'ENN' error packet
Summary: valid response packet can be treated as 'ENN' error packet
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: remote (show other bugs)
Version: 6.8
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-02 17:08 UTC by richard.stuckey
Modified: 2022-01-07 20:55 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description richard.stuckey 2008-12-02 17:08:01 UTC
[Converted from Gnats 2560]

In file remote.c, the function remote_send is used to send a packet to the remote target and receive a response packet back to it.  It checks whether the response packet is an 'ENN' error response with the test

  if ((*buf)[0] == 'E')
    error (_("Remote failure reply: %s"), *buf);

This test is too weak: if the response packet contains valid data which happens to begin with an 'E' then it will be incorrectly treated as an error.

The correct test is performed in the function  packet_check_result in this file:

      if (buf[0] == 'E'
          && isxdigit (buf[1]) && isxdigit (buf[2])
          && buf[3] == '\0')
        /* "Enn"  - definitly an error.  */
        return PACKET_ERROR;

In fact, this function should be used throughout this file to check all response packets; e.g. in the function remote_rcmd there is the code

      if (buf[0] == '\0')
        error (_("Target does not support this command."));
      if (buf[0] == 'O' && buf[1] != 'K')
        {
          remote_console_output (buf + 1); /* 'O' message from stub.  */
          continue;
        }
      if (strcmp (buf, "OK") == 0)
        break;
      if (strlen (buf) == 3 && buf[0] == 'E'
          && isdigit (buf[1]) && isdigit (buf[2]))
        {
          error (_("Protocol error with Rcmd"));
        }

where the tests essentially duplicate the code in packet_check_result (though strlen is a very inefficient means of checking that the 4th character in a buffer is a NUL!).

Release:
insight 6.8
Comment 1 richard.stuckey 2008-12-02 17:08:01 UTC
Fix:
Replace all checks on the response packet with calls to packet_check_result and check the result of this function call.
Comment 2 drow@false.org 2008-12-02 17:19:13 UTC
From: Daniel Jacobowitz <drow@false.org>
To: richard.stuckey@arc.com
Cc: gdb-gnats@sources.redhat.com
Subject: Re: remote/2560: valid reponse packet can  be treated as 'ENN'
	error packet
Date: Tue, 2 Dec 2008 12:19:13 -0500

 On Tue, Dec 02, 2008 at 05:04:33PM -0000, richard.stuckey@arc.com wrote:
 > This test is too weak: if the response packet contains valid data which happens to begin with an 'E' then it will be incorrectly treated as an error.
 
 remote_send is only used in four cases: p/P and g/G.  If you
 encountered it in one of those, I suggest sending back lowercase
 hex rather than uppercase hex.
 
 That does not invalidate your correct report; merely a suggestion.
 
 -- 
 Daniel Jacobowitz
 CodeSourcery
Comment 3 richard.stuckey 2008-12-02 17:22:38 UTC
From: "Richard Stuckey" <Richard.Stuckey@arc.com>
To: "Daniel Jacobowitz" <drow@false.org>
Cc: <gdb-gnats@sources.redhat.com>
Subject: RE: remote/2560: valid reponse packet can  be treated as 'ENN'error packet
Date: Tue, 2 Dec 2008 17:22:38 -0000

 Yes, it was getting a register frame: the first hex digit for the first
 register was 'E' !
 
 Good idea about the lowercase.
 
     Thanks
 
 -----Original Message-----
 From: Daniel Jacobowitz [mailto:drow@false.org]=20
 Sent: 02 December 2008 17:19
 To: Richard Stuckey
 Cc: gdb-gnats@sources.redhat.com
 Subject: Re: remote/2560: valid reponse packet can be treated as
 'ENN'error packet
 
 On Tue, Dec 02, 2008 at 05:04:33PM -0000, richard.stuckey@arc.com wrote:
 > This test is too weak: if the response packet contains valid data
 which happens to begin with an 'E' then it will be incorrectly treated
 as an error.
 
 remote_send is only used in four cases: p/P and g/G.  If you
 encountered it in one of those, I suggest sending back lowercase
 hex rather than uppercase hex.
 
 That does not invalidate your correct report; merely a suggestion.
 
 --=20
 Daniel Jacobowitz
 CodeSourcery
 

Comment 4 Sourceware Commits 2018-04-26 22:52:27 UTC
The master branch has been updated by Pedro Alves <palves@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b75abf5bb636869fd893ecf98414b8b2fe0d4a12

commit b75abf5bb636869fd893ecf98414b8b2fe0d4a12
Author: Andrzej Kaczmarek <andrzej.kaczmarek@codecoup.pl>
Date:   Thu Apr 26 23:47:25 2018 +0100

    Fix remote 'g' command error handling (PR remote/9665)
    
    'g' command returns hex-string as response so simply checking for 'E'
    to determine if it failed is not enough and can trigger spurious error
    messages.  For example, invalid behaviour can be easily triggered on
    Cortex-M as follows:
    
      (gdb) set $r0 = 0xe0
      Sending packet: $P0=e0000000#72...Packet received: OK
      Packet P (set-register) is supported
      Sending packet: $g#67...Packet received: E0000000849A0020...
      Remote failure reply: E0000000849A0020...
    
    This patch fixes the problem by calling putpkt()/getpkt() directly and
    checking result with packet_check_result().  This works fine since Enn
    response has odd number of bytes while proper response has even number
    of bytes.
    
    Also, remote_send() is now not used anywhere so it can be removed.
    
    gdb/Changelog:
    2018-04-26  Andrzej Kaczmarek  <andrzej.kaczmarek@codecoup.pl>
    
    	PR remote/9665
    	* remote.c (send_g_packet): Use putpkt/getpkt/packet_check_result
    	instead of remote_send.
    	(remote_send): Remove.
Comment 5 Tom Tromey 2022-01-07 20:55:00 UTC
Sounds like this was fixed in 2018.