Re: [PATCH] findprog: Support searching in a specified path string

[Paul Smith <psmith@gnu.org>]

On Sun, 2019-09-08 at 13:38 +0200, Bruno Haible wrote:
> > the path search is done
> > on the _parent's_ PATH environment value not the one passed in to
> > posix_spawnp().  I don't want that, I need the search done in the
> > child's environment.  I can't decide if this behavior by
> > posix_spawnp() is correct or not, and the POSIX spec isn't clear,
> > but it's how it works (at least on GNU/Linux).
> 
> For me, the POSIX spec [1] is clear: envp "constitute[s] the
> environment for the new process image". It does not say that the
> search for the program is done through envp.

It doesn't make clear where the search is done, one way or the other. 
But I agree there's no portability problem here.

> If there were many libc functions that take a '[const] char**'
> environment as argument, I would agree that this was the way to go.
> But there are no getenv, setenv, putenv variants that take a 'char
> **' arguments; so it appears that POSIX and libc APIs don't recognize
> 'char **' as a valuable data type.

Well, none of those functions need a char**.  But char** appears in a
number of other POSIX functions (any function that takes an environment
for example!  Which is what this function would do.)  However either
way is fine with me.

> >     prog = find_in_path_str ("myprog", lookup_path ());
> > 
> > and if lookup_path() returns NULL it defaults to the environment
> > PATH,
> 
> I don't think NULL should be interpreted as "look in the default
> PATH".  Rather, the convention is that an empty or null PATH means
> the current directory.

I find that VERY odd, and possibly a security risk.  According to
POSIX, the behavior if PATH is not present or is empty is
implementation-defined and my preference/expectation would be to behave
as if the program is not found.  However, that's a different
discussion.

> > rather than having to do something like:
> > 
> >     const char *path = lookup_path ();
> >     if (path)
> >       prog = find_in_path_str ("myprog", path);
> >     else
> >       prog = find_in_path ("myprog");
> 
> If the caller needs this logic, it should better be coded explicitly
> like this.
> 
> Note also that what you need is not merely an *attempt* to determine
> the filename of the executable, but you need it always - since you
> _don't_ want the program to be looked up in the parent's PATH. Thus
> you need also a return value that indicates that the program was not
> found in PATH.

There is already a return value to that effect in find_prog_in_path():
it returns the same pointer that was passed in.  I don't see why we
should make the interface any different for the new function.  It
allows the returned value to always be usable, avoids memory allocation
if not needed, and provides a simple way to check whether the file was
found or not.

> Otherwise,
> assume
>   parent PATH = "/bin:/usr/bin"
>   child PATH = "/tmp"
>   program = "cat"
> the find_in_path_str search would do a lookup in the child PATH, not
> find it, return "cat", and the posix_spawnp would then find "cat" in
> the parent PATH and invoke /bin/cat.

Sorry, I should have called it out more clearly, but in my email I said
that after this lookup we would invoke posix_spawn() (no "p").  You're
right, but if the user doesn't want the system to do the lookup she
simply won't use the posix_spawnp() method.  I wrote:

> > So what I want to do is run find_in_path_str() passing in the PATH
> > from the child's environment then pass the result to posix_spawn.

> This, in turn, means that we need to provide also an implementation
> for Windows, Cygwin, EMX, DOS.

Yes, clearly that would be ideal--it's not needed for my use-case since
I use it with posix_spawn() which obviously doesn't exist on these
other platforms, but additional portability would be a good thing.

The current implementation of find_prog_in_path() seems to use that
same reasoning to avoid implementing path search on these other
platforms: it apparently assumes that the calling code can just leave
it to the platform to resolve instead.

> And this means that it can't really share much with the existing
> findprog.c.  So the implementation should go into a different .c
> file.

I don't see why.  Path lookup would use identical methods so if it's
useful to have find_prog_in_path_str() implemented for all these other
platforms, why wouldn't that same code be used to allow
find_prog_in_path() to work properly for all these other platforms as
well?