[RFA] Crasher bug in infptrace.c

[RFA] Crasher bug in infptrace.c

[Michael Snyder]

Here's one for the books...

Child_xfer_memory (one of the oldest functions in gdb)  uses alloca 
to allocate a buffer that can be arbitrarily large (as large as the
size of a memory read/write).  Alloca is known to be unsafe for large
enough chunks of memory, because it puts them on the stack -- and 
sure enough, it turns out that you can crash GDB by reading a large
enough data object from target memory.  For Linux, "large enough"
appears to be about 8 megabytes.  But this code has been as it is
for over ten years, and I've never heard of a problem with it before.

Test case attached (although because it causes GDB to core dump, 
it results in an ERROR instead of a FAIL...)

I don't believe this buffer is actually needed at all, but I've
gone with the minimum change instead of rewriting the function
so that it doesn't use a local buffer.

By the way, this code has been cloned in rs6000-nat.c, symm-nat.c, 
infttrace.c, and x86-64-linux-nat.c, so they probably have the
same bug.  I haven't touched them because I can't easily test them.


2001-12-30  Michael Snyder  <msnyder@redhat.com>

	* infptrace.c (GDB_MAX_ALLOCA): New define.
	(child_xfer_memory): Use xmalloc/xfree instead of alloca if the
	size of the buffer exceeds GDB_MAX_ALLOCA (default 1 megabyte, 
	can be overridden with whatever value is appropriate to the host).

2001-12-30  Michael Snyder  <msnyder@redhat.com>

	* gdb.base/huge.exp: New test.  Print a very large target data object.
	(skip_huge_test): New test variable.  Define if you want to skip this
	test.  The test reads an 8 megabyte data object from the target, so it
	might be very time consuming on remote targets with a slow connection.
	* gdb.base/huge.c: New file.  Test case for above.

Index: infptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/infptrace.c,v
retrieving revision 1.19
diff -c -3 -p -r1.19 infptrace.c
*** infptrace.c	2001/11/19 23:59:55	1.19
--- infptrace.c	2001/12/31 00:10:49
*************** store_inferior_registers (int regno)
*** 480,485 ****
--- 480,490 ----
  #endif /* !defined (FETCH_INFERIOR_REGISTERS).  */
  \f
  
+ /* Set an upper limit on alloca.  */
+ #ifndef GDB_MAX_ALLOCA
+ #define GDB_MAX_ALLOCA 0x100000
+ #endif
+ 
  #if !defined (CHILD_XFER_MEMORY)
  /* NOTE! I tried using PTRACE_READDATA, etc., to read and write memory
     in the NEW_SUN_PTRACE case.  It ought to be straightforward.  But
*************** child_xfer_memory (CORE_ADDR memaddr, ch
*** 506,514 ****
    /* 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 =
!     (PTRACE_XFER_TYPE *) alloca (count * sizeof (PTRACE_XFER_TYPE));
  
    if (write)
      {
--- 511,529 ----
    /* Round ending address up; get number of longwords that makes.  */
    int count = ((((memaddr + len) - addr) + sizeof (PTRACE_XFER_TYPE) - 1)
  	       / sizeof (PTRACE_XFER_TYPE));
+   int alloc = count * sizeof (PTRACE_XFER_TYPE);
+   PTRACE_XFER_TYPE *buffer;
+ 
    /* Allocate buffer of that many longwords.  */
