[Libguestfs] [libnbd PATCH v4 0/2] lib/utils: introduce async-signal-safe execvpe()
Laszlo Ersek
lersek at redhat.com
Sun Mar 19 09:41:37 UTC 2023
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()
http://mid.mail-archive.com/20230215141158.2426855-10-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 <eblake at redhat.com>
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> + Reviewed-by: Eric Blake <eblake at redhat.com>
> + Reviewed-by: Richard W.M. Jones <rjones at 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 <lersek at redhat.com>
> + Reviewed-by: Richard W.M. Jones <rjones at redhat.com>
> + Reviewed-by: Eric Blake <eblake at 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
More information about the Libguestfs
mailing list