[PATCH] Linux/gdbserver: Fix memory read ptrace fallback issues

["Maciej W. Rozycki" <macro at codesourcery dot com>]

Hi,

 While trying to investigate suspicious MIPS16/Linux test suite results 
noted with the recent MIPS ISA bit solution proposal I have come across 
this problem in gdbserver.  It contributed to about 2550 failures per 
multilib.

 In linux_read_memory we make a two-way choice as to how we read 
debuggee's memory -- we prefer pread64 or lseek/read of /proc/<pid>/mem 
and failing that we resort to a long boring series of PEEKTEXT ptrace 
requests.  We use this in particular in linux_qxfer_libraries_svr4 to 
access names of shared libraries loaded.  There we rely on 
linux_read_memory to return data in the buffer provided even if the 
function wasn't able to read the whole number of bytes requested.

 This has two problems as I revealed.  If any call in the pread64 fails 
then the supplied buffer is obviously not filled.  Then if the ptrace 
fallback path is not able to retrieve the whole number of bytes requested, 
then it does not supply partially retrieved data to the buffer.  This is 
the scenario that I observed.

 Gdbserver tried to retrieved the name of the dynamic linker that is 
normally linked to ld.so's internal link map from the PT_INTERP segment of 
the executable debugged.  This segment is typically very short, it can 
take a single page only.  Gdbserver wants to read 4095 (PATH_MAX) bytes 
and the offset of the name into the page PT_INTERP is mapped into is 
already 0x134.  That plus 4095 goes beyond the page:

$ mips-linux-gnu-readelf -l func-ptrs

Elf file type is EXEC (Executable file)
Entry point 0x4005c1
There are 8 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  PHDR           0x000034 0x00400034 0x00400034 0x00100 0x00100 R E 0x4
  INTERP         0x000134 0x00400134 0x00400134 0x0008c 0x0008c R   0x1
      [Requesting program interpreter: .../mips16/lib/ld.so.1]
  REGINFO        0x0001e0 0x004001e0 0x004001e0 0x00018 0x00018 R   0x4
  LOAD           0x000000 0x00400000 0x00400000 0x007f4 0x007f4 R E 0x10000
  LOAD           0x0007f4 0x004107f4 0x004107f4 0x00078 0x0008c RW  0x10000
  DYNAMIC        0x0001f8 0x004001f8 0x004001f8 0x00108 0x00108 RWE 0x4
  NOTE           0x0001c0 0x004001c0 0x004001c0 0x00020 0x00020 R   0x4
  NULL           0x000000 0x00000000 0x00000000 0x00000 0x00000     0x4
[...]
$ cat /proc/1234/maps
00400000-00401000 r-xp 00000000 00:15 6838968    .../gdb.base/func-ptrs
00410000-00411000 rw-p 00000000 00:15 6838968    .../gdb.base/func-ptrs
2aaa8000-2aac2000 r-xp 00000000 00:15 11153755   .../mips16/lib/ld-2.15.so
2aac2000-2aac5000 rw-p 00000000 00:00 0
2aad1000-2aad2000 r--p 00019000 00:15 11153755   .../mips16/lib/ld-2.15.so
2aad2000-2aad3000 rw-p 0001a000 00:15 11153755   .../mips16/lib/ld-2.15.so
[...]
$

so linux_read_memory can at most read 0x401000 - 0x400134 = 3788 bytes.  
If the pread64 path fails, e.g. /proc not mounted, then it falls back to 
the ptrace loop that eventually fails too, because it can't read beyond 
offset 3788.  The temporary buffer used by the ptrace loop is then not 
copied to the user buffer and linux_qxfer_libraries_svr4 receives nothing.

 The second problem is if /proc is indeed mounted and pread64 succeeds, 
then linux_read_memory will fall back to ptrace anyway, because in this 
case pread64 will return 3788 bytes that is different to the number 
requested.  Then the ptrace loop will go back to retrieving data from the 
beginning, again fail after 3788 have been read, ignore its temporary 
buffer and linux_qxfer_libraries_svr4 will get what pread64 has already 
retrieved -- waste of time.

 The change below addresses these two problems.  Any data successfully 
