[Toybox] Release prep.

[Andreas Gampe]

Here's a symbolized ASAN abort after the import of your fix:


 ==7750==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x004b00004be8 at pc 0x0056c377f290 bp 0x007ff7193f50 sp
0x007ff7193f48

READ of size 8 at 0x004b00004be8 thread T0

Stack Trace:
  RELADDR           FUNCTION         FILE:LINE
     7e28f  top_common+6367  external/toybox/toys/posix/ps.c:1420
     7c463  top_main+555     external/toybox/toys/posix/ps.c:1666
     1f7db  toy_exec+311     external/toybox/main.c:169
     1ef77  toybox_main+91   external/toybox/main.c:182
     1fa27  main+431         external/toybox/main.c:240
     ac9b3  __libc_init+91   bionic/libc/bionic/libc_init_dynamic.cpp:129

0x004b00004be8 is located 0 bytes to the right of 18920-byte region

allocated by thread T0 here:

Stack Trace:
  RELADDR           FUNCTION         FILE:LINE
     9d133          /system/lib64/libclang_rt.asan-aarch64-android.so
     1c333  xmalloc+19       external/toybox/lib/xwrap.c:71
  v-------------->  collate          external/toybox/toys/posix/ps.c:1175
     7ce63  top_common+1203  external/toybox/toys/posix/ps.c:1396
     7c463  top_main+555     external/toybox/toys/posix/ps.c:1666
     1f7db  toy_exec+311     external/toybox/main.c:169
     1ef77  toybox_main+91   external/toybox/main.c:182
     1fa27  main+431         external/toybox/main.c:240
     ac9b3  __libc_init+91   bionic/libc/bionic/libc_init_dynamic.cpp:129
      ee2f  _start_main+71   bionic/libc/arch-common/bionic/crtbegin.c:45
     2cc23  __dl__start+7    /proc/self/cwd/bionic/linker/arch/arm64/begin.S:36


Best,
 Andreas

On Thu, Feb 8, 2018 at 8:44 AM, Andreas Gampe <agampe at google.com> wrote:
> Argh. I should have given you the full failure message (or be more
> explicit in the suggested solution) - sorry, my mistake. ASAN
> complains about a heap buffer overflow, not a null pointer access
> (which should always kill top, not just under ASAN). Your fix was:
>
>   struct carveup *otb = old.tb ? *old.tb : 0, *ntb = new.tb ? *new.tb : 0;
>
> That only works if the tb value started out as null (as the loop
> itself only increments the pointers). That doesn't seem to be the
> failure case that ASAN is complaining about.
>
>
> I think this would be a better solution:
>
>   struct carveup *otb = old.count ? *old.tb : 0, *ntb = new.count ? *new.tb : 0;
>
>
> Best,
>  Andreas
>
>
> On Thu, Feb 8, 2018 at 7:06 AM, Rob Landley <rob at landley.net> wrote:
>> On 02/07/2018 10:41 AM, Andreas Gampe wrote:
>>> Stack Trace:
>>>   RELADDR           FUNCTION         FILE:LINE
>>>      7e253  top_common+6387  external/toybox/toys/posix/ps.c:1420
>>>      7c413  top_main+555     external/toybox/toys/posix/ps.c:1666
>>>      1f7db  toy_exec+311     external/toybox/main.c:169
>>>      1ef77  toybox_main+91   external/toybox/main.c:182
>>>      1fa27  main+431         external/toybox/main.c:240
>>>      ac8a3  __libc_init+91   bionic/libc/bionic/libc_init_dynamic.cpp:129
>>>
>>> ps.c:1419f
>>>
>>>     while (old.count || new.count) {
>>>       struct carveup *otb = *old.tb, *ntb = *new.tb;
>>>
>>> You probably should not proactively dereference both old.tb and
>>> new.tb. One might be invalid if not both old.count and new.count > 0.
>>
>> Oops, yes.
>>
>> Quick and dirty patch pushed, I need to properly re-read this chunk when
>> I get more time and verify the logic.
>>
>> Rob