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

On Wed, Feb 15, 2023 at 03:11:39PM +0100, Laszlo Ersek wrote: > Don't try to test async-signal-safety, but strive to exercise as many as > possible paths through nbd_internal_execvpe_init() and > nbd_internal_fork_safe_execvpe(). > > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > lib/test-fork-safe-execvpe.c | 117 +++++++++ > lib/Makefile.am | 10 + > lib/test-fork-safe-execvpe.sh | 270 ++++++++++++++++++++ > .gitignore | 1 + > 4 files changed, 398 insertions(+) > > diff --git a/lib/test-fork-safe-execvpe.c b/lib/test-fork-safe-execvpe.c > new file mode 100644 > index 000000000000..09b91102b613 > --- /dev/null > +++ b/lib/test-fork-safe-execvpe.c > + > +/* This is a perror() replacement that makes the error message more > + * machine-readable, for a select few error numbers. Do not use it as a general > + * error() replacement, only upon nbd_internal_execvpe_init() and > + * nbd_internal_fork_safe_execvpe() failure. > + */ > +static void > +xperror (const char *s) Handy for the .sh counterpart that reads expected output. > + > +int > +main (int argc, char **argv) > +{ > + struct execvpe ctx; > + const char *prog_file; > + string_vector prog_argv; > + size_t i; > + > + if (argc < 3) { > + fprintf (stderr, "%1$s: usage: %1$s program-to-exec argv0 ...\n", argv[0]); > + return EXIT_FAILURE; > + } > + > + prog_file = argv[1]; > + > + /* For the argv of the program to execute, we need to drop our argv[0] (= our > + * own name) and argv[1] (= the program we need to execute), and to tack on a > + * terminating null pointer. Note that "argc" does not include the terminating > + * NULL. > + */ > + prog_argv = (string_vector)empty_vector; > + if (string_vector_reserve (&prog_argv, argc - 2 + 1) == -1) { > + perror ("string_vector_reserve"); > + return EXIT_FAILURE; > + } > + > + for (i = 2; i < argc; ++i) > + (void)string_vector_append (&prog_argv, argv[i]); > + (void)string_vector_append (&prog_argv, NULL); I don't know if I would have used the cast-to-void. But it doesn't hurt (this is a unit test and you are documenting that we are basically ignoring append errors as unlikely), and at this point it's more effort to rip than out than leave well enough alone.

> + > + if (nbd_internal_execvpe_init (&ctx, prog_file, prog_argv.len) == -1) { > + xperror ("nbd_internal_execvpe_init"); > + goto reset_prog_argv; > + } > + > + /* Print out the generated candidates. */ > + for (i = 0; i < ctx.pathnames.len; ++i) > + (void)fprintf (stdout, "%s\n", ctx.pathnames.ptr[i]); Peering into the internals. Naughty for normal code, but perfectly acceptable for the unit test.

> + > + if (fflush (stdout) == EOF) { > + perror ("fflush"); > + goto uninit_execvpe; > + } > + > + (void)nbd_internal_fork_safe_execvpe (&ctx, &prog_argv, environ); > + xperror ("nbd_internal_fork_safe_execvpe"); > + /* fall through */ > + > +uninit_execvpe: > + nbd_internal_execvpe_uninit (&ctx); > + > +reset_prog_argv: > + string_vector_reset (&prog_argv); > + > + return EXIT_FAILURE; > +} > +++ b/lib/test-fork-safe-execvpe.sh > + > +set -e > + > +# Determine the absolute pathname of the execvpe helper binary. The "realpath" > +# utility is not in POSIX, but Linux, FreeBSD and OpenBSD all have it. not in POSIX yet, but see https://www.austingroupbugs.net/view.php?id=1457

> +bname=$(basename -- "$0" .sh) > +dname=$(dirname -- "$0") > +execvpe=$(realpath -- "$dname/$bname") > + > +# This is an elaborate way to control the PATH variable around the $execvpe > +# helper binary as narrowly as possible. > +# > +# If $1 is "_", then the $execvpe helper binary is invoked with PATH unset. > +# Otherwise, the binary is invoked with PATH set to $1. > +# > +# $2 and onward are passed to $execvpe; note that $2 becomes *both* > +# "program-to-exec" for the helper *and* argv[0] for the program executed by the > +# helper. > +# > +# The command itself (including the PATH setting) is written to "cmd" (for error > +# reporting purposes only); the standard output and error are saved in "out" and > +# "err" respectively; the exit status is written to "status". This function > +# should never fail; if it does, then that's a bug in this unit test script, or > +# the disk is full etc. > +run() Looks nice. > + > +# Create a temporary directory and change the working directory to it. > +tmpd=$(mktemp -d) > +trap 'rm -r -- "$tmpd"' EXIT Our testsuite has cleanup_fn in functions.sh to make this sort of cleanup work more robust (more than just on EXIT).

From these, INT, TERM, EXIT and ERR should *already* be covered by just

> +cd "$tmpd" > + > +# If the "file" parameter of execvpe() is an empty string, then we must fail -- > +# in nbd_internal_execvpe_init() -- regardless of PATH. > +run _ ""; init_fail ENOENT > +run "" ""; init_fail ENOENT > +run . ""; init_fail ENOENT > + > +# 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 There are two places where ELOOP could matter: both in PATH (symlink/f can't resolve, regardless of the binary name we are looking up) and in the binary (./symlink or PATH=. symlink). Looking ahead, I'm seeing your tests of symlink as a directory name, but not as a binary name. But I don't know if adding it would increase coverage of your code, so much as coverage of the underlying execve() call.

