Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder <msnyder at redhat dot com>]

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.