[Toybox] memcpy overlap in ps

[Rob Landley]

On 10/06/2016 04:15 PM, Evgenii Stepanov wrote:
> Hi Rob,
> 
> thanks for the explanation. This is definitely not a false positive -
> the report even contains dst and src ranges for the memcpy() call, and
> they indeed overlap. Should be possible to reproduce w/o ASan by
> checking the addresses in the code.

Right before the memcpy/memmove I added:

printf("buf=%p s=%p len=%ld\n", buf, s, (long)len);
if (s+len >= buf) printf("BANANA!\n");

And did

  ./ps -aO NAME

And I don't see an instance of overlap?

(I shouldn't have to test the other way because buf should always be > len?)

> I've added some debug printfs to this code, and got this:

I'm guessing on line 753?

> j = 5
> ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb961

This doesn't have s and buf, it has tb and buf. Still, it lets us know
you're looking at the non-thread case during the PS_NAME copy. Line 758
reassigns ptb so you'd have to have your printf before that.

Then we do a slightly funky basename():

          i = ptb->slot[SLOT_argv0len];
          s = ptb->str+ptb->offset[4];
          while (-1!=(k = stridx(s, '/')) && k<i) {
            s += k+1;
            i -= k+1;
          }

Given that offset[4] should be a properly null terminated string, that
while() loop shouldn't advance past the end of it, and _should_ decrease
i by the same amount it increased s. Since "buf" should point to the
next byte after that string... how do they wind up overlapping?

(Is ptb->slot[SLOT_argv0len] winding up bigger than
strlen(ptb->str+ptb->offset[4]) somehow? I'm not seeing it in my tests
here, is it a 32-bit thing? Bionic? Android kernel?)


> USER       PID  PPID     VSZ    RSS WCHAN              PC S NAME
> root         1     0   10628   1272 SyS_epoll_     4d805c S init
> j = 5
> ptb 0x0 tb 0x5564dfb750 buf 0x5564dfb94b

Basically the same data as above. We're not copying from buf to tb,
we're copying len bytes from s to buf. This doesn't tell me what the bad
memcpy's inputs are.

> The change (the one that requires a google login) simply replaces
> memcpy with memmove, which sounds like a bad idea at this point.

Well... maybe? I still don't know what's going on.

Rob