!   if (len < GDB_MAX_ALLOCA)
!     {
!       buffer = (PTRACE_XFER_TYPE *) alloca (alloc);
!     }
!   else
!     {
!       buffer = (PTRACE_XFER_TYPE *) xmalloc (alloc);
!       make_cleanup (xfree, buffer);
!     }
  
    if (write)
      {
Index: testsuite/gdb.base/huge.c
===================================================================
RCS file: huge.c
diff -N huge.c
*** /dev/null	Tue May  5 13:32:27 1998
--- huge.c	Sun Dec 30 16:10:49 2001
***************
*** 0 ****
--- 1,19 ----
+ /*
+  * Test GDB's ability to read a very large data object from target memory.
+  */
+ 
+ /* 
+  * A value that will produce a target data object 
+  * large enough to crash GDB.  0x200000 is big enough
+  * on Linux, other systems may need a larger number.
+  */
+ 
+ #define CRASH_GDB 0x200000
+ 
+ static int a[CRASH_GDB], b[CRASH_GDB];
+ 
+ main()
+ {
+   memcpy (a, b, sizeof (a));
+   return 0;
+ }
Index: testsuite/gdb.base/huge.exp
===================================================================
RCS file: huge.exp
diff -N huge.exp
*** /dev/null	Tue May  5 13:32:27 1998
--- huge.exp	Sun Dec 30 16:10:49 2001
***************
*** 0 ****
--- 1,57 ----
+ # Copyright 2001 Free Software Foundation, Inc.
+ 
+ # This program is free software; you can redistribute it and/or modify
+ # it under the terms of the GNU General Public License as published by
+ # the Free Software Foundation; either version 2 of the License, or
+ # (at your option) any later version.
+ # 
+ # This program is distributed in the hope that it will be useful,
+ # but WITHOUT ANY WARRANTY; without even the implied warranty of
+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ # GNU General Public License for more details.
+ # 
+ # You should have received a copy of the GNU General Public License
+ # along with this program; if not, write to the Free Software
+ # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+ 
+ # Please email any bugs, comments, and/or additions to this file to:
+ # bug-gdb@prep.ai.mit.edu
+ 
+ # This file was written by Michael Snyder (msnyder@redhat.com)
+ 
+ if $tracelevel then {
+ 	strace $tracelevel
+ }
+ 
+ set prms_id 0
+ set bug_id 0
+ 
+ # Define if you want to skip this test
+ # (could be very time-consuming on remote targets with slow connection).
+ #
+ if [target_info exists gdb,skip_huge_test] {
+     return;
+ }
+ 
+ set testfile "huge"
+ set srcfile ${testfile}.c
+ set binfile ${objdir}/${subdir}/${testfile}
+ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+      gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+ }
+ 
+ # Start with a fresh gdb.
+ 
+ gdb_exit
+ gdb_start
+ gdb_reinitialize_dir $srcdir/$subdir
+ gdb_load ${binfile}
+ 
+ set timeout 30
+ 
+ if { ! [ runto main ] } then {
+     gdb_suppress_entire_file "Run to main failed, so all tests in this file will automatically fail."
+ }
+ 
+ gdb_test "print a" ".1 = .0 .repeats \[0123456789\]+ times.." "print a very large data object"
+ 

Re: [RFA] Crasher bug in infptrace.c

[Eli Zaretskii]

On Sun, 30 Dec 2001, Michael Snyder wrote:

> + /* Set an upper limit on alloca.  */
> + #ifndef GDB_MAX_ALLOCA
> + #define GDB_MAX_ALLOCA 0x100000
> + #endif

Isn't it better to use `getrlimit' to find out the max stack size, than 
to set up arbitrary limits (and proliferate system-specific definitions 
of GDB_MAX_ALLOCA)?  At least on systems where `getrlimit' is available, 
I think we should use it.

(FWIW, 1MB is too large for DJGPP, whose runtime stack defaults to 
512KB.  `getrlimit' is supported, so it will tell.)

Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder]

Eli Zaretskii wrote:
> 
> On Sun, 30 Dec 2001, Michael Snyder wrote:
> 
> > + /* Set an upper limit on alloca.  */
> > + #ifndef GDB_MAX_ALLOCA
> > + #define GDB_MAX_ALLOCA 0x100000
> > + #endif
> 
> Isn't it better to use `getrlimit' to find out the max stack size, than
> to set up arbitrary limits (and proliferate system-specific definitions
> of GDB_MAX_ALLOCA)?  At least on systems where `getrlimit' is available,
> I think we should use it.

Good idea, but it will require additional work (such as
using configure to determine whether getrlimit is supported).
May I suggest that this patch be accepted until someone
volunteers to do a better one?


> (FWIW, 1MB is too large for DJGPP, whose runtime stack defaults to
> 512KB.  `getrlimit' is supported, so it will tell.)

Does djgpp use infptrace?

Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder]

Michael Snyder wrote:
> 
> Eli Zaretskii wrote:
> >
> > On Sun, 30 Dec 2001, Michael Snyder wrote:
> >
> > > + /* Set an upper limit on alloca.  */
> > > + #ifndef GDB_MAX_ALLOCA
> > > + #define GDB_MAX_ALLOCA 0x100000
> > > + #endif
> >
> > Isn't it better to use `getrlimit' to find out the max stack size, than
> > to set up arbitrary limits (and proliferate system-specific definitions
> > of GDB_MAX_ALLOCA)?  At least on systems where `getrlimit' is available,
> > I think we should use it.
> 
> Good idea, but it will require additional work (such as
> using configure to determine whether getrlimit is supported).
> May I suggest that this patch be accepted until someone
> volunteers to do a better one?
> 
> > (FWIW, 1MB is too large for DJGPP, whose runtime stack defaults to
> > 512KB.  `getrlimit' is supported, so it will tell.)
> 
> Does djgpp use infptrace?

And I meant to add -- if so, you can always define GDB_MAX_ALLOCA
to a smaller value.  That's why I included it.

Re: [RFA] Crasher bug in infptrace.c

[Eli Zaretskii]

On Mon, 31 Dec 2001, Michael Snyder wrote:

> May I suggest that this patch be accepted until someone
> volunteers to do a better one?

