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

[Rob Landley]

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?

Rob