Libguestfs - Feb 2023 - [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()

On 2/16/23 00:33, Eric Blake wrote:
> On Wed, Feb 15, 2023 at 05:03:48PM -0600, Eric Blake wrote:
>>> +
>>> +# Create subdirectories for triggering non-fatal internal error
conditions of
>>> +# execvpe(). (Almost) every subdirectory will contain one entry,
called "f".
>>> +#
>>> +# Create a directory that's empty.
>>> +mkdir empty
>>> +
>>> +# Create a directory with a named pipe (FIFO) in it.
>>> +mkdir fifo
>>> +mkfifo fifo/f
>>> +
>>> +# Create a directory with a non-executable file in it.
>>> +mkdir nxregf
>>> +touch nxregf/f
>>> +
>>> +# Create a symlink loop.
>>> +ln -s symlink symlink
> 
> Another interesting thing you might want to add to the test:
> 
> mkdir -p subdir/f
> 
> then show that PATH=...:subdir:... does not get tripped up by
> subdir/f/ having the execute bit set (aka search bit since it's a
> directory) but not being an executable.
> 
Hmm. There could be two spots to test this (because what we're
triggering here is EACCES). One, *between* the following two tests:

run "" fifo/f;    execve_fail fifo/f    EACCES
run "" nxregf/f;  execve_fail nxregf/f  EACCES

because "fifo/f" is effectively the same test as "subdir/f"
-- not a
regular file in the first place.

Two, inside:

run empty:fifo:nxregf:symlink f
execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP

(again inserted between the fifo and the nxregf cases).

Now, while developing the test cases, I did (somewhat unwittingly) end
up testing what you propose, but then I decided that trying to execute a
directory was not much different from trying to execute a fifo -- the x
file mode bit shouldn't matter because the file type wouldn't be right
in the first place (the x bit should only matter on regular files).

So... Do you think "subdir/f" improves code coverage? If so, do you
agree that I should extend one (or both!) of the above two tests -- or
else, should I add something entirely new?

Thanks!
Laszlo

On Tue, Feb 21, 2023 at 02:06:33PM +0100, Laszlo Ersek
wrote:
> On 2/16/23 00:33, Eric Blake wrote:
> > On Wed, Feb 15, 2023 at 05:03:48PM -0600, Eric Blake wrote:
> >>> +
> >>> +# Create subdirectories for triggering non-fatal internal
error conditions of
> >>> +# execvpe(). (Almost) every subdirectory will contain one
entry, called "f".
> >>> +#
> >>> +# Create a directory that's empty.
> >>> +mkdir empty
> >>> +
> >>> +# Create a directory with a named pipe (FIFO) in it.
> >>> +mkdir fifo
> >>> +mkfifo fifo/f
> >>> +
> >>> +# Create a directory with a non-executable file in it.
> >>> +mkdir nxregf
> >>> +touch nxregf/f
> >>> +
> >>> +# Create a symlink loop.
> >>> +ln -s symlink symlink
> > 
> > Another interesting thing you might want to add to the test:
> > 
> > mkdir -p subdir/f
> > 
> > then show that PATH=...:subdir:... does not get tripped up by
> > subdir/f/ having the execute bit set (aka search bit since it's a
> > directory) but not being an executable.
> > 
> 
> Hmm. There could be two spots to test this (because what we're
> triggering here is EACCES). One, *between* the following two tests:
> 
> run "" fifo/f;    execve_fail fifo/f    EACCES
> run "" nxregf/f;  execve_fail nxregf/f  EACCES
> 
> because "fifo/f" is effectively the same test as
"subdir/f" -- not a
> regular file in the first place.
> 
> Two, inside:
> 
> run empty:fifo:nxregf:symlink f
> execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP
> 
> (again inserted between the fifo and the nxregf cases).
> 
> Now, while developing the test cases, I did (somewhat unwittingly) end
> up testing what you propose, but then I decided that trying to execute a
> directory was not much different from trying to execute a fifo -- the x
> file mode bit shouldn't matter because the file type wouldn't be
right
> in the first place (the x bit should only matter on regular files).
There's no reason for a fifo to ever have the x bit, but a directory
regularly has the search bit (which overlays the x bit) set.  I've
never accidentally attempted to execute a fifo, but I have, more than
once, asked to execute something I thought was a file only to have it
turn out to be a directory name.  See also commit b17fa77b in nbdkit,
where it actually hurt us (our shim script was sticking
/path/to/nbdkit first in PATH, but when we had /path/to/nbdkit/bash/
as a subdirectory, we actively triggered attempts to execute the
subdirectory instead of the intended /bin/bash).
> 
> So... Do you think "subdir/f" improves code coverage? If so, do
you
> agree that I should extend one (or both!) of the above two tests -- or
> else, should I add something entirely new?
I'd be happy with extending at least one of the above two tests, if
that's easier than adding something new.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org