retrieved by the ptrace loop is copied over to the user buffer even if 
linux_read_memory returns unsuccessfully.  If pread64 successfully 
retrieves any data, then ptrace is not used to read that data again, the 
attempt to read data is resumed at the point where pread64 stopped.  This 
will normally not retrieve any further data as pread64 would have provided 
that (internally, in the Linux kernel, the read handlers for 
/proc/<pid>/maps special files are implemented as a wrapper around the 
very same worker function that the PEEKTEXT ptrace request uses.

 I have successfully regression-tested this change with the i686-linux-gnu 
and mips-linux-gnu targets.  OK to apply?

2012-05-16  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/gdbserver/
	* linux-low.c (linux_store_registers): Don't re-retrieve data
	with ptrace that has already been obtained from /proc.  Always
	copy any data retrieved with ptrace to the buffer supplied.

  Maciej

gdb-gdbserver-linux-read-memory-ptrace.diff
Index: gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbserver/linux-low.c	2012-05-16 17:12:48.000000000 +0100
+++ gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c	2012-05-16 20:26:34.415575387 +0100
@@ -4356,23 +4356,19 @@ linux_store_registers (struct regcache *
 static int
 linux_read_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
 {
+  int pid = lwpid_of (get_thread_lwp (current_inferior));
+  register PTRACE_XFER_TYPE *buffer;
+  register CORE_ADDR addr;
+  register int count;
+  char filename[64];
   register int i;
-  /* Round starting address down to longword boundary.  */
-  register CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
-  /* Round ending address up; get number of longwords that makes.  */
-  register int count
-    = (((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
-      / sizeof (PTRACE_XFER_TYPE);
-  /* Allocate buffer of that many longwords.  */
-  register PTRACE_XFER_TYPE *buffer
-    = (PTRACE_XFER_TYPE *) alloca (count * sizeof (PTRACE_XFER_TYPE));
   int fd;
-  char filename[64];
-  int pid = lwpid_of (get_thread_lwp (current_inferior));
 
   /* Try using /proc.  Don't bother for one word.  */
   if (len >= 3 * sizeof (long))
     {
+      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);
@@ -4385,38 +4381,55 @@ linux_read_memory (CORE_ADDR memaddr, un
 	 32-bit platforms (for instance, SPARC debugging a SPARC64
 	 application).  */
 #ifdef HAVE_PREAD64
-      if (pread64 (fd, myaddr, len, memaddr) != len)
+      bytes = pread64 (fd, myaddr, len, memaddr);
 #else
-      if (lseek (fd, memaddr, SEEK_SET) == -1 || read (fd, myaddr, len) != len)
+      bytes = -1;
+      if (lseek (fd, memaddr, SEEK_SET) != -1)
+	bytes = read (fd, myaddr, len);
 #endif
-	{
-	  close (fd);
-	  goto no_proc;
-	}
 
       close (fd);
-      return 0;
+      if (bytes == len)
+	return 0;
+
+      /* Some data was read, we'll try to get the rest with ptrace.  */
+      if (bytes > 0)
+	{
+	  memaddr += bytes;
+	  myaddr += bytes;
+	  len -= bytes;
+	}
     }
 
  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 = (PTRACE_XFER_TYPE *) alloca (count * sizeof (PTRACE_XFER_TYPE));
+
   /* Read all the longwords */
+  errno = 0;
   for (i = 0; i < count; i++, addr += sizeof (PTRACE_XFER_TYPE))
     {
-      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[i] = ptrace (PTRACE_PEEKTEXT, pid,
 			  (PTRACE_ARG3_TYPE) (uintptr_t) addr, 0);
       if (errno)
-	return errno;
+	break;
     }
 
   /* Copy appropriate bytes out of the buffer.  */
+  i *= sizeof (PTRACE_XFER_TYPE);
+  i -= memaddr & (sizeof (PTRACE_XFER_TYPE) - 1);
   memcpy (myaddr,
 	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
-	  len);
+	  i < len ? i : len);
 
-  return 0;
+  return errno;
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's