Re: [RFA] Crasher bug in infptrace.c

[Michael Snyder <msnyder at redhat dot com>]

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
>   }
> 
>