[Buildroot] [RFC PATCH 0/2] use `command -v' instead of `which'

Arnout Vandecappelle arnout at mind.be
Sun Oct 3 09:49:24 UTC 2021



On 02/10/2021 21:22, Petr Vorel wrote:
> Hi Yann, Arnout, all,
> 
>> Petr, Arnout, All,
> 
>> On 2021-09-30 22:16 +0200, Petr Vorel spake thusly:
>>>> On 2021-09-26 23:32 +0200, Arnout Vandecappelle spake thusly:
>>>>> On 21/09/2021 22:51, Petr Vorel wrote:
>>>> This is causing quite some issues.
>> [--SNIP--]
>>>> Second, this is causing a lot of error messages:
>>>>      $ make defconfig
>>>>      [...]
>>>>      $ make help
>>>>      make[1]: command: Command not found
>>>>      [...]
>>> New error. But I was not able to reproduce it on x86_64 on current master
>>> (5916cc5011). What am I missing to reproduce it?
> 
>> We've investigated with Arnout, who was also unable to reproduce, and we
>> eventually found the cause for this issue, which is two fold.
> Thanks a lot for investigation!
> 
>> First, make, at least in some versions of make, will run commands that
>> it believes are "simple", directly with execve() (or any other exec*()
>> wrapper), instead of running it through a shell via system().
> 
>> This is what happens when it see a command like this:
> 
>>      export PERL=$(shell command -v perl)
> Hm, that's bad. Can you post any version which is affected?
> It looks like I tested it only on 4.3, but have plenty of VM with older
> make releases.
> 
>> which we have in package/Makefile.in at 240. In this case, make will try to
> NOTE for myself (when debugging this later): package/Makefile.in (@240 is probably garbage)

  It *is* on line 240 in current master, and package/Makefile.in hasn't changed 
since July 4 (except for the command -v replacement, of course).

> 
>> run the command (split à-la python), as can be seen with strace:
> 
>>      ['command', '-v', 'perl']
> 
>> Second, the issue was invisible to Arnout, because the distribution he
>> uses, Fedora, provides /usr/bin/command, which is a simple shell script
>> that just basically does:
> 
>>      #!/bin/sh
>>      builtin command "${@}"
> 
> Hm, wrapping shell builtin into script is really strange.

  POSIX doesn't say anything about `command` etc. being shell builtins. Thus, a 
program written for POSIX may assume that it can execvp("command", ...) (i.e. 
without going through the shell).

  I assume that someone in RedHat encountered such an issue and that that was 
their solution.

> BTW I was already thinking to add similar script to buildroot,

  Right - instead of providing our on `which`, we can also provide our own 
`command`!

  One small caveat though: we currently don't add anything to PATH. Adding our 