I didn't mean to say that the patch should be rejected.  It's not my 
prerogative to do so in this case.

> Does djgpp use infptrace?

No, of course not.

Re: [RFA] Crasher bug in infptrace.c

[Eli Zaretskii]

On Mon, 31 Dec 2001, Michael Snyder wrote:

> > > (FWIW, 1MB is too large for DJGPP, whose runtime stack defaults to
> > > 512KB.  `getrlimit' is supported, so it will tell.)
> > 
> > Does djgpp use infptrace?
> 
> And I meant to add -- if so, you can always define GDB_MAX_ALLOCA
> to a smaller value.

Having yet another system-dependent macro is something I wanted to avoid.

Also, a static compile-time limit would have trouble in the face of users 
and sysadmins who can change the stack limits with `ulimit' and similar 
facilities.  Even with DJGPP, you can modify the maximum stack size of a 
program to an arbitrarily large or small value with a special utility.

Perhaps setting the limit you suggested to a much smaller value, like 
10KB, say, would be a better and easier way of solving this.  IIRC, last 
time the alloca issue was discussed, the consensus about the maximum size
of safe off-stack allocations was that it should be a few KB.

Re: [RFA] Crasher bug in infptrace.c

[Andrew Cagney]

> Here's one for the books...
> 
> Child_xfer_memory (one of the oldest functions in gdb)  uses alloca 
> to allocate a buffer that can be arbitrarily large (as large as the
> size of a memory read/write).  Alloca is known to be unsafe for large
> enough chunks of memory, because it puts them on the stack -- and 
> sure enough, it turns out that you can crash GDB by reading a large
> enough data object from target memory.  For Linux, "large enough"
> appears to be about 8 megabytes.  But this code has been as it is
> for over ten years, and I've never heard of a problem with it before.


BTW, the gdbint.texinfo document suggests that anything more than a few 
k is dangerous.

http://sources.redhat.com/gdb/onlinedocs/gdbint_13.html#SEC103


> Test case attached (although because it causes GDB to core dump, 
> it results in an ERROR instead of a FAIL...)
> 
> I don't believe this buffer is actually needed at all, but I've
> gone with the minimum change instead of rewriting the function
> so that it doesn't use a local buffer.
> 
> By the way, this code has been cloned in rs6000-nat.c, symm-nat.c, 
> infttrace.c, and x86-64-linux-nat.c, so they probably have the
> same bug.  I haven't touched them because I can't easily test them.


Probably a good move, perhaps add a FIXME comment to them so that the 
person that does encounter the bug knows they are not seeing things :-)

> +   int alloc = count * sizeof (PTRACE_XFER_TYPE);
> +   PTRACE_XFER_TYPE *buffer;
> + 
>     /* Allocate buffer of that many longwords.  */
> !   if (len < GDB_MAX_ALLOCA)
> !     {
> !       buffer = (PTRACE_XFER_TYPE *) alloca (alloc);
> !     }
> !   else
> !     {
> !       buffer = (PTRACE_XFER_TYPE *) xmalloc (alloc);
> !       make_cleanup (xfree, buffer);
> !     }


I think it would be better to just abandon the alloca() case and just 
use xmalloc().  That way the same code path (xmalloc()) is always used 
and mysterious / obscure bugs that end up being attributed to 
len?=GDB_MAX_ALLOCA can be avoided.


Andrew

Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder]

Andrew Cagney wrote:
> 
> > Here's one for the books...
> >
> > Child_xfer_memory (one of the oldest functions in gdb)  uses alloca
> > to allocate a buffer that can be arbitrarily large (as large as the
> > size of a memory read/write).  Alloca is known to be unsafe for large
> > enough chunks of memory, because it puts them on the stack -- and
> > sure enough, it turns out that you can crash GDB by reading a large
> > enough data object from target memory.  For Linux, "large enough"
> > appears to be about 8 megabytes.  But this code has been as it is
> > for over ten years, and I've never heard of a problem with it before.
> 
> BTW, the gdbint.texinfo document suggests that anything more than a few
> k is dangerous.
> 
> http://sources.redhat.com/gdb/onlinedocs/gdbint_13.html#SEC103

OK, I'll resubmit the patch with a smaller limit, perhaps 4K or 8K.


> > Test case attached (although because it causes GDB to core dump,
> > it results in an ERROR instead of a FAIL...)
> >
> > I don't believe this buffer is actually needed at all, but I've
> > gone with the minimum change instead of rewriting the function
> > so that it doesn't use a local buffer.
> >
> > By the way, this code has been cloned in rs6000-nat.c, symm-nat.c,
> > infttrace.c, and x86-64-linux-nat.c, so they probably have the
> > same bug.  I haven't touched them because I can't easily test them.
> 
> Probably a good move, perhaps add a FIXME comment to them so that the
> person that does encounter the bug knows they are not seeing things :-)

