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

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

Hi Pedro,

> >  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.
> 
> 
> It looks like the pread64/lseek/read path actually reads directly
> into MYADDR (the supplied buffer).

 Yes, that's true, but for this to happen all the calls up to and 
including pread64/read need to succeed.

> > 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.
> 
> 
> Right.
> 
> >  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.
> 
> 
> So how about just returning immediately if reading from /proc manages to
> read something?  From what you say, the PEEKTEXT fallback then won't just
> normally fail; it'll _always_ fail.  I suspect that'd make the code a bit
> simpler.

 Maybe.  Question is, actually why it was written like this in the first 
place.  And the only answer I could come up with was: to retrieve the 
right error code for the caller to investigate.  You won't get that with 
pread64/read if they succeed reading any data at all, even if the amount 
is less than requested.  Hence I decided to preserve the original flow.  
I do not feel comfortable with cooking up an artificial value of errno.

 Alternatively we could revamp the whole API to make 
the_target->read_memory return the number of bytes actually read, just 
like read and friends do.  That would of course ask for a complementing 
change to the_target->write_memory too.  I even thought about it when I 
finally tracked down what the cause of odd behaviour was, but I decided I 
was too tired debugging this issue already to turn gdbserver upside down 
at that stage.

 I think my fix will serve its purpose and if we decide to turn overhaul 
this part of gdbserver indeed, then perhaps it would be best done after 
7.5 has been branched.

> >    /* 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;
> 
> 
> You shouldn't assume that "errno" is preserved across library
> calls (memcpy in this case).

 Oh, that was an oversight rather than a deliberate assumption, sorry 
about that.  Here's a trivial update that I have applied to my fix.

  Maciej

Index: gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/gdbserver/linux-low.c	2012-05-18 19:28:38.255559523 +0100
+++ gdb-fsf-trunk-quilt/gdb/gdbserver/linux-low.c	2012-05-18 19:28:19.795559309 +0100
@@ -4384,6 +4384,7 @@ linux_read_memory (CORE_ADDR memaddr, un
   register int count;
   char filename[64];
   register int i;
+  int ret;
   int fd;
 
   /* Try using /proc.  Don't bother for one word.  */
@@ -4443,6 +4444,7 @@ linux_read_memory (CORE_ADDR memaddr, un
       if (errno)
 	break;
     }
+  ret = errno;
 
   /* Copy appropriate bytes out of the buffer.  */
   i *= sizeof (PTRACE_XFER_TYPE);
@@ -4451,7 +4453,7 @@ linux_read_memory (CORE_ADDR memaddr, un
 	  (char *) buffer + (memaddr & (sizeof (PTRACE_XFER_TYPE) - 1)),
 	  i < len ? i : len);
 
-  return errno;
+  return ret;
 }
 
 /* Copy LEN bytes of data from debugger memory at MYADDR to inferior's