[Toybox] ASAN errors in toysh

[Eric Roshan-Eisner]

Thanks for the quick response. Feel free to ignore these reports until
after you cut the release ;).

With the new code it could branch out and find some new ones:
$ toybox sh -c 'for;i 0'
$ toybox sh -c '`<`'
$ toybox sh -c '"$'"'"'"' # mostly shell escape; it's just " $ ' " without
spaces



On Sun, Jan 8, 2023 at 12:04 PM Rob Landley <rob at landley.net> wrote:

> 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...
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20230109/cd2b62df/attachment.htm>