> + > +# Create a directory with a (most likely) binary executable in it. > +mkdir bin > +expr_pathname=$(command -p -v expr) > +cp -- "$expr_pathname" bin/f > + > +# Create a directory with an executable shell script that does not contain a > +# shebang (#!). The script will print $0 and $1, and not depend on PATH for it. > +mkdir sh > +printf "command -p printf '%%s %%s\\\\n' \"\$0\" \"\$1\"\\n" >sh/f > +chmod u+x sh/f Based on your reactions to my comments on 09/29 about handling empty PATH= the same as PATH=., and on whether to allow the child process to chdir() and/or deal with a PATH element or binary name such as "+s" that could impact /bin/sh's behavior, there may be other cases you want to add to the testsuite.

> + > +# In the tests below, invoke each "f" such that the "file" parameter of > +# execvpe() contain a <slash> character. > +# > +# Therefore, PATH does not matter. Set it to the empty string. (Which in this > +# implementation would cause nbd_internal_execvpe_init() to fail with ENOENT, if > +# the "file" parameter didn't contain a <slash>.) > +run "" empty/f; execve_fail empty/f ENOENT > +run "" fifo/f; execve_fail fifo/f EACCES > +run "" nxregf/f; execve_fail nxregf/f EACCES > +run "" nxregf/f/; execve_fail nxregf/f/ ENOTDIR > +run "" symlink/f; execve_fail symlink/f ELOOP > + > +# This runs "expr 1 + 1". > +run "" bin/f 1 + 1; success bin/f,2 > + > +# This triggers the ENOEXEC branch in nbd_internal_fork_safe_execvpe(). > +# nbd_internal_fork_safe_execvpe() will first try > +# > +# execve("sh/f", {"sh/f", "arg1", NULL}, envp) > +# > +# hitting ENOEXEC. Then it will successfully call > +# > +# execve("/bin/sh", {"sh/f", "sh/f", "arg1", NULL}, envp) > +# > +# The shell script will get "sh/f" for $0 and "arg1" for $1, and print those > +# out. > +run "" sh/f arg1; success sh/f,"sh/f arg1" > + > +# In the tests below, the "file" parameter of execvpe() will not contain a > +# <slash> character. > +# > +# Show that PATH matters that way -- first, trigger an ENOENT in > +# nbd_internal_execvpe_init() by setting PATH to the empty string. > +run "" expr 1 + 1; init_fail ENOENT Another spot that might need tweaking based on your reaction to my comments on the previous patch. > + > +# Fall back to confstr(_CS_PATH) in nbd_internal_execvpe_init(), by unsetting > +# PATH. Verify the generated candidates by invoking "getconf PATH" here, and > +# appending "/expr" to each prefix. > +expected_output=$( > + path=$(command -p getconf PATH) We may have to check that getconf works, or skip the test if it doesn't (POSIX says it should exist, but not all the world is POSIX). Can be done as a followup if CI shows a failure.

> + IFS=: > + for p in $path; do > + printf '%s/%s\n' $p expr > + done > + command -p expr 1 + 1 > +) > +run _ expr 1 + 1; success "${expected_output//$'\n'/,}" > + > +# Continue with tests where the "file" parameter of execvpe() does not contain a > +# <slash> character, but now set PATH to explicit prefix lists. > +# > +# Show that, if the last candidate fails execve() with an error number that > +# would not be fatal otherwise, we do get that error number. > +run empty:fifo:nxregf:symlink f > +execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP > + > +# Put a single prefix in PATH, such that it lead to a successful execution. This leads

> +# exercises two things at the same time: (a) that nbd_internal_execvpe_init() > +# produces *one* candidate (i.e., that no <colon> is seen), and (b) that > +# nbd_internal_fork_safe_execvpe() succeeds for the *last* candidate. Repeat the > +# test with "expr" (called "f" under "bin") and the shell script (called "f" > +# under "sh", triggering the ENOEXEC branch). > +run bin f 1 + 1; success bin/f,2 > +run sh f arg1; success sh/f,"sh/f arg1" > + > +# Demonstrate that the order of candidates matters. The first invocation finds > +# "expr" (called "f" under "bin"), the second invocation finds the shell script > +# under "sh" (triggering the ENOEXEC branch). > +run empty:bin:sh f 1 + 1; success empty/f,bin/f,sh/f,2 > +run empty:sh:bin f arg1; success empty/f,sh/f,bin/f,"sh/f arg1" > + > +# Check the expansion of zero-length prefixes in PATH to ".", plus the > +# (non-)insertion of the "/" separator. > +run a/: f; execve_fail a/f,./f ENOENT > +run :a/ f; execve_fail ./f,a/f ENOENT > +run : f; execve_fail ./f,./f ENOENT > +pushd bin > +run : f 1 + 1; success ./f,./f,2 > +popd > +run :a/:::b/: f; execve_fail ./f,a/f,./f,./f,b/f,./f ENOENT Overall a nice unit test. Reviewed-by: Eric Blake <eblake(a)redhat.com&gt;