own thing (whether it's which or command) will require us to add something to 
PATH, which by itself can have surprising effects as well.

> but I'm really surprised that Fedora added that to distro.
> 
>> However, this is provided by no package in the distribution I use,
>> Ubuntu 20.04.1 LTS (filtering to ignore /usr/bin/commander et al.):
> 
>>      $ apt-file search bin/command |grep -E 'bin/command$'
>>      [nothing]
> 
> This is probably Fedora/RHEL specific. I'll investigate which package it belongs

  Yes it is. It's provided by the bash package.

> and ask Fedora maintainer for a reason (unfortunately Fedora does not have any
> search like Debian https://packages.debian.org/ [1]).
> 
>> So, probably you did not see the error either, because your distribution
>> also provides command as an actual executable. Could you check that by
>> running:    which command     (Ahaha! :-])
> 
>> Note: if we change the line above to
> 
>>      export PERL=$(shell command -v perl 2>/dev/null)
> 
>> then make no longer believes this is a simple command, and will execute
>> with system() and the warning goes away.

  However, a future version of GNU make may very well decide that '2>/dev/null' 
is something it can handle itself, and bypass the shell as well.

> Hm, definitely worth to more investigate which make releases are affected and
> report if not already fixed.
> 
>> So, bottom line, there are more impacts than previously expected, and we
>> need to think the transition more carefully.
> +1
> 
>> And to be extra clear: I am OK with transitionning away from which, or
>> at least from relying on which being provided by the distro.
> Thanks!
> 
> Also going back to the issue with bash 'command -v' implementation. First is it
> also relevant to this issue or not?

  It's an issue if we pass several parameters to `command -v`. We probably 
should simply never do that - the case that Markus found is a bug anyway.


> You mentioned Ubuntu with bash as /bin/sh as a default shell in previous mail.
> Testing 'make defconfig && make help 2>&1 |grep -i found' on bash as /bin/sh
> does not trigger the error (make[1]: command: Command not found). Thus bash it's
> not the problem (at least less problematic than make you mentioned).
> 
> Also, does anybody understand POSIX spec [2] whether only single command_name
> can be used as Markus reported [3]? Should we report it to bash?

  The POSIX spec says:

command [-p][-v|-V] command_name

So it's only specified with a single argument. The POSIX spec is pretty vague 
about what to do with stuff that is not specified, like a second argument. So I 
guess implementations can do what they want...


  By the way, the differences between which and command -v are a bit larger than 
this. command -v also works for shell builtins, while which doesn't - unless 
which is defined as a function, which is the case for the `which` package in 
Fedora...

$ command -v ls
alias ls='ls -AFh'
$ which ls
alias ls='ls -AFh'
         /usr/bin/ls
$ /bin/which ls
/usr/bin/ls
$ type which
which is a function
which ()
{
     ( alias;
     eval ${which_declare} ) | /usr/bin/which --tty-only --read-alias 
--read-functions --show-tilde --show-dot "$@"
}


  I think we can safely say: this is a mess :-)


  From this, I actually conclude that it is indeed safer to go to command -v, 
with a wrapper script that we add to $PATH to handle the case where make tries 
to bypass the shell.


  Regards,
  Arnout


> 
> bash:
> $ command -v ls uname
> alias ls='ls --color=auto'
> /usr/bin/uname
> 
> dash:
> $ command -v ls uname
> /usr/bin/ls
> 
> busybox sh
> $ command -v ls uname
> /usr/bin/ls
> 
>   I always used command -v with single argument in LTP and iputils, thus I
> didn't get this problem.
> 
> [1] https://packages.debian.org/search?suite=default&section=all&arch=any&searchon=contents&keywords=%2Fusr%2Fbin%2Fcommand
> [2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html
> [3] https://lore.kernel.org/buildroot/YVTIghzHs82uFBIe@pevik/T/#m95c17eb8374e4e3dd6eee700d397aa12cca0739e
> [4] https://savannah.gnu.org/projects/which
> 
>>>> So, I suggest that we do revert this patch, and work on a better
>>>> transition away from which, if at all. One very quick solution would be
>>>> to bundle our own which in Buildroot and then we'd have a quick way out
>>>> of that Debian's mess...
>>> Sure, if it causes problems which are not easily fixed, I'm not against
>>> reverting it. But I don't think that problem is that complex, that we'd need to
>>> compile which. But I apologize for causing troubles.
> 
>> And again, I want to reiterate that: you have no reason to apologise. :-)
>> Your patch was reviewed and applied, and there was no way we could have
>> found the issues above without trying in the first place.
> Yes, but next time I need testing on more distros, not just my laptop.
> 
>> But now, we can't keep this in the current state so, after discussing
>> this with Arnout, I am going to revert the patch.
> Sure, understand (I see you already reverted it).
> 
>> We can look at a better way to solve the Debian unstable issue about
>> which, probably the first being to open a bug with them, so that they
>> revert the warning, and second to find a way to no longer rely on which
>> from the distro (either by transitionning to something else, or by
>> bundling our own?).
> I'll need to find more time to investigate the problem to suggest some
> solution. I'd prefer to fix builtin shell than backport which (which is shell
> just on Debian, there is also C version in upstream which discontinued [4]).
> 
> 
>> Thanks for your work on Buildroot! :-)
> yw :)
> 
> Kind regards,
> Petr
> 
>> Regards,
>> Yann E. MORIN.


More information about the buildroot mailing list