[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()

1: c5f89eaa0aaf ! 1: 2a3c95d0701f lib/utils: introduce async-signal-safe execvpe() @@ Commit message not to pass it, per APPLICATION USAGE [09], but on Linux/glibc, O_EXEC does not seem supported, only O_PATH does [10]. - Thus the chosen approach -- pre-generate filenames -- contains a small + Thus the chosen approach -- pre-generate filenames -- contains a small TOCTTOU race (highlighted by Eric) after all, but it should be harmless. Implementation-defined details: @@ Commit message If PATH is set but empty ("set to null") [02], or PATH is unset and confstr(_CS_PATH) fails or returns no information or returns the empty - string, we fail the initial scanning (!) with ENOENT. This is consistent - with bash's behavior on Linux/glibc: - - $ PATH= ls - bash: ls: No such file or directory + string, we fail the initial scanning (!) with ENOENT. Details chosen for unspecified behavior: @@ Commit message Suggested-by: Eric Blake <eblake(a)redhat.com&gt; Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; + Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; + + + ## Notes ## + v4: + + - remove double space typo from commit message [Eric] + + - remove the allusion to bash compatibility in the + "Implementation-defined details" part of the commit message, where the + latter discusses PATH being "set to null" [Eric] + + - pick up R-b's from Eric and Rich + + - keep the #include "..." list sorted -- #include "checked-overflow.h" + above "minmax.h", not below it + + - in get_path(), replace the FIXME comments with notes that explain why + we don't lock the environment [Eric] ## lib/internal.h ## @@ lib/internal.h: struct command { @@ lib/internal.h: extern void nbd_internal_fork_safe_assert (int result, const cha ## lib/utils.c ## @@ - #include <limits.h> + #include <sys/uio.h> - #include "minmax.h" + #include "array-size.h" +#include "checked-overflow.h" + #include "minmax.h" #include "internal.h" - @@ lib/utils.c: nbd_internal_fork_safe_assert (int result, const char *file, long line, - xwrite (STDERR_FILENO, "' failed.\n", 10); + assertion, "' failed.\n", (char *)NULL); abort (); } + @@ lib/utils.c: nbd_internal_fork_safe_assert (int result, const char *file, long l + bool env_path_found; + size_t path_size, path_size2; + -+ /* FIXME: lock the environment. */ ++ /* Note: per POSIX, here we should lock the environment, even just for ++ * getenv(). However, glibc and any other high-quality libc will not be ++ * modifying "environ" during getenv(), and no sane application should modify ++ * the environment after launching threads. ++ */ + path = getenv ("PATH"); + if ((env_path_found = (path != NULL))) + path = strdup (path); -+ /* FIXME: unlock the environment. */ ++ /* This is where we'd unlock the environment. */ + + if (env_path_found) { + /* This handles out-of-memory as well. */ 2: e8fba75ecf93 ! 2: 647a46b965c4 lib/utils: add unit tests for async-signal-safe execvpe() @@ Commit message nbd_internal_fork_safe_execvpe(). Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; + Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; + Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; + + + ## Notes ## + v4: + + - pick up R-b's from Rich and Eric + + - "errors.c" makes the test case dependent on pthread_getspecific(), so + reflect Eric's commit 742cbd8c7adc ("lib: Use PTHREAD_LIBS where + needed", 2023-03-17), that is, "xxx_LDADD = $(PTHREAD_LIBS)", to this + test case [thanks to Eric for that fixup BTW] + + - replace EXIT trap handler with cleanup_fn [Eric] + + - Create "subdir/f" as a directory, and extend two test scenarios to + show that "subdir/f", even though "f" has search (execute) permission, + results in EACCES (directly), and does not stop advancement through + PATH="...:subdir:..." (indirectly) [Eric]. Use "mkdir + mkdir" for + creating the "f" directory, rather than "mkdir -p", for symmetry with + "mkdir + mkfifo" before, and "mkdir + touch" after. + + - replace "<imperative>, such that <subjunctive>" with "<imperative>, + such that <indicative>" (= s/lead/leads/) [Eric] ## lib/test-fork-safe-execvpe.c (new) ## @@ @@ lib/Makefile.am: pkgconfig_DATA = libnbd.pc test_fork_safe_assert_SOURCES = \ @@ lib/Makefile.am: test_fork_safe_assert_SOURCES = \ - test-fork-safe-assert.c \ utils.c \ $(NULL) + test_fork_safe_assert_LDADD = $(PTHREAD_LIBS) + +test_fork_safe_execvpe_SOURCES = \ + $(top_srcdir)/common/utils/vector.c \ @@ lib/Makefile.am: test_fork_safe_assert_SOURCES = \ + test-fork-safe-execvpe.c \ + utils.c \ + $(NULL) ++test_fork_safe_execvpe_LDADD = $(PTHREAD_LIBS) ## lib/test-fork-safe-execvpe.sh (new) ## @@ @@ lib/test-fork-safe-execvpe.sh (new) +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + ++. ../tests/functions.sh ++ +set -e + +# Determine the absolute pathname of the execvpe helper binary. The "realpath" @@ lib/test-fork-safe-execvpe.sh (new) + +# Create a temporary directory and change the working directory to it. +tmpd=$(mktemp -d) -+trap 'rm -r -- "$tmpd"' EXIT ++cleanup_fn rm -r -- "$tmpd" +cd "$tmpd" + +# If the "file" parameter of execvpe() is an empty string, then we must fail -- @@ lib/test-fork-safe-execvpe.sh (new) +mkdir fifo +mkfifo fifo/f + ++# Create a directory with a directory in it. ++mkdir subdir ++mkdir subdir/f ++ +# Create a directory with a non-executable file in it. +mkdir nxregf +touch nxregf/f @@ lib/test-fork-safe-execvpe.sh (new) +# 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 "" subdir/f; execve_fail subdir/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 @@ lib/test-fork-safe-execvpe.sh (new) +# +# 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 ++run empty:fifo:subdir:nxregf:symlink f ++execve_fail empty/f,fifo/f,subdir/f,nxregf/f,symlink/f ELOOP + -+# Put a single prefix in PATH, such that it lead to a successful execution. This -+# 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). ++# Put a single prefix in PATH, such that it leads to a successful execution. ++# This 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" +