From 366e3746c572c2c78454761e62fa9181cba413ca Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 31 Mar 2022 22:04:42 +0100
Subject: [PATCH] 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
---
 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);
-- 
2.30.2