Will do.


> > +   int alloc = count * sizeof (PTRACE_XFER_TYPE);
> > +   PTRACE_XFER_TYPE *buffer;
> > +
> >     /* Allocate buffer of that many longwords.  */
> > !   if (len < GDB_MAX_ALLOCA)
> > !     {
> > !       buffer = (PTRACE_XFER_TYPE *) alloca (alloc);
> > !     }
> > !   else
> > !     {
> > !       buffer = (PTRACE_XFER_TYPE *) xmalloc (alloc);
> > !       make_cleanup (xfree, buffer);
> > !     }
> 
> I think it would be better to just abandon the alloca() case and just
> use xmalloc().  That way the same code path (xmalloc()) is always used
> and mysterious / obscure bugs that end up being attributed to
> len?=GDB_MAX_ALLOCA can be avoided.

I don't think so -- this function gets called a lot.  Heavy use of 
xmalloc on small buffers might lead to fragmentation.  Let's try the
idea of using alloca for small buffers and xmalloc for big ones.

Re: [RFA] Crasher bug in infptrace.c

[Andrew Cagney]

> 
> Will do.
> 
> 
> 
>> > +   int alloc = count * sizeof (PTRACE_XFER_TYPE);
>> > +   PTRACE_XFER_TYPE *buffer;
>> > +
>> > /* Allocate buffer of that many longwords.  */
>> > !   if (len < GDB_MAX_ALLOCA)
>> > !     {
>> > !       buffer = (PTRACE_XFER_TYPE *) alloca (alloc);
>> > !     }
>> > !   else
>> > !     {
>> > !       buffer = (PTRACE_XFER_TYPE *) xmalloc (alloc);
>> > !       make_cleanup (xfree, buffer);
>> > !     }
> 
>> 
>> I think it would be better to just abandon the alloca() case and just
>> use xmalloc().  That way the same code path (xmalloc()) is always used
>> and mysterious / obscure bugs that end up being attributed to
>> len?=GDB_MAX_ALLOCA can be avoided.
> 
> 
> I don't think so -- this function gets called a lot.  Heavy use of 
> xmalloc on small buffers might lead to fragmentation.  Let's try the
> idea of using alloca for small buffers and xmalloc for big ones.


I think trying to tune an alloca() buffer size is really dangerous. 
GDB's crashability starts to depend on how many alloca's have gone 
before / after the above call.  Regarding fragmentation, wouldn't it be 
better to get the code working correctly and only when fragmentation is 
demonstrated to be a problem, modify the algorithm.

Anyway, looking at the code, I'm wondering if it would actually be 
better to just eliminate that bounce buffer and, instead just transfer 
the data directly.  This might leave the buffer in an undefined state, I 
think, however, that is ok.

Andrew

Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder]

Andrew Cagney wrote:
> 
> >
> > Will do.
> >
> >
> >
> >> > +   int alloc = count * sizeof (PTRACE_XFER_TYPE);
> >> > +   PTRACE_XFER_TYPE *buffer;
> >> > +
> >> > /* Allocate buffer of that many longwords.  */
> >> > !   if (len < GDB_MAX_ALLOCA)
> >> > !     {
> >> > !       buffer = (PTRACE_XFER_TYPE *) alloca (alloc);
> >> > !     }
> >> > !   else
> >> > !     {
> >> > !       buffer = (PTRACE_XFER_TYPE *) xmalloc (alloc);
> >> > !       make_cleanup (xfree, buffer);
> >> > !     }
> >
> >>
> >> I think it would be better to just abandon the alloca() case and just
> >> use xmalloc().  That way the same code path (xmalloc()) is always used
> >> and mysterious / obscure bugs that end up being attributed to
> >> len?=GDB_MAX_ALLOCA can be avoided.
> >
> >
> > I don't think so -- this function gets called a lot.  Heavy use of
> > xmalloc on small buffers might lead to fragmentation.  Let's try the
> > idea of using alloca for small buffers and xmalloc for big ones.
> 
> I think trying to tune an alloca() buffer size is really dangerous.
> GDB's crashability starts to depend on how many alloca's have gone
> before / after the above call.  Regarding fragmentation, wouldn't it be
> better to get the code working correctly and only when fragmentation is
> demonstrated to be a problem, modify the algorithm.
> 
> Anyway, looking at the code, I'm wondering if it would actually be
> better to just eliminate that bounce buffer and, instead just transfer
> the data directly.  This might leave the buffer in an undefined state, I
> think, however, that is ok.

I spent an hour thinking about how to do that (without significantly
uglifying the code), and decided it was more trouble than I wanted to
go to.  I agree with you -- the function doesn't require a buffer 
at all.  Anyone who wants to rewrite the function to that extent
is more than welcome to by me.  ;-)

