Re: bin/55979 (sh single quotes removes nul characters)

[Robert Elz <kre%munnari.OZ.AU@localhost>]

The following reply was made to PR bin/55979; it has been noted by GNATS.

From: Robert Elz <kre%munnari.OZ.AU@localhost>
To: Kamil Rytarowski <kamil%netbsd.org@localhost>
Cc: gnats-bugs%netbsd.org@localhost
Subject: Re: bin/55979 (sh single quotes removes nul characters)
Date: Fri, 12 Feb 2021 00:48:31 +0700

     Date:        Wed, 10 Feb 2021 18:37:43 +0100
     From:        Kamil Rytarowski <kamil%netbsd.org@localhost>
     Message-ID:  <d926a6ef-5877-f1de-bf77-d7b2b57027c2%netbsd.org@localhost>
 
   | It shows me:
   |
   | $ ./sh hello.com
   |
   | ==14798==WARNING: MemorySanitizer: use-of-uninitialized-value
   |     #0 0x46fa69 in shellexec /usr/src/bin/sh/exec.c:136:18
 
 I know what this is now, it was introduced (inadvertently) as part of
 the fix for PRs 42184 and 52687 (two PRs, same issue) - the problem there
 was short-circuiting command not found processing, when a command name
 without a '/' in it was not found in a PATH search.
 
 The PATH search needs to be done in the parent shell (the one which read
 the command and is processing it) so the results can be cached, but all
 other processing (including redirects, which are still supposed to happen -
 and in particular which can redirect stderr, to which the command not found
 error message should be sent) is supposed to happen in a sub-shell.   The
 way it was before the fix, when the parent shell fails to locate the command
 in the path search, it simply aborted command processing, issued the error
 message and went on with whatever should happen next (issuing next prompt,
 or whatever) - the redirects never happened (so the not found error message
 went to the shell's stderr, rather than to where stderr had been redirected).
 The fix for that was simply to drop the short-circuit test, and treat not-found
 commands the same as found commands, fork a sub-shell, process all the
 redirects, and then go find the command again (which will fail again, except
 in the very weird case of a race condition where the command suddenly appears
 in a directory that is in PATH, but we don't care about that, it's a race,
 someone wins, someone loses.)
 
 The relevant var (field in the cmdentry struct), the index, wasn't being
 set in the not-found case.   It marks which entry in PATH located the
 command (but of course, there is none for a not-found command).  With
 the old short circuit in place, that was fine, as after the initial not
 found lookup, everything stopped, and nothing ever looked at that value.
 
 Now, that has changed, and the second time we search for the command, we
 do look at it (PATH isn't an array, so the uninit'd var isn't being used
 as an index, just to compare against the counted elements in PATH as they
 are processed one by one - it avoids meaningless attempts at execl() when
 we have already determined that the command won't be found there).   Since
 the command doesn't exist anywhere, it really makes no difference what the
 index value we're looking at says, anywhere it points us to (including
 nowhere) isn't going to locate the command.  Except in the (non-existing
 I believe) case of hardware which actually detects the use of the uninitialised
 value and does something strange (stranger than simply using some random value)
 (MSAN is the one example of such "hardware") I believe this problem makes
 no difference to anything that matters.
 
 So, while it will get fixed, I don't think this will actually affect the
 actual issue in this PR (as amended) and I most likely won't bother
 requesting pullups to -8 or -9 (the change that caused it happened in
 Aug 2018, before -9 was released, and was marked to be pulled up to -8,
 which I assume happened).   That is unless some other change related to
 this PR is needed, and should be pulled up, in which case this trivial fix
 can hitch a ride.
 
 A commit to fix this in HEAD will come sometime in the not too distant
 future.
 
 The issue in this PR triggered the MSAN detection, as it ends up attempting
 to run a bogus (and hence, not found) command name.   Why that is happening
 is the next thing to discover, but it cannot be because of this (MSAN) issue,
 this comes later.
 
 kre