[Toybox] memcpy overlap in ps

[Evgenii Stepanov]

Looks like this happens when /proc/$PID/cmdline is empty, which is
true for "kthreadd" on android.
Numbered strings below are ptb->str + ptb->offset[i].
For some reason ptb->slot[SLOT_argv0len] == 8 when cmdline is empty,
but I don't see where it's coming from.
And ptb->str + ptb->offset[4] + ptb->slot[SLOT_argv0len] overlaps with
buf; and there are no "/" characters in cmdline to limit the memcpy
range.
Looks like a bug?

0:
1: SyS_epoll_wait
2:
3:
4: /init --second-stage
buf: 0x55928d7961
ptb->str = 0x55928d7935, ptb->offset[4] = 23, s = 0x55928d794c
s: /init --second-stage, i: 5, len: 3051
skip 1
buf 0x55928d7961, s 0x55928d794d, len 4
USER       PID  PPID     VSZ    RSS WCHAN              PC S NAME
root         1     0   10628   1316 SyS_epoll_     4d805c S init
0:
1: kthreadd
2:
3:
4:
buf: 0x55928d794b
ptb->str = 0x55928d7935, ptb->offset[4] = 21, s = 0x55928d794a
s: , i: 8, len: 3073
buf 0x55928d794b, s 0x55928d794a, len 8
=================================================================
==23291==ERROR: AddressSanitizer: memcpy-param-overlap: memory ranges
[0x0055928d794b,0x0055928d7953) and [0x0055928d794a, 0x0055928d7952)
overlap

On Fri, Oct 7, 2016 at 12:00 PM, Rob Landley <rob at landley.net> wrote:
> 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