[binutils-gdb] gdbserver: special case target_write_memory len==0

[binutils-gdb] gdbserver: special case target_write_memory len==0

[Pedro Alves]

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

commit 366e3746c572c2c78454761e62fa9181cba413ca
Author: Pedro Alves <pedro@palves.net>
Date:   Thu Mar 31 22:04:42 2022 +0100

    gdbserver: special case target_write_memory len==0
    
    The next patch in this series adds a common helper routine for both
    memory reads and writes, like this:
    
     static int
     proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
                      const gdb_byte *writebuf, int len)
     {
       gdb_assert ((readbuf == nullptr) != (writebuf == nullptr));
       ...
     }
    
     int
     linux_process_target::read_memory (CORE_ADDR memaddr,
                                        unsigned char *myaddr, int len)
     {
       return proc_xfer_memory (memaddr, myaddr, nullptr, len);
     }
    
     linux_process_target::write_memory (CORE_ADDR memaddr,
                                        const unsigned char *myaddr, int len)
     {
       return proc_xfer_memory (memaddr, nullptr, myaddr, len);
     }
    
    Surprisingly, the assertion fails.  That happens because it can happen
    that target_write_memory is called with LEN==0, due to this in
    gdb/remote.c:
    
     /* Determine whether the remote target supports binary downloading.
        This is accomplished by sending a no-op memory write of zero length
        to the target at the specified address. (...) */
    
     void
     remote_target::check_binary_download (CORE_ADDR addr)
     {
     ...
           p = rs->buf.data ();
           *p++ = 'X';
           p += hexnumstr (p, (ULONGEST) addr);
           *p++ = ',';
           p += hexnumstr (p, (ULONGEST) 0);
           *p++ = ':';
           *p = '\0';
    
    In this scenario, in gdbserver's target_write_memory, the "myaddr"
    argument of the_target->write_memory is passed the data() of a local
    gdb::byte_vector (which is a specialized std::vector).  It's valid for
    std::vector::data() to return NULL when the vector is empty.
    
    This commit adds an early return to target_write_memory to avoid
    target backends having to care about this.  For good measure, do the
    same on the read side, in read_inferior_memory.
    
    Change-Id: Iac8f04fcf99014c624ef4036bd318ca1771ad491

Diff:
---
 gdbserver/target.cc | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/gdbserver/target.cc b/gdbserver/target.cc
index 5009146d663..e9d1e1aa38c 100644
--- a/gdbserver/target.cc
+++ b/gdbserver/target.cc
@@ -124,8 +124,14 @@ done_accessing_memory (void)
 int
 read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
-  int res;
-  res = the_target->read_memory (memaddr, myaddr, len);
+  /* At the time of writing, GDB only sends write packets with LEN==0,
+     not read packets (see comment in target_write_memory), but it
+     doesn't hurt to prevent problems if it ever does, or we're
+     connected to some client other than GDB that does.  */
+  if (len == 0)
+    return 0;
+
+  int res = the_target->read_memory (memaddr, myaddr, len);
   check_mem_read (memaddr, myaddr, len);
   return res;
 }
@@ -152,6 +158,13 @@ int
 target_write_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
 		     ssize_t len)
 {
+  /* GDB may send X packets with LEN==0, for probing packet support.
+     If we let such a request go through, then buffer.data() below may
+     return NULL, which may confuse target implementations.  Handle it
+     here to avoid lower levels having to care about this case.  */
+  if (len == 0)
+    return 0;
+
   /* Make a copy of the data because check_mem_write may need to
      update it.  */
   gdb::byte_vector buffer (myaddr, myaddr + len);