[binutils-gdb] gdbserver/linux: Access memory even if threads are running

[binutils-gdb] gdbserver/linux: Access memory even if threads are running

[Pedro Alves]

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

commit 421490af33bfbfe8a8429f0e43fb3e9f8727476e
Author: Pedro Alves <pedro@palves.net>
Date:   Mon Mar 28 18:35:34 2022 +0100

    gdbserver/linux: Access memory even if threads are running
    
    Similarly to how the native Linux target was changed
    and subsequently reworked in these commits:
    
     05c06f318fd9 Linux: Access memory even if threads are running
     8a89ddbda2ec Avoid /proc/pid/mem races (PR 28065)
    
    ... teach GDBserver to access memory even when the current thread is
    running, by always accessing memory via /proc/PID/mem.
    
    The existing comment:
    
      /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
         running LWP.  */
    
    ... is incorrect for /proc/PID/mem does allow that.
    
    Actually, from GDB's perspective, GDBserver could already access
    memory while threads were running, but at the expense of pausing all
    threads for the duration of the memory access, via
    prepare_to_access_memory.  This new implementation does not require
    pausing any thread, thus
    linux_process_target::prepare_to_access_memory /
    linux_process_target::done_accessing_memory become nops.  A subsequent
    patch will remove the whole prepare_to_access_memory infrastructure
    completely.
    
    The GDBserver linux-low.cc implementation is simpler than GDB's
    linux-nat.c's, because GDBserver always adds the unfollowed vfork/fork
    children to the process list immediately when the fork/vfork event is
    seen out of ptrace.  I.e., there's no need to keep the file descriptor
    stored on a side map, we can store it directly in the process
    structure.
    
    Change-Id: I0abfd782ceaa4ddce8d3e5f3e2dfc5928862ef61

Diff:
---
 gdbserver/linux-low.cc | 239 ++++++++++++++++++-------------------------------
 gdbserver/linux-low.h  |  11 ++-
 2 files changed, 93 insertions(+), 157 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index d882072e71e..449c6641f58 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -403,8 +403,22 @@ linux_process_target::low_delete_thread (arch_lwp_info *info)
   gdb_assert (info == nullptr);
 }
 
+/* Open the /proc/PID/mem file for PROC.  */
+
+static void
+open_proc_mem_file (process_info *proc)
+{
+  gdb_assert (proc->priv->mem_fd == -1);
+
+  char filename[64];
+  xsnprintf (filename, sizeof filename, "/proc/%d/mem", proc->pid);
+
+  proc->priv->mem_fd
+    = gdb_open_cloexec (filename, O_RDWR | O_LARGEFILE, 0).release ();
+}
+
 process_info *
-linux_process_target::add_linux_process (int pid, int attached)
+linux_process_target::add_linux_process_no_mem_file (int pid, int attached)
 {
   struct process_info *proc;
 
@@ -412,7 +426,17 @@ linux_process_target::add_linux_process (int pid, int attached)
   proc->priv = XCNEW (struct process_info_private);
 
   proc->priv->arch_private = low_new_process ();
+  proc->priv->mem_fd = -1;
+
+  return proc;
+}
+
 
+process_info *
+linux_process_target::add_linux_process (int pid, int attached)
+{
+  process_info *proc = add_linux_process_no_mem_file (pid, attached);
+  open_proc_mem_file (proc);
   return proc;
 }
 
@@ -932,7 +956,11 @@ linux_process_target::create_inferior (const char *program,
 			 NULL, NULL, NULL, NULL);
   }
 
-  add_linux_process (pid, 0);
+  /* When spawning a new process, we can't open the mem file yet.  We
+     still have to nurse the process through the shell, and that execs
+     a couple times.  The address space a /proc/PID/mem file is
+     accessing is destroyed on exec.  */
+  process_info *proc = add_linux_process_no_mem_file (pid, 0);
 
   ptid = ptid_t (pid, pid);
   new_lwp = add_lwp (ptid);
@@ -940,6 +968,10 @@ linux_process_target::create_inferior (const char *program,
 
   post_fork_inferior (pid, program);
 
+  /* PROC is now past the shell running the program we want, so we can
+     open the /proc/PID/mem file.  */
+  open_proc_mem_file (proc);
+
   return pid;
 }
 
@@ -1095,7 +1127,9 @@ linux_process_target::attach (unsigned long pid)
   ptid_t ptid = ptid_t (pid, pid);
   int err;
 
-  proc = add_linux_process (pid, 1);
+  /* Delay opening the /proc/PID/mem file until we've successfully
+     attached.  */
+  proc = add_linux_process_no_mem_file (pid, 1);
 
   /* Attach to PID.  We will check for other threads
      soon.  */
@@ -1108,6 +1142,8 @@ linux_process_target::attach (unsigned long pid)
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
     }
 
+  open_proc_mem_file (proc);
+
   /* Don't ignore the initial SIGSTOP if we just attached to this
      process.  It will be collected by wait shortly.  */
   initial_thread = find_thread_ptid (ptid_t (pid, pid));
