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

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.