Handle ENOTDIR in pathexec_run

Handle ENOTDIR in pathexec_run

[Jacob Vosmaer via supervision]

Hello,

First time on this list. Long time fan of Runit.

We have recently run into an edge case in Runit, which I think might
be worth patching.

If a user starts up runsvdir with existing-but-not-directory garbage
in their PATH,
runsvdir ends up in a broken state with a cryptic error. This is how
you can reproduce it:

mkdir -p svcs/foo
cat >svcs/foo/run <<EOF
#!/bin/sh
echo hello world
while true; do
  sleep 1
done
EOF

chmod +x svcs/foo/run

PATH=/dev/null:$PATH runsvdir svcs

This will fail to launch runsv for svcs/foo. My understanding is this
happens because pathexec_run tries to call execve("/dev/null/runsv",
...) which fails with ENOTDIR.

Clearly this is a pathological PATH, but apparently there are people
out there who have this. And most other tools seem to ignore such
garbage entries. For example:

PATH=/dev/null:$PATH bash -c ls

The patch below makes runit ignore this type of bad path entry.

--- a/src/pathexec_run.c
+++ b/src/pathexec_run.c
@@ -35,7 +35,7 @@ void pathexec_run(const char *file,const char *
const *argv,const char * const *
     execve(tmp.s,argv,envp);
     if (errno != error_noent) {
       savederrno = errno;
-      if ((errno != error_acces) && (errno != error_perm) && (errno
!= error_isdir)) return;
+      if ((errno != error_acces) && (errno != error_perm) && (errno
!= error_isdir) && (errno != error_notdir)) return;
     }

     if (!path[split]) {

What do you think?

Best regards,

Jacob Vosmaer
GitLab, Inc.

Re: Handle ENOTDIR in pathexec_run

[Jonathan de Boyne Pollard]

Jacob Vosmaer:

> The patch below makes runit ignore this type of bad path entry.
> What do you think?
>

It's not good enough.

I was all set to apply this to djbwares, as the runit code for pathexec 
is actually Daniel J. Bernstein's original code.  But it highlighted 
some basic problems of running execve() in a loop to do a PATH search.

Simply put: One wants to (largely) ignore the error if there's a problem 
with the directory prefix from the search path; and one wants to retain 
the error if there's a problem with the final pathname component, the 
name that is being searched for or the actual node that it names.  
Unfortunately, execve() can return EACCESS in both cases, and simply 
deciding to exit the loop based upon nothing but the errno value is 
therefore an unworkable strategy.

If you look at the BSD C library in FreeBSD, you'll see that it attempts 
to distinguish between the two cases in its execvpe() by calling stat() 
on error.  If the stat() succeeds, then the problem cannot be with the 
directory prefix.  So it remembers the EACCESS, which must be a problem 
with the file itself.  If the stat() fails, then the problem is with the 
directory prefix (or with symbolic link traversal, hence the use of 
stat() not lstat()) and it largely ignores the EACCESS.

If you look at the internal_execve() function in the next release of the 
nosh toolset, you'll see that I've used a different design and made use 
of (relatively, they still being over a decade old) newer API functions, 
instead.  The code uses openat() on the path prefixes with O_DIRECTORY 
(and O_CLOEXEC), if that fails largely ignoring that error and 
continuing to the next PATH component.  It then uses, relative to that 
file descriptor, openat() with O_EXEC and then fexecve() for the final 
component (which it also this way does not have to do string 
concatenation with), retaining that error if that fails.  Things are 
slightly complicated by some trickiness relating to executable scripts 
with interpreters, but that's the gist of it.

pathexec_run() needs similar attention.

Re: Handle ENOTDIR in pathexec_run

[Laurent Bercot]

>Simply put: One wants to (largely) ignore the error if there's a problem with the directory prefix from the search path; and one wants to retain the error if there's a problem with the final pathname component, the name that is being searched for or the actual node that it names.  Unfortunately, execve() can return EACCESS in both cases, and simply deciding to exit the loop based upon nothing but the errno value is therefore an unworkable strategy.

Can you elaborate why you need to differentiate between error in the
directory prefix and error in the final pathname component? Why do you
think that retaining the error (and failing the pathexec) is better if
there is a problem with the final pathname component? IMO, if the
binary there is unsuitable for executing, it makes sense to move on
to the next path.

I'm going to add ENOTDIR support to the skalibs' pathexec_run; I'm
interested in knowing why you think it's not good enough.

--
Laurent