@@ -1542,6 +1578,7 @@ linux_process_target::mourn (process_info *process)
 
   /* Freeing all private data.  */
   priv = process->priv;
+  close (priv->mem_fd);
   low_delete_process (priv->arch_private);
   free (priv);
   process->priv = NULL;
@@ -5309,93 +5346,71 @@ linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
   return the_target->read_memory (memaddr, myaddr, len);
 }
 
-/* Copy LEN bytes from inferior's memory starting at MEMADDR
-   to debugger memory starting at MYADDR.  */
 
-int
-linux_process_target::read_memory (CORE_ADDR memaddr,
-				   unsigned char *myaddr, int len)
+/* Helper for read_memory/write_memory using /proc/PID/mem.  Because
+   we can use a single read/write call, this can be much more
+   efficient than banging away at PTRACE_PEEKTEXT.  Also, unlike
+   PTRACE_PEEKTEXT/PTRACE_POKETEXT, this works with running threads.
+   One an only one of READBUF and WRITEBUF is non-null.  If READBUF is
+   not null, then we're reading, otherwise we're writing.  */
+
+static int
+proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
+		  const gdb_byte *writebuf, int len)
 {
-  int pid = lwpid_of (current_thread);
-  PTRACE_XFER_TYPE *buffer;
-  CORE_ADDR addr;
-  int count;
-  char filename[64];
-  int i;
-  int ret;
-  int fd;
+  gdb_assert ((readbuf == nullptr) != (writebuf == nullptr));
 
-  /* Try using /proc.  Don't bother for one word.  */
-  if (len >= 3 * sizeof (long))
+  process_info *proc = current_process ();
+
+  int fd = proc->priv->mem_fd;
+  if (fd == -1)
+    return EIO;
+
+  while (len > 0)
     {
       int bytes;
 
-      /* We could keep this file open and cache it - possibly one per
-	 thread.  That requires some juggling, but is even faster.  */
-      sprintf (filename, "/proc/%d/mem", pid);
-      fd = open (filename, O_RDONLY | O_LARGEFILE);
-      if (fd == -1)
-	goto no_proc;
-
       /* If pread64 is available, use it.  It's faster if the kernel
 	 supports it (only one syscall), and it's 64-bit safe even on
 	 32-bit platforms (for instance, SPARC debugging a SPARC64
 	 application).  */
 #ifdef HAVE_PREAD64
-      bytes = pread64 (fd, myaddr, len, memaddr);
+      bytes = (readbuf != nullptr
+	       ? pread64 (fd, readbuf, len, memaddr)
+	       : pwrite64 (fd, writebuf, len, memaddr));
 #else
       bytes = -1;
       if (lseek (fd, memaddr, SEEK_SET) != -1)
-	bytes = read (fd, myaddr, len);
+	bytes = (readbuf != nullptr
+		 ? read (fd, readbuf, len)
+		 ? write (fd, writebuf, len));
 #endif
 
-      close (fd);
-      if (bytes == len)
-	return 0;
-
-      /* Some data was read, we'll try to get the rest with ptrace.  */
-      if (bytes > 0)
+      if (bytes < 0)
+	return errno;
+      else if (bytes == 0)
 	{
-	  memaddr += bytes;
-	  myaddr += bytes;
-	  len -= bytes;
+	  /* EOF means the address space is gone, the whole process
+	     exited or execed.  */
+	  return EIO;
 	}
-    }
 
- no_proc:
-  /* Round starting address down to longword boundary.  */
-  addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  count = ((((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-	   / sizeof (PTRACE_XFER_TYPE));
-  /* Allocate buffer of that many longwords.  */
-  buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
-
-  /* Read all the longwords */
-  errno = 0;
-  for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
-    {
-      /* Coerce the 3rd arg to a uintptr_t first to avoid potential gcc warning
-	 about coercing an 8 byte integer to a 4 byte pointer.  */
-      buffer[i] = ptrace (PTRACE_PEEKTEXT, pid,
-			  (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-			  (PTRACE_TYPE_ARG4) 0);
-      if (errno)
-	break;
+      memaddr += bytes;
+      if (readbuf != nullptr)
+	readbuf += bytes;
+      else
+	writebuf += bytes;
+      len -= bytes;
     }
-  ret = errno;
 
-  /* Copy appropriate bytes out of the buffer.  */
-  if (i > 0)
-    {
-      i *= sizeof (PTRACE_XFER_TYPE);
-      i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
-      memcpy (myaddr,
-	      (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	      i < len ? i : len);
-    }
+  return 0;
+}
 
-  return ret;
+int
+linux_process_target::read_memory (CORE_ADDR memaddr,
+				   unsigned char *myaddr, int len)
+{
+  return proc_xfer_memory (memaddr, myaddr, nullptr, len);
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's
@@ -5406,25 +5421,6 @@ int
 linux_process_target::write_memory (CORE_ADDR memaddr,
 				    const unsigned char *myaddr, int len)
 {
-  int i;
-  /* Round starting address down to longword boundary.  */
-  CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  int count
-    = (((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-    / sizeof (PTRACE_XFER_TYPE);
-
-  /* Allocate buffer of that many longwords.  */
-  PTRACE_XFER_TYPE *buffer = XALLOCAVEC (PTRACE_XFER_TYPE, count);
-
-  int pid = lwpid_of (current_thread);
-
-  if (len == 0)
-    {
-      /* Zero length write always succeeds.  */
-      return 0;
-    }
-
   if (debug_threads)
     {
       /* Dump up to four bytes.  */
@@ -5432,7 +5428,7 @@ linux_process_target::write_memory (CORE_ADDR memaddr,
       char *p = str;
       int dump = len < 4 ? len : 4;
 
-      for (i = 0; i < dump; i++)
+      for (int i = 0; i < dump; i++)
 	{
 	  sprintf (p, "%02x", myaddr[i]);
 	  p += 2;
@@ -5440,54 +5436,10 @@ linux_process_target::write_memory (CORE_ADDR memaddr,
       *p = '\0';
 
       threads_debug_printf ("Writing %s to 0x%08lx in process %d",
-			    str, (long) memaddr, pid);
+			    str, (long) memaddr, current_process ()->pid);
     }
 
-  /* Fill start and end extra bytes of buffer with existing memory data.  */
-
-  errno = 0;
-  /* Coerce the 3rd arg to a uintptr_t first to avoid potential gcc warning
-     about coercing an 8 byte integer to a 4 byte pointer.  */
-  buffer[0] = ptrace (PTRACE_PEEKTEXT, pid,
-		      (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-		      (PTRACE_TYPE_ARG4) 0);
-  if (errno)
-    return errno;
-
-  if (count > 1)
-    {
-      errno = 0;
-      buffer[count - 1]
-	= ptrace (PTRACE_PEEKTEXT, pid,
-		  /* Coerce to a uintptr_t first to avoid potential gcc warning
-		     about coercing an 8 byte integer to a 4 byte pointer.  */
-		  (PTRACE_TYPE_ARG3) (uintptr_t) (addr + (count - 1)
-						  * sizeof (PTRACE_XFER_TYPE)),
-		  (PTRACE_TYPE_ARG4) 0);
-      if (errno)
-	return errno;
-    }
-
-  /* Copy data to be written over corresponding part of buffer.  */
-
-  memcpy ((char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  myaddr, len);
-
-  /* Write the entire buffer.  */
-
-  for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
-    {
-      errno = 0;
-      ptrace (PTRACE_POKETEXT, pid,
-	      /* Coerce to a uintptr_t first to avoid potential gcc warning
-		 about coercing an 8 byte integer to a 4 byte pointer.  */
-	      (PTRACE_TYPE_ARG3) (uintptr_t) addr,
-	      (PTRACE_TYPE_ARG4) buffer[i]);
-      if (errno)
-	return errno;
-    }
-
-  return 0;
+  return proc_xfer_memory (memaddr, nullptr, myaddr, len);
 }
 
 void
@@ -6191,25 +6143,6 @@ linux_process_target::unpause_all (bool unfreeze)
   unstop_all_lwps (unfreeze, NULL);
 }
 
-int
-linux_process_target::prepare_to_access_memory ()
-{
-  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
-     running LWP.  */
-  if (non_stop)
-    target_pause_all (true);
-  return 0;
-}
-
-void
-linux_process_target::done_accessing_memory ()
-{
-  /* Neither ptrace nor /proc/PID/mem allow accessing memory through a
-     running LWP.  */
-  if (non_stop)
-    target_unpause_all (true);
-}
-
 /* Extract &phdr and num_phdr in the inferior.  Return 0 on success.  */
 
 static int
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 27cc9641f12..4284fb3348a 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -127,6 +127,9 @@ struct process_info_private
 
   /* &_r_debug.  0 if not yet determined.  -1 if no PT_DYNAMIC in Phdrs.  */
   CORE_ADDR r_debug;
+
+  /* The /proc/pid/mem file used for reading/writing memory.  */
+  int mem_fd;
 };
 
 struct lwp_info;
@@ -163,10 +166,6 @@ public:
 
   void store_registers (regcache *regcache, int regno) override;
 
-  int prepare_to_access_memory () override;
-
-  void done_accessing_memory () override;
-
   int read_memory (CORE_ADDR memaddr, unsigned char *myaddr,
 		   int len) override;
 
@@ -544,6 +543,10 @@ private:
      data.  */
   process_info *add_linux_process (int pid, int attached);
 
+  /* Same as add_linux_process, but don't open the /proc/PID/mem file
+     yet.  */
+  process_info *add_linux_process_no_mem_file (int pid, int attached);
+
   /* Add a new thread.  */
   lwp_info *add_lwp (ptid_t ptid);