[Toybox] ASAN errors in toysh

[Rob Landley]

On 1/8/23 01:41, Eric Roshan-Eisner wrote:
> I ran the shell through the afl++ fuzzer, and it split out a few different ASAN
> failures for simple inputs:

Yeah, I'm grinding through the existing ones the test suite triggers right now,
but since it's in pending I'm trying not to (further) hold up the 0.8.9 release
for it.

> heap-buffer-overflow:
> $ toybox sh -c '$'

Fixed, added test.

> $ toybox sh -c '+()'

Yeah, it does the same for +(.) too. The wildcard_path() plumbing's still
unfinished. The basics are sketched out but I need to cycle back to it. (There's
a reason it's still in pending...)

Sigh, is there any way to get ASAN to stop showing the useless "shadow bytes"
nonsense? (And "shadow byte legend" which is meta-nonsense?) I want the stack
traces, not your internal metadata showing how proud you are of your
implementation details. Grrr. Also, if the _first_ stack trace showing where the
access _happened_ was actually a STACK TRACE rather than just a single file+line
address, that would be nice. You give me a trace of where it was allocated from
but not where the access you're objecting to occurred? Funky priorities, that...

Hmmm, the problem doesn't look like it's in wildcard_path(), it seems like
collect_wildcards() is filling out the deck[] wrong. (Think of
collect_wildcards() as a bit like regcomp() and wildcard_path() as slightly
regexec, but not really. When the eleventh doctor says "think of a banana" and
then goes that's a rubbish analogy, forget the banana... yeah.)

Sigh, adding a test for this sucks because bash defaults to extglob being off
for -c (where in the hideous nest of rc files it parses on bringup does that get
switched on, or is this one of those "defaults differ for different contexts"
things bash loves to do...) And the extra-creaky bit is:

  $ bash -c 'shopt -s extglob; echo +()'
  bash: -c: line 0: syntax error near unexpected token `('
  bash: -c: line 0: `shopt -s extglob; echo +()'
  $ bash -c $'shopt -s extglob\necho +()'
  +()

Because the entire line is parsed before any of the commands in it are run!
Which is #*%(#&%& _NOT_ the case with "for i in a b c; do echo $i; done" but...
ARGH. Not emailing chet. Not emailing chet.

Ahem. Not holding up the release for more work on pending either, lemme get back
to this...

(And yes, I should probably redo the temporary variable names on the second pass
of wildcard plumbing. I have a tendency to put random word association names
into things during development, and then people complain and I remove the
references later. Calling a group wildcards a "deck" is straining it enough, but
when I've got a second one calling it "ant" is probably unintelligible outside
the UK. Nope, never actually been there...)

> $ toybox sh -c '<<0;0'

I accept that it parsed wrong, but what does... ok, that's start a HERE document
terminated by the line "0" and then queue up running the command '0' afterwards,
and then it runs out of input and complains about an unterminated HERE document
without having proceeded to run the queued 0.

  $ <<0;potato
  > 0
  bash: potato: command not found

Right, test for that is just:
shxpect 'queued work after HERE' I$'<<0;echo hello\n' E"> " I$'0\n' O$'hello\n'

And yes bash passes it for "TEST_HOST=1 make test_sh".

Ahem, test added, lemme come back to actually _fixing_ this. Trying to cut a
release...

> $ toybox sh -c '{$,}'

That's actually the same test as trailing $ above, already fixed. (I can add a
specific test for it, but the actual bug was trailing $ because {$,} expands to
one argument that's just "$" and then the trailing , drops out because "" is
ignored in certain circumstances. Interesting corner case actually, you can
bring it BACK by appending it to an empty string, ala:

  $ bash -c 'for i in ""{$,}; do echo ="$i"=; done'
  =$=
  ==
  $ ./sh -c 'for i in ""{$,}; do echo ="$i"=; done'
  =$=
  ==

And yes, mine is getting that right. And I _think_ I already have a test for it.
Eh, add another anyway just to be sure...

> floating-point-exception:
> $ toybox sh -c '((0%0))'

(I actually did this one first.)

And /= 0. Added two checks and four tests to sh.test. (Catching and handling the
signal without leaks is unnecessarily awkward. I'd need a siglongjmp() stack
mechanism, and would prefer the plumbing just not _cause_ those...)

> Also found some ASAN failures on the vi command.
> 
> heap-buffer-overflow:
> $ echo p > input; toybox vi -s input ascii.txt
> stack-buffer-overflow:
> $ echo s000000000000000 > input; toybox vi -s input ascii.txt

Understood, but I haven't even _started_ review/cleanup of that one yet.

Moving targets tend to get bumped down the todo list, and even if I _was_
focusing on something but it moved out from under me I may not get back to it
for _years_. Happend more than once on dd.c, and I should cycle back to man.c.
Both are low hanging fruit at this point. I'm waiting to see if anything in the
entire Linux From Scratch build under than the kernel (which I have a patch for)
actually _uses_ bc, since anybody born after 1980 is either going to use
$((math) or just python -c 'print 37/4.2' . Not that I want to _encourage_ that.

> -Eric

Rob

P.S. Remind me to let your original email through moderation when I get home...