<div dir="ltr">Thanks for the quick response. Feel free to ignore these reports until after you cut the release ;).<div><br></div><div>With the new code it could branch out and find some new ones:</div><div><div>$ toybox sh -c 'for;i 0'<br></div><div><div>$ toybox sh -c '`<`'<br></div><div><div>$ toybox sh -c '"$'"'"'"' # mostly shell escape; it's just " $ ' " without spaces <br></div><div><br></div><div><br></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jan 8, 2023 at 12:04 PM Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 1/8/23 01:41, Eric Roshan-Eisner wrote:<br>
> I ran the shell through the afl++ fuzzer, and it split out a few different ASAN<br>
> failures for simple inputs:<br>
<br>
Yeah, I'm grinding through the existing ones the test suite triggers right now,<br>
but since it's in pending I'm trying not to (further) hold up the 0.8.9 release<br>
for it.<br>
<br>
> heap-buffer-overflow:<br>
> $ toybox sh -c '$'<br>
<br>
Fixed, added test.<br>
<br>
> $ toybox sh -c '+()'<br>
<br>
Yeah, it does the same for +(.) too. The wildcard_path() plumbing's still<br>
unfinished. The basics are sketched out but I need to cycle back to it. (There's<br>
a reason it's still in pending...)<br>
<br>
Sigh, is there any way to get ASAN to stop showing the useless "shadow bytes"<br>
nonsense? (And "shadow byte legend" which is meta-nonsense?) I want the stack<br>
traces, not your internal metadata showing how proud you are of your<br>
implementation details. Grrr. Also, if the _first_ stack trace showing where the<br>
access _happened_ was actually a STACK TRACE rather than just a single file+line<br>
address, that would be nice. You give me a trace of where it was allocated from<br>
but not where the access you're objecting to occurred? Funky priorities, that...<br>
<br>
Hmmm, the problem doesn't look like it's in wildcard_path(), it seems like<br>
collect_wildcards() is filling out the deck[] wrong. (Think of<br>
collect_wildcards() as a bit like regcomp() and wildcard_path() as slightly<br>
regexec, but not really. When the eleventh doctor says "think of a banana" and<br>
then goes that's a rubbish analogy, forget the banana... yeah.)<br>
<br>
Sigh, adding a test for this sucks because bash defaults to extglob being off<br>
for -c (where in the hideous nest of rc files it parses on bringup does that get<br>
switched on, or is this one of those "defaults differ for different contexts"<br>
things bash loves to do...) And the extra-creaky bit is:<br>
<br>
  $ bash -c 'shopt -s extglob; echo +()'<br>
  bash: -c: line 0: syntax error near unexpected token `('<br>
  bash: -c: line 0: `shopt -s extglob; echo +()'<br>
  $ bash -c $'shopt -s extglob\necho +()'<br>
  +()<br>
<br>
Because the entire line is parsed before any of the commands in it are run!<br>
Which is #*%(#&%& _NOT_ the case with "for i in a b c; do echo $i; done" but...<br>
ARGH. Not emailing chet. Not emailing chet.<br>
<br>
Ahem. Not holding up the release for more work on pending either, lemme get back<br>
to this...<br>
<br>
(And yes, I should probably redo the temporary variable names on the second pass<br>
of wildcard plumbing. I have a tendency to put random word association names<br>
into things during development, and then people complain and I remove the<br>
references later. Calling a group wildcards a "deck" is straining it enough, but<br>
when I've got a second one calling it "ant" is probably unintelligible outside<br>
the UK. Nope, never actually been there...)<br>
<br>
> $ toybox sh -c '<<0;0'<br>
<br>
I accept that it parsed wrong, but what does... ok, that's start a HERE document<br>
terminated by the line "0" and then queue up running the command '0' afterwards,<br>
and then it runs out of input and complains about an unterminated HERE document<br>
without having proceeded to run the queued 0.<br>
<br>
  $ <<0;potato<br>
  > 0<br>
  bash: potato: command not found<br>
<br>
Right, test for that is just:<br>
shxpect 'queued work after HERE' I$'<<0;echo hello\n' E"> " I$'0\n' O$'hello\n'<br>
<br>
And yes bash passes it for "TEST_HOST=1 make test_sh".<br>
<br>
Ahem, test added, lemme come back to actually _fixing_ this. Trying to cut a<br>
release...<br>
<br>
> $ toybox sh -c '{$,}'<br>
<br>
That's actually the same test as trailing $ above, already fixed. (I can add a<br>
specific test for it, but the actual bug was trailing $ because {$,} expands to<br>
one argument that's just "$" and then the trailing , drops out because "" is<br>
ignored in certain circumstances. Interesting corner case actually, you can<br>
bring it BACK by appending it to an empty string, ala:<br>
<br>
  $ bash -c 'for i in ""{$,}; do echo ="$i"=; done'<br>
  =$=<br>
  ==<br>
  $ ./sh -c 'for i in ""{$,}; do echo ="$i"=; done'<br>
  =$=<br>
  ==<br>
<br>
And yes, mine is getting that right. And I _think_ I already have a test for it.<br>
Eh, add another anyway just to be sure...<br>
<br>
> floating-point-exception:<br>
> $ toybox sh -c '((0%0))'<br>
<br>
(I actually did this one first.)<br>
<br>
And /= 0. Added two checks and four tests to sh.test. (Catching and handling the<br>
signal without leaks is unnecessarily awkward. I'd need a siglongjmp() stack<br>
mechanism, and would prefer the plumbing just not _cause_ those...)<br>
<br>
> Also found some ASAN failures on the vi command.<br>
> <br>
> heap-buffer-overflow:<br>
> $ echo p > input; toybox vi -s input ascii.txt<br>
> stack-buffer-overflow:<br>
> $ echo s000000000000000 > input; toybox vi -s input ascii.txt<br>
<br>
Understood, but I haven't even _started_ review/cleanup of that one yet.<br>
<br>
Moving targets tend to get bumped down the todo list, and even if I _was_<br>
focusing on something but it moved out from under me I may not get back to it<br>
for _years_. Happend more than once on dd.c, and I should cycle back to man.c.<br>
Both are low hanging fruit at this point. I'm waiting to see if anything in the<br>
entire Linux From Scratch build under than the kernel (which I have a patch for)<br>
actually _uses_ bc, since anybody born after 1980 is either going to use<br>
$((math) or just python -c 'print 37/4.2' . Not that I want to _encourage_ that.<br>
<br>
> -Eric<br>
<br>
Rob<br>
<br>
P.S. Remind me to let your original email through moderation when I get home...<br>
</blockquote></div>