Re: [RFA] Crasher bug in infptrace.c

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 907 bytes --]

> Anyway, looking at the code, I'm wondering if it would actually be
>> better to just eliminate that bounce buffer and, instead just transfer
>> the data directly.  This might leave the buffer in an undefined state, I
>> think, however, that is ok.
> 
> 
> I spent an hour thinking about how to do that (without significantly
> uglifying the code), and decided it was more trouble than I wanted to
> go to.  I agree with you -- the function doesn't require a buffer 
> at all.  Anyone who wants to rewrite the function to that extent
> is more than welcome to by me.  ;-)


How does the attached work-in-progress look as a starting point?  Ran it 
against the testsuite and that came up with a few regressions ..... 
However, it isn't totally broken.

One thing I didn't realize is that I can't trust the alignment of the 
hosts buffer and consequently I can't actually avoid the double buffering.

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 3884 bytes --]

Index: infptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/infptrace.c,v
retrieving revision 1.20
diff -p -r1.20 infptrace.c
*** infptrace.c	2002/01/08 00:59:29	1.20
--- infptrace.c	2002/01/14 01:40:42
***************
*** 26,31 ****
--- 26,32 ----
  #include "target.h"
  #include "gdb_string.h"
  #include "regcache.h"
+ #include "gdb_assert.h"
  
  #include "gdb_wait.h"
  
*************** child_xfer_memory (CORE_ADDR memaddr, ch
*** 505,510 ****
--- 506,603 ----
  		   struct mem_attrib *attrib ATTRIBUTE_UNUSED,
  		   struct target_ops *target)
  {
+ #if 1
+   CORE_ADDR addr = memaddr;
+   long nr_bytes = len;
+   PTRACE_XFER_TYPE buffer[1000];
+   while (nr_bytes > 0)
+     {
+       CORE_ADDR bufaddr;
+       long count;
+       long sizeof_buffer;
+       long addr_off;
+       long addr_len;
+       long addr_pad;
+       long i;
+ 
+       /* Round starting address down to longword boundary to pick up
+          all of the first byte.  */
+       bufaddr = addr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
+ 
+       /* Round ending address up to ensure that all of the last word
+          is copied up and then adjust that to fit in BUFFER.  */
+       sizeof_buffer = (addr - bufaddr) + len + sizeof (PTRACE_XFER_TYPE) - 1;
+       if (sizeof_buffer > sizeof (buffer))
+ 	sizeof_buffer = sizeof (buffer);
+       count = sizeof_buffer / sizeof (PTRACE_XFER_TYPE);
+       sizeof_buffer = count * sizeof (PTRACE_XFER_TYPE); /* adjust */
+ 
+       /* Break the buffer down into three byte regions: ADDR_OFF
+ 	 specifies the offset into BUFFER that ADDR starts;
+ 	 ADDR_LEN specifies the number of ADDR bytes actually in
+ 	 the buffer; and ADDR_PAD is the number of left over bytes
+ 	 in the buffer.  Either/or ADDR_OFF and ADDR_PAD can be
+ 	 zero.  */
+       addr_off = addr - bufaddr;
+       addr_len = sizeof_buffer - addr_off;
+       if (addr_len > len)
+ 	{
+ 	  addr_len = len;
+ 	}
+       addr_pad = sizeof_buffer - (addr_len + addr_off);
+       gdb_assert (addr_off + addr_len + addr_pad == sizeof_buffer);
+       
+       /* For writes, pre-fill the buffer with data that has to go out.  */
+       if (write)
+ 	{
+ 	  /* If the start is misaligned, a read - modify - write is
+              needed.  */
+ 	  if (addr_off > 0)
+ 	    buffer[0] = ptrace (PT_READ_I, PIDGET (inferior_ptid), 
+ 				(PTRACE_ARG3_TYPE) bufaddr, 0);
+ 	  /* If the end is misaligned, a read - modify - write is
+              needed.  */
+ 	  if (addr_pad > 0 && count > 1)
+ 	    {
+ 	      buffer[count - 1] =
+ 		ptrace (PT_READ_I, PIDGET (inferior_ptid),
+ 			((PTRACE_ARG3_TYPE)
+ 			 (bufaddr + (count - 1) * sizeof (PTRACE_XFER_TYPE))), 0);
+ 	    }
+ 	  /* Fill in LEN bytes in the middle. */
+ 	  memcpy (buffer + addr_off, myaddr, addr_len);
+ 	}
+ 
+       /* Transfer COUNT words. */
+       for (i = 0; i < count; i++)
+ 	{
+ 	  CORE_ADDR a = bufaddr + i * sizeof (PTRACE_XFER_TYPE);
+ 	  if (write)
+ 	    {
+ 	      ptrace (PT_WRITE_D, PIDGET (inferior_ptid), 
+ 		      (PTRACE_ARG3_TYPE) a, buffer[i]);
+ 	    }
+ 	  else
+ 	    {
+ 	      buffer[i] = ptrace (PT_READ_I, PIDGET (inferior_ptid),
+ 				  (PTRACE_ARG3_TYPE) a, 0);
+ 	    }
+ 	}
+ 
+       /* Copy any read data into MYADDR.  */
+       if (!write)
+ 	memcpy (myaddr, buffer + addr_off, addr_len);
+ 
+       /* Update all counters.  */
+       addr += addr_len;
+       nr_bytes -= addr_len;
+     }
+   return len;
+ #ifdef CLEAR_INSN_CACHE
+   if (write)
+     CLEAR_INSN_CACHE ();
+ #endif
+ #else
    int i;
    /* Round starting address down to longword boundary.  */
    CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
*************** child_xfer_memory (CORE_ADDR memaddr, ch
*** 592,597 ****
--- 685,691 ----
    if (old_chain != NULL)
      do_cleanups (old_chain);
    return len;
+ #endif
  }
  \f
  

Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder]

