[Toybox] [PATCH] grep: fix two bugs found by hwasan.

[enh]

On Tue, Jul 16, 2019 at 9:21 PM Rob Landley <rob at landley.net> wrote:
>
> On 7/16/19 5:05 PM, enh via Toybox wrote:
> > The first bug appeared as a memory overwrite, but was actually visible
> > without hwasan: basically any `grep -F` that let to multiple matches on
> > the same line was broken.
>
> "used the wrong variable" is the kinda bug I go "almost certainly my fault if
> the changed version works at all, easily a thinko" and move on without digging
> too deeply.
>
> > The second bug was another memory overwrite, visible when I ran the
> > existing grep tests.
>
> This is not. Why did I have that +1 there, which you are removing?
>
>           while (dlb) {
>             struct double_list *dl = dlist_pop(&dlb);
>             unsigned *uu = (void *)(dl->data+((strlen(dl->data)+1)|3)+1);
>
>             outline(dl->data, '-', name, lcount-before, uu[0]+1, uu[1]);
>             free(dl->data);
>             free(dl);
>             before--;
>           }
>
> Because I stashed an unsigned value after the string, because the -B "before"
> lines need to show offset of the match, and I had to stick it somewhere so it
> would still be avaliable when we get around to showing those lines (while
> matching a later line, because -B).
>
> strlen(dl->data) skip string.
> +1 skip null terminator.
> |3 round up to 4 byte alignment.
> +1 move to start of next entry because a pointer ending in |3 ain't aligned.
>
> Then uu[0]+1 is because 0 says not to display it, and we subtract the 1 back out
> when displaying:
>
>   if (bcount && FLAG(b)) numdash(bcount-1, dash);
>
> Your fix results in unaligned access, which your current hardware doesn't care
> about but others do.
>
> If there _is_ a memory problem, it sounds like the remalloc() didn't allocate
> enough space?
>
>       if (discard && TT.B) {
>         unsigned *uu, ul = (ulen+1)|3;
>
>         line = xrealloc(line, ul+8); // 2*sizeof(unsigned) in LP64
>         uu = (void *)(line+ul+1);
>         uu[0] = offset-len;
>         uu[1] = ulen;
>         dlist_add(&dlb, line);
>
> The ulen came from getdelim(), we skip the NULL, round up,.. aha! But do _not_
> then add one, instead I added it to the uu assignement. So the allocation's one
> byte short.
>
> (Hmmm... the logic should probably be (len|3)+1 because that would always skip
> the null terminator but wouldn't add any OTHER space if the result is aligned?)
>
> I committed a fix on top of yours, does it still pass your sanitizer?

yes, thanks. (a comment there about alignment would have helped... in
2019 it's rare to see anyone still care about alignment!)

i'll send a patch that adds an ASAN=1 to the build scripts so it's
easier to test like this. whenever hwasan finds a bug (the last one i
had to fix was in BSD grep, amusingly enough) i try to reproduce and
fix on the host for convenience and although it's not hard to do it's
quite a long command line.

i was hoping to talk about valgrind -> asan -> hwasan [where we are
now] -> ARM MTE [where we want to be] in the Google I/O 2019 C++ talk,
so i could send you a link. as it was, we only got hwasan into the
Pixel 2 kernels for Q
[https://source.android.com/devices/tech/debug/hwasan], and i didn't
want to spend time on what's effectively still vaporware from an app
developer perspective. but there are some slides by the folks doing
the actual work from an LLVM conference last year:
https://llvm.org/devmtg/2018-10/slides/Serebryany-Stepanov-Tsyrklevich-Memory-Tagging-Slides-LLVM-2018.pdf
--- TL;DR: MTE is "always-on asan in production".

> Rob