This is version 4 of the following sub-series: [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe() [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()
20230215141158.2426855-10-lersek@redhat.com">http://mid.mail-archive.com/20230215141158.2426855-10-lersek@redhat.com 20230215141158.2426855-11-lersek@redhat.com">http://mid.mail-archive.com/20230215141158.2426855-11-lersek@redhat.com The Notes section on each patch records the updates for that patch. For assisting with incremental review, here's a range-diff: > 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 <ebl...@redhat.com> > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > + Reviewed-by: Eric Blake <ebl...@redhat.com> > + Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> > + > + > + ## 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 <ler...@redhat.com> > + Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> > + Reviewed-by: Eric Blake <ebl...@redhat.com> > + > + > + ## 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" > + Thanks for reviewing, Laszlo Laszlo Ersek (2): lib/utils: introduce async-signal-safe execvpe() lib/utils: add unit tests for async-signal-safe execvpe() .gitignore | 1 + lib/Makefile.am | 11 + lib/internal.h | 22 ++ lib/test-fork-safe-execvpe.c | 117 +++++++ lib/test-fork-safe-execvpe.sh | 277 +++++++++++++++ lib/utils.c | 355 ++++++++++++++++++++ 6 files changed, 783 insertions(+) create mode 100644 lib/test-fork-safe-execvpe.c create mode 100755 lib/test-fork-safe-execvpe.sh base-commit: 742cbd8c7adce91eb61b74524df3eb0180799653 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