Andrew Cagney wrote:
> 
> > Anyway, looking at the code, I'm wondering if it would actually be
> >> better to just eliminate that bounce buffer and, instead just transfer
> >> the data directly.  This might leave the buffer in an undefined state, I
> >> think, however, that is ok.
> >
> >
> > I spent an hour thinking about how to do that (without significantly
> > uglifying the code), and decided it was more trouble than I wanted to
> > go to.  I agree with you -- the function doesn't require a buffer
> > at all.  Anyone who wants to rewrite the function to that extent
> > is more than welcome to by me.  ;-)
> 
> How does the attached work-in-progress look as a starting point?  Ran it
> against the testsuite and that came up with a few regressions .....
> However, it isn't totally broken.

Close to how I envisioned it, but see comments below.

> One thing I didn't realize is that I can't trust the alignment of the
> hosts buffer and consequently I can't actually avoid the double buffering.

That's right...  ;-)   That's one reason why I bailed on it.

>   ------------------------------------------------------------------------
> Index: infptrace.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infptrace.c,v
> retrieving revision 1.20
> diff -p -r1.20 infptrace.c
> *** infptrace.c 2002/01/08 00:59:29     1.20
> --- infptrace.c 2002/01/14 01:40:42
> ***************
> *** 26,31 ****
> --- 26,32 ----
>   #include "target.h"
>   #include "gdb_string.h"
>   #include "regcache.h"
> + #include "gdb_assert.h"
> 
>   #include "gdb_wait.h"
> 
> *************** child_xfer_memory (CORE_ADDR memaddr, ch
> *** 505,510 ****
> --- 506,603 ----
>                    struct mem_attrib *attrib ATTRIBUTE_UNUSED,
>                    struct target_ops *target)
>   {
> + #if 1
> +   CORE_ADDR addr = memaddr;
> +   long nr_bytes = len;
> +   PTRACE_XFER_TYPE buffer[1000];
> +   while (nr_bytes > 0)
> +     {
> +       CORE_ADDR bufaddr;
> +       long count;
> +       long sizeof_buffer;
> +       long addr_off;
> +       long addr_len;
> +       long addr_pad;
> +       long i;
> +
> +       /* Round starting address down to longword boundary to pick up
> +          all of the first byte.  */
> +       bufaddr = addr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
> +
> +       /* Round ending address up to ensure that all of the last word
> +          is copied up and then adjust that to fit in BUFFER.  */
> +       sizeof_buffer = (addr - bufaddr) + len + sizeof (PTRACE_XFER_TYPE) - 1;
> +       if (sizeof_buffer > sizeof (buffer))
> +       sizeof_buffer = sizeof (buffer);

Now you've thrown away your rounding on the ending address.

> +       count = sizeof_buffer / sizeof (PTRACE_XFER_TYPE);

Now count will never be greater than 250, so all writes of
greater than 1000 bytes will lose.

> +       sizeof_buffer = count * sizeof (PTRACE_XFER_TYPE); /* adjust */
> +
> +       /* Break the buffer down into three byte regions: ADDR_OFF
> +        specifies the offset into BUFFER that ADDR starts;
> +        ADDR_LEN specifies the number of ADDR bytes actually in
> +        the buffer; and ADDR_PAD is the number of left over bytes
> +        in the buffer.  Either/or ADDR_OFF and ADDR_PAD can be
> +        zero.  */
> +       addr_off = addr - bufaddr;
> +       addr_len = sizeof_buffer - addr_off;

OK, except that sizeof_buffer has been truncated to 1000, 
therefore addr_len is forced to be less than or equal to that.

> +       if (addr_len > len)

And this can't evaluate to true if len > 1000.

> +       {
> +         addr_len = len;
> +       }
> +       addr_pad = sizeof_buffer - (addr_len + addr_off);
> +       gdb_assert (addr_off + addr_len + addr_pad == sizeof_buffer);
> +
> +       /* For writes, pre-fill the buffer with data that has to go out.  */
> +       if (write)
> +       {
> +         /* If the start is misaligned, a read - modify - write is
> +              needed.  */
> +         if (addr_off > 0)
> +           buffer[0] = ptrace (PT_READ_I, PIDGET (inferior_ptid),
> +                               (PTRACE_ARG3_TYPE) bufaddr, 0);
> +         /* If the end is misaligned, a read - modify - write is
> +              needed.  */
> +         if (addr_pad > 0 && count > 1)
> +           {
> +             buffer[count - 1] =
> +               ptrace (PT_READ_I, PIDGET (inferior_ptid),
> +                       ((PTRACE_ARG3_TYPE)
> +                        (bufaddr + (count - 1) * sizeof (PTRACE_XFER_TYPE))), 0);
> +           }
> +         /* Fill in LEN bytes in the middle. */
> +         memcpy (buffer + addr_off, myaddr, addr_len);
> +       }
> +
> +       /* Transfer COUNT words. */
> +       for (i = 0; i < count; i++)
> +       {
> +         CORE_ADDR a = bufaddr + i * sizeof (PTRACE_XFER_TYPE);
> +         if (write)
> +           {
> +             ptrace (PT_WRITE_D, PIDGET (inferior_ptid),
> +                     (PTRACE_ARG3_TYPE) a, buffer[i]);
> +           }
> +         else
> +           {
> +             buffer[i] = ptrace (PT_READ_I, PIDGET (inferior_ptid),
> +                                 (PTRACE_ARG3_TYPE) a, 0);
> +           }
> +       }
> +
> +       /* Copy any read data into MYADDR.  */
> +       if (!write)
> +       memcpy (myaddr, buffer + addr_off, addr_len);
> +
> +       /* Update all counters.  */
> +       addr += addr_len;
> +       nr_bytes -= addr_len;
> +     }
> +   return len;
> + #ifdef CLEAR_INSN_CACHE
> +   if (write)
> +     CLEAR_INSN_CACHE ();
> + #endif
> + #else
>     int i;
>     /* Round starting address down to longword boundary.  */
>     CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
> *************** child_xfer_memory (CORE_ADDR memaddr, ch
> *** 592,597 ****
> --- 685,691 ----
>     if (old_chain != NULL)
>       do_cleanups (old_chain);
>     return len;
> + #endif
>   }
> 
>

Re: [RFA] Crasher bug in infptrace.c

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

> +       if (sizeof_buffer > sizeof (buffer))
>> +       sizeof_buffer = sizeof (buffer);
> 
> 
> Now you've thrown away your rounding on the ending address.
> 
> 
>> +       count = sizeof_buffer / sizeof (PTRACE_XFER_TYPE);
> 
> 
> Now count will never be greater than 250, so all writes of
> greater than 1000 bytes will lose.
> 


Yes.  I found/fixed them + forgot to flush the insn cache + forgot to 
increment myaddr + forgot to write to i of d didn't work, yet it was 
still as broken!  Eventually I found the only real bug :-^

> +         memcpy (buffer + addr_off, myaddr, addr_len);


It should be ``(char*)buffer + addr_off'' :-(

Attatched is a version that appears to work (no regressions).

enjoy,
Andrew



[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 4562 bytes --]

? diffs
Index: infptrace.c
===================================================================
RCS file: /cvs/src/src/gdb/infptrace.c,v
retrieving revision 1.20
diff -p -r1.20 infptrace.c
*** infptrace.c	2002/01/08 00:59:29	1.20
--- infptrace.c	2002/01/14 03:20:14
***************
*** 26,31 ****
--- 26,32 ----
  #include "target.h"
  #include "gdb_string.h"
  #include "regcache.h"
+ #include "gdb_assert.h"
  
  #include "gdb_wait.h"
  
*************** child_xfer_memory (CORE_ADDR memaddr, ch
*** 505,510 ****
--- 506,614 ----
  		   struct mem_attrib *attrib ATTRIBUTE_UNUSED,
  		   struct target_ops *target)
  {
+ #if 1
+   CORE_ADDR addr = memaddr;
+   long nr_bytes = len;
+   char *srcdest = myaddr;
+   while (nr_bytes > 0)
+     {
+       PTRACE_XFER_TYPE buffer[1000];
+       long sizeof_buffer;
+       CORE_ADDR bufaddr;
+       long count;
+       long addr_off;
+       long addr_len;
+       long addr_pad;
+       long i;
+ 
+       /* Round starting address down to longword boundary to pick up
+          all of the first byte.  */
+       bufaddr = addr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
+ 
+       /* Round ending address up to ensure that all of the last word
+          is copied up and then adjust that to fit in BUFFER.  */
+       sizeof_buffer = ((addr - bufaddr)
+ 		       + nr_bytes
+ 		       + sizeof (PTRACE_XFER_TYPE) - 1);
+       if (sizeof_buffer > sizeof (buffer))
+ 	sizeof_buffer = sizeof (buffer);
+       count = sizeof_buffer / sizeof (PTRACE_XFER_TYPE);
+       sizeof_buffer = count * sizeof (PTRACE_XFER_TYPE); /* adjust */
+ 
+       /* Break the buffer down into three byte regions: ADDR_OFF
+ 	 specifies the offset into BUFFER that ADDR starts;
+ 	 ADDR_LEN specifies the number of ADDR bytes actually in
+ 	 the buffer; and ADDR_PAD is the number of left over bytes
+ 	 in the buffer.  Either/or ADDR_OFF and ADDR_PAD can be
+ 	 zero.  */
+       addr_off = addr - bufaddr;
+       addr_len = sizeof_buffer - addr_off;
+       if (addr_len > nr_bytes)
+ 	addr_len = nr_bytes;
+       addr_pad = sizeof_buffer - (addr_len + addr_off);
+       gdb_assert (addr_off + addr_len + addr_pad == sizeof_buffer);
+       
+       /* For writes, pre-fill the buffer with data that has to go out.  */
+       if (write)
+ 	{
+ 	  /* If the start is misaligned, a read - modify - write is
+              needed.  */
+ 	  if (addr_off > 0)
+ 	    buffer[0] = ptrace (PT_READ_I, PIDGET (inferior_ptid), 
+ 				(PTRACE_ARG3_TYPE) bufaddr, 0);
+ 	  /* If the end is misaligned, a read - modify - write is
+              needed.  */
+ 	  if (addr_pad > 0 && count > 1)
+ 	    {
+ 	      buffer[count - 1] =
+ 		ptrace (PT_READ_I, PIDGET (inferior_ptid),
+ 			((PTRACE_ARG3_TYPE)
+ 			 (bufaddr + (count - 1) * sizeof (PTRACE_XFER_TYPE))),
+ 			0);
+ 	    }
+ 	  /* Fill in LEN bytes in the middle. */
+ 	  memcpy ((char *) buffer + addr_off, srcdest, addr_len);
+ 	}
+ 
+       /* Transfer COUNT words. */
+       for (i = 0; i < count; i++)
+ 	{
+ 	  CORE_ADDR a = bufaddr + i * sizeof (PTRACE_XFER_TYPE);
+ 	  if (write)
+ 	    {
+ 	      errno = 0;
+ 	      ptrace (PT_WRITE_D, PIDGET (inferior_ptid), 
+ 		      (PTRACE_ARG3_TYPE) a, buffer[i]);
+ 	      if (errno)
+ 		{
+ 		  /* Using the appropriate one (I or D) is necessary
+ 		     for Gould NP1, at least.  */
+ 		  errno = 0;
+ 		  ptrace (PT_WRITE_I, PIDGET (inferior_ptid), 
+ 			  (PTRACE_ARG3_TYPE) addr, buffer[i]);
+ 		}
+ 	    }
+ 	  else
+ 	    {
+ 	      buffer[i] = ptrace (PT_READ_I, PIDGET (inferior_ptid),
+ 				  (PTRACE_ARG3_TYPE) a, 0);
+ 	    }
+ 	}
+ 
+       /* Copy any read data into MYADDR.  */
+       if (!write)
+ 	memcpy (srcdest, (char *) buffer + addr_off, addr_len);
+ 
+       /* Update all counters.  */
+       addr += addr_len;
+       nr_bytes -= addr_len;
+       srcdest += addr_len;
+     }
+ #ifdef CLEAR_INSN_CACHE
+   if (write)
+     CLEAR_INSN_CACHE ();
+ #endif
+ #else
    int i;
    /* Round starting address down to longword boundary.  */
    CORE_ADDR addr = memaddr & -(CORE_ADDR) sizeof (PTRACE_XFER_TYPE);
*************** child_xfer_memory (CORE_ADDR memaddr, ch
*** 591,596 ****
--- 695,715 ----
  
    if (old_chain != NULL)
      do_cleanups (old_chain);
+ #endif
+ #if 0
+   {
+     int i;
+     static FILE *t;
+     if (!t)
+       t = fopen ("log", "w");
+     fprintf (t, "%c 0x%lx %ld :",
+ 	     write ? 'w' : 'r',
+ 	     (long) memaddr, (long) len);
+     for (i = 0; i < len; i++)
+       fprintf (t, "%02x", myaddr[i] & 0xff);
+     fprintf (t, "\n");
+   }
+ #endif
    return len;
  }
  \f