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

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2023-Mar-19 09:41 UTC

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

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 at redhat.com
http://mid.mail-archive.com/20230215141158.2426855-11-lersek at 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

Laszlo Ersek

2023-Mar-19 09:41 UTC

head link

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

execvp() [01] is powerful:

- it performs PATH search [02] if necessary,

- it falls back to executing the file with the shell as a shell script in
  case the underlying execv() or execve() fails with ENOEXEC.

However, execvp() is not async-signal-safe [03], and so we shouldn't call
it in a child process forked from a multi-threaded parent process [04],
which the libnbd client application may well be.

Introduce three APIs for safely emulating execvp() with execve():

- nbd_internal_execvpe_init(): to be called in the parent process, before
  fork(). Allocate and format candidate pathnames for execution with
  execve(), based on PATH. Allocate a buffer for the longer command line
  that the shell script fallback needs.

- nbd_internal_fork_safe_execvpe(): to be called in the child process. Try
  the candidates formatted previously in sequence with execve(), as long
  as execve() fails with errors related to pathname resolution /
  inode-level file type / execution permission. Stop iterating on fatal
  errors; if the fatal error is ENOEXEC, activate the shell script
  fallback. As a small added feature, take the environment from an
  explicit parameter (effectively imitating the GNU-specific execvpe()
  function -- cf. commit dc64ac5cdd0b, "states: Use execvp instead of
  execvpe", 2019-11-15).

- nbd_internal_execvpe_uninit(): to be called in the parent process, after
  fork(). Release the resources allocated with
  nbd_internal_execvpe_init().

The main idea with the above distribution of responsibilities is that any
pre-scanning of PATH -- which was suggested by Eric Blake -- should not
include activities that are performed by execve() anyway. I've considered
and abandoned two approaches:

- During scanning (i.e., in nbd_internal_execvpe_init()), check each
  candidate with access(X_OK) [05].

  I dropped this because the informative APPLICATION USAGE section of the
  same specification [06] advises against it, saying "[a]n application
  should instead attempt the action itself and handle the [EACCES] error
  that occurs if the file is not accessible".

- During scanning, open each candidate with O_EXEC [07] until one open()
  succeeds. Save the sole file descriptor for
  nbd_internal_fork_safe_execvpe(). In the latter, call fexecve() [08],
  which is free of all error conditions that necessitate iteration over
  candidates. Still implement the ENOEXEC (shell script) fallback.

  I dropped this because (a) fexecve() is a quite recent interface
  (highlighted by Eric); (b) for an application to open a file for
  execution with fexecve(), it's more robust to pass O_EXEC to open() than
  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
TOCTTOU race (highlighted by Eric) after all, but it should be harmless.

Implementation-defined details:

- If PATH searching is necessary, but PATH is absent [01], then fall back
  to confstr(_CS_PATH) [11], similarly to Linux/glibc execvpe() [12].

  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.

Details chosen for unspecified behavior:

- Hardwire "/bin/sh" as the shell's pathname [01].

[01] https://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html
[02]
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
[03]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
[04] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html
[05] https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html
[06]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html#tag_16_09_07
[07] https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
[08] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html
[09]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html#tag_16_111_07
[10] https://man7.org/linux/man-pages/man2/open.2.html
[11] https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html
[12] https://man7.org/linux/man-pages/man3/execvpe.3.html

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 |  22 ++
 lib/utils.c    | 355 ++++++++++++++++++++
 2 files changed, 377 insertions(+)

diff --git a/lib/internal.h b/lib/internal.h
index 7543f545cd95..35cb5e8994ee 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -368,6 +368,18 @@ struct command {
   uint32_t error; /* Local errno value */
 };
 
+struct execvpe {
+  string_vector pathnames;
+
+  /* Note: "const_string_vector" is not a good type for
"sh_argv" below. Even if
+   * we reserved enough space in a "const_string_vector",
+   * const_string_vector_append() would still not be async-signal-safe, due to
+   * the underlying const_string_vector_insert() calling assert().
+   */
+  char **sh_argv;
+  size_t num_sh_args;
+};
+
 /* Test if a callback is "null" or not, and set it to null. */
 #define CALLBACK_IS_NULL(cb)     ((cb).callback == NULL && (cb).free ==
NULL)
 #define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
@@ -542,4 +554,14 @@ extern void nbd_internal_fork_safe_assert (int result,
const char *file,
                                   __func__, #expression))
 #endif
 
+extern int nbd_internal_execvpe_init (struct execvpe *ctx, const char *file,
+                                      size_t num_args)
+  LIBNBD_ATTRIBUTE_NONNULL (1, 2);
+extern void nbd_internal_execvpe_uninit (struct execvpe *ctx)
+  LIBNBD_ATTRIBUTE_NONNULL (1);
+extern int nbd_internal_fork_safe_execvpe (struct execvpe *ctx,
+                                           const string_vector *argv,
+                                           char * const *envp)
+  LIBNBD_ATTRIBUTE_NONNULL (1, 2, 3);
+
 #endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index a8c6c217e2e4..22b466d48258 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -29,6 +29,7 @@
 #include <sys/uio.h>
 
 #include "array-size.h"
+#include "checked-overflow.h"
 #include "minmax.h"
 
 #include "internal.h"
@@ -505,3 +506,357 @@ nbd_internal_fork_safe_assert (int result, const char
*file, long line,
            assertion, "' failed.\n", (char *)NULL);
   abort ();
 }
+
+/* Returns the value of the PATH environment variable -- falling back to
+ * confstr(_CS_PATH) if PATH is absent -- as a dynamically allocated string. On
+ * failure, sets "errno" and returns NULL.
+ */
+static char *
+get_path (void)
+  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free)
+{
+  char *path;
+  bool env_path_found;
+  size_t path_size, path_size2;
+
+  /* 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);
+  /* This is where we'd unlock the environment. */
+
+  if (env_path_found) {
+    /* This handles out-of-memory as well. */
+    return path;
+  }
+
+  errno = 0;
+  path_size = confstr (_CS_PATH, NULL, 0);
+  if (path_size == 0) {
+    /* If _CS_PATH does not have a configuration-defined value, just store
+     * ENOENT to "errno".
+     */
+    if (errno == 0)
+      errno = ENOENT;
+
+    return NULL;
+  }
+
+  path = malloc (path_size);
+  if (path == NULL)
+    return NULL;
+
+  path_size2 = confstr (_CS_PATH, path, path_size);
+  assert (path_size2 == path_size);
+  return path;
+}
+
+/* nbd_internal_execvpe_init() and nbd_internal_fork_safe_execvpe() together
+ * present an execvp() alternative that is async-signal-safe.
+ *
+ * nbd_internal_execvpe_init() may only be called before fork(), for filling in
+ * the caller-allocated, uninitialized "ctx" structure. If
+ * nbd_internal_execvpe_init() succeeds, then fork() may be called.
+ * Subsequently, in the child process, nbd_internal_fork_safe_execvpe() may be
+ * called with the inherited "ctx" structure, while in the parent
process,
+ * nbd_internal_execvpe_uninit() must be called to uninitialize (evacuate) the
+ * "ctx" structure.
+ *
+ * On failure, "ctx" will not have been modified, "errno"
is set, and -1 is
+ * returned. Failures include:
+ *
+ * - Errors forwarded from underlying functions such as strdup(), confstr(),
+ *   malloc(), string_vector_append().
+ *
+ * - ENOENT: "file" is an empty string.
+ *
+ * - ENOENT: "file" does not contain a <slash> "/"
character, the PATH
+ *           environment variable is not set, and confstr() doesn't
associate a
+ *           configuration-defined value with _CS_PATH.
+ *
+ * - ENOENT: "file" does not contain a <slash> "/"
character, and: (a) the PATH
+ *           environment variable is set to the empty string, or (b) PATH is
not
+ *           set, and confstr() outputs the empty string for _CS_PATH.
+ *
+ * - EOVERFLOW: the sizes or counts of necessary objects could not be
expressed.
+ *
+ * - EINVAL: "num_args" is less than 2.
+ *
+ * On success, the "ctx" structure will have been filled in, and 0 is
returned.
+ *
+ * - "pathnames" member:
+ *
+ *   - All strings pointed-to by elements of the "pathnames"
string_vector
+ *     member are owned by "pathnames".
+ *
+ *   - If "file" contains a <slash> "/" character,
then the sole entry in
+ *     "pathnames" is a copy of "file".
+ *
+ *   - If "file" does not contain a <slash> "/"
character:
+ *
+ *     Let "system path" be defined as the value of the PATH
environment
+ *     variable, if the latter exists, and as the value output by confstr() for
+ *     _CS_PATH otherwise. Per the ENOENT specifications above, "system
path" is
+ *     a non-empty string. Let "system path" further be of the form
+ *
+ *       <prefix_0> [n = 1]
+ *
+ *     or
+ *
+ *       <prefix_0>:<prefix_1>:...:<prefix_(n-1)> [n >= 2]
+ *
+ *     where for each 0 <= i < n, <prefix_i> does not contain the
<colon> ":"
+ *     character. In the (n = 1) case, <prefix_0> is never empty (see
ENOENT
+ *     above), while in the (n >= 2) case, any individual <prefix_i>
may or may
+ *     not be empty.
+ *
+ *     The "pathnames" string_vector member has n elements; for each
0 <= i < n,
+ *     element i is of the form
+ *
+ *       suffix(curdir(<prefix_i>))file
+ *
+ *     where
+ *
+ *       curdir(x) := "."  if x = ""
+ *       curdir(x) := x    otherwise
+ *
+ *     and
+ *
+ *       suffix(x) := x      if "x" ends with a <slash>
"/"
+ *       suffix(x) := x "/"  otherwise
+ *
+ *     This transformation implements the POSIX XBD / PATH environment variable
+ *     semantics, creating candidate pathnames for execution by
+ *     nbd_internal_fork_safe_execvpe(). nbd_internal_fork_safe_execvpe() will
+ *     iterate over the candidate pathnames with execve() until execve()
+ *     succeeds, or fails with an error that is due to neither pathname
+ *     resolution, nor the candidate not being a regular file, nor the
candidate
+ *     lacking execution permission.
+ *
+ * - The "sh_argv" array member will have at least (num_args + 1)
elements
+ *   allocated, and none populated.
+ *
+ *   (The minimum value of "num_args" is 2 -- see EINVAL above.
According to
+ *   POSIX, "[t]he argument /arg0/ should point to a filename string that
is
+ *   associated with the process being started by one of the /exec/
functions",
+ *   plus "num_args" includes the null pointer that terminates the
argument
+ *   list.)
+ *
+ *   This allocation is made in anticipation of execve() failing for a
+ *   particular candidate inside nbd_internal_fork_safe_execvpe() with ENOEXEC
+ *   ("[t]he new process image file has the appropriate access permission
but
+ *   has an unrecognized format"). While that failure terminates the
iteration,
+ *   the failed call
+ *
+ *     execve (pathnames[i],
+ *             { argv[0], argv[1], ..., NULL }, // (num_args >= 2) elements
+ *             { envp[0], envp[1], ..., NULL })
+ *
+ *   must be repeated as
+ *
+ *     execve (<shell-path>,
+ *             { argv[0], pathnames[i],         // ((num_args + 1) >= 3)
+                 argv[1], ..., NULL },          // elements
+ *             { envp[0], envp[1], ..., NULL })
+ *
+ *   for emulating execvp(). The allocation in the "sh_argv" member
makes it
+ *   possible just to *link* the original "argv" elements and the
"pathnames[i]"
+ *   candidate into the right positions.
+ *
+ *   (POSIX leaves the shell pathname unspecified; "/bin/sh" should
be good
+ *   enough.)
+ *
+ *   The shell *binary* will see itself being executed under the name
"argv[0]",
+ *   will receive "pathnames[i]" as the pathname of the shell
*script* to read
+ *   and interpret ("command_file" in POSIX terminology), will expose
+ *   "pathnames[i]" as the positional parameter $0 to the script, and
will
+ *   forward "argv[1]" and the rest to the script as positional
parameters $1
+ *   and onward.
+ */
+int
+nbd_internal_execvpe_init (struct execvpe *ctx, const char *file,
+                           size_t num_args)
+{
+  int rc;
+  char *sys_path;
+  string_vector pathnames;
+  char *pathname;
+  size_t num_sh_args;
+  char **sh_argv;
+  size_t sh_argv_bytes;
+
+  rc = -1;
+
+  if (file[0] == '\0') {
+    errno = ENOENT;
+    return rc;
+  }
+
+  /* First phase. */
+  sys_path = NULL;
+  pathnames = (string_vector)empty_vector;
+
+  if (strchr (file, '/') == NULL) {
+    size_t file_len;
+    const char *sys_path_element, *scan;
+    bool finish;
+
+    sys_path = get_path ();
+    if (sys_path == NULL)
+      return rc;
+
+    if (sys_path[0] == '\0') {
+      errno = ENOENT;
+      goto free_sys_path;
+    }
+
+    pathname = NULL;
+    file_len = strlen (file);
+    sys_path_element = sys_path;
+    scan = sys_path;
+    do {
+      assert (sys_path_element <= scan);
+      finish = (*scan == '\0');
+      if (finish || *scan == ':') {
+        const char *sys_path_copy_start;
+        size_t sys_path_copy_size;
+        size_t sep_copy_size;
+        size_t pathname_size;
+        char *p;
+
+        if (scan == sys_path_element) {
+          sys_path_copy_start = ".";
+          sys_path_copy_size = 1;
+        } else {
+          sys_path_copy_start = sys_path_element;
+          sys_path_copy_size = scan - sys_path_element;
+        }
+
+        assert (sys_path_copy_size >= 1);
+        sep_copy_size = (sys_path_copy_start[sys_path_copy_size - 1] !=
'/');
+
+        if (ADD_OVERFLOW (sys_path_copy_size, sep_copy_size,
&pathname_size) ||
+            ADD_OVERFLOW (pathname_size, file_len, &pathname_size) ||
+            ADD_OVERFLOW (pathname_size, 1u, &pathname_size)) {
+          errno = EOVERFLOW;
+          goto empty_pathnames;
+        }
+
+        pathname = malloc (pathname_size);
+        if (pathname == NULL)
+          goto empty_pathnames;
+        p = pathname;
+
+        memcpy (p, sys_path_copy_start, sys_path_copy_size);
+        p += sys_path_copy_size;
+
+        memcpy (p, "/", sep_copy_size);
+        p += sep_copy_size;
+
+        memcpy (p, file, file_len);
+        p += file_len;
+
+        *p++ = '\0';
+
+        if (string_vector_append (&pathnames, pathname) == -1)
+          goto empty_pathnames;
+        /* Ownership transferred. */
+        pathname = NULL;
+
+        sys_path_element = scan + 1;
+      }
+
+      ++scan;
+    } while (!finish);
+  } else {
+    pathname = strdup (file);
+    if (pathname == NULL)
+      return rc;
+
+    if (string_vector_append (&pathnames, pathname) == -1)
+      goto empty_pathnames;
+    /* Ownership transferred. */
+    pathname = NULL;
+  }
+
+  /* Second phase. */
+  if (num_args < 2) {
+    errno = EINVAL;
+    goto empty_pathnames;
+  }
+  if (ADD_OVERFLOW (num_args, 1u, &num_sh_args) ||
+      MUL_OVERFLOW (num_sh_args, sizeof *sh_argv, &sh_argv_bytes)) {
+    errno = EOVERFLOW;
+    goto empty_pathnames;
+  }
+  sh_argv = malloc (sh_argv_bytes);
+  if (sh_argv == NULL)
+    goto empty_pathnames;
+
+  /* Commit. */
+  ctx->pathnames = pathnames;
+  ctx->sh_argv = sh_argv;
+  ctx->num_sh_args = num_sh_args;
+  rc = 0;
+  /* Fall through, for freeing temporaries. */
+
+empty_pathnames:
+  if (rc == -1) {
+    free (pathname);
+    string_vector_empty (&pathnames);
+  }
+
+free_sys_path:
+  free (sys_path);
+
+  return rc;
+}
+
+void
+nbd_internal_execvpe_uninit (struct execvpe *ctx)
+{
+  free (ctx->sh_argv);
+  ctx->num_sh_args = 0;
+  string_vector_empty (&ctx->pathnames);
+}
+
+int
+nbd_internal_fork_safe_execvpe (struct execvpe *ctx, const string_vector *argv,
+                                char * const *envp)
+{
+  size_t pathname_idx;
+
+  NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->pathnames.len > 0);
+
+  pathname_idx = 0;
+  do {
+    (void)execve (ctx->pathnames.ptr[pathname_idx], argv->ptr, envp);
+    if (errno != EACCES && errno != ELOOP && errno !=
ENAMETOOLONG &&
+        errno != ENOENT && errno != ENOTDIR)
+      break;
+
+    ++pathname_idx;
+  } while (pathname_idx < ctx->pathnames.len);
+
+  if (errno == ENOEXEC) {
+    char **sh_argp;
+    size_t argv_idx;
+
+    NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->num_sh_args >= argv->len);
+    NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->num_sh_args - argv->len == 1);
+
+    sh_argp = ctx->sh_argv;
+    *sh_argp++ = argv->ptr[0];
+    *sh_argp++ = ctx->pathnames.ptr[pathname_idx];
+    for (argv_idx = 1; argv_idx < argv->len; ++argv_idx)
+      *sh_argp++ = argv->ptr[argv_idx];
+
+    (void)execve ("/bin/sh", ctx->sh_argv, envp);
+  }
+
+  return -1;
+}

Laszlo Ersek

2023-Mar-19 09:41 UTC

head link

[Libguestfs] [libnbd PATCH v4 2/2] lib/utils: add unit tests for async-signal-safe execvpe()

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 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  | 117 +++++++++
 lib/Makefile.am               |  11 +
 lib/test-fork-safe-execvpe.sh | 277 ++++++++++++++++++++
 .gitignore                    |   1 +
 4 files changed, 406 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
@@ -0,0 +1,117 @@
+  /* nbd client library in userspace
+ * Copyright (C) 2013-2023 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+
+extern char **environ;
+
+/* 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)
+{
+  const char *err;
+
+  if (s != NULL && *s != '\0')
+    (void)fprintf (stderr, "%s: ", s);
+
+  switch (errno) {
+  case EACCES:
+    err = "EACCES";
+    break;
+  case ELOOP:
+    err = "ELOOP";
+    break;
+  case ENOENT:
+    err = "ENOENT";
+    break;
+  case ENOTDIR:
+    err = "ENOTDIR";
+    break;
+  default:
+    err = strerror (errno);
+  }
+  (void)fprintf (stderr, "%s\n", err);
+}
+
+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);
+
+  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]);
+
+  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;
+}
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 4fa02feef584..366fb74dbef1 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -33,6 +33,7 @@ EXTRA_DIST = \
 	$(generator_built) \
 	libnbd.syms \
 	test-fork-safe-assert.sh \
+	test-fork-safe-execvpe.sh \
 	$(NULL)
 
 lib_LTLIBRARIES = libnbd.la
@@ -105,10 +106,12 @@ pkgconfig_DATA = libnbd.pc
 
 TESTS = \
 	test-fork-safe-assert.sh \
+	test-fork-safe-execvpe.sh \
 	$(NULL)
 
 check_PROGRAMS = \
 	test-fork-safe-assert \
+	test-fork-safe-execvpe \
 	$(NULL)
 
 test_fork_safe_assert_SOURCES = \
@@ -118,3 +121,11 @@ test_fork_safe_assert_SOURCES = \
 	utils.c \
 	$(NULL)
 test_fork_safe_assert_LDADD = $(PTHREAD_LIBS)
+
+test_fork_safe_execvpe_SOURCES = \
+	$(top_srcdir)/common/utils/vector.c \
+	errors.c \
+	test-fork-safe-execvpe.c \
+	utils.c \
+	$(NULL)
+test_fork_safe_execvpe_LDADD = $(PTHREAD_LIBS)
diff --git a/lib/test-fork-safe-execvpe.sh b/lib/test-fork-safe-execvpe.sh
new file mode 100755
index 000000000000..5d2671946c66
--- /dev/null
+++ b/lib/test-fork-safe-execvpe.sh
@@ -0,0 +1,277 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2013-2023 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# 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"
+# utility is not in POSIX, but Linux, FreeBSD and OpenBSD all have it.
+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()
+{
+    local pathctl=$1
+    local program=$2
+    local exit_status
+
+    shift 1
+
+    if test _ = "$pathctl"; then
+        printf 'unset PATH; %s %s %s\n' "$execvpe"
"$program" "$*" >cmd
+        set +e
+        (
+            unset PATH
+            "$execvpe" "$program" "$@" >out
2>err
+        )
+        exit_status=$?
+        set -e
+    else
+        printf 'PATH=%s %s %s %s\n' "$pathctl"
"$execvpe" "$program" "$*" >cmd
+        set +e
+        PATH=$pathctl "$execvpe" "$program" "$@"
>out 2>err
+        exit_status=$?
+        set -e
+    fi
+    printf '%d\n' $exit_status >status
+}
+
+# After "run" returns, the following three functions can verify the
result.
+#
+# Check if the helper binary failed in nbd_internal_execvpe_init().
+#
+# $1 is the error number (a macro name such as ENOENT) that's expected of
+# nbd_internal_execvpe_init().
+init_fail()
+{
+    local expected_error="nbd_internal_execvpe_init: $1"
+    local cmd=$(< cmd)
+    local err=$(< err)
+    local status=$(< status)
+
+    if test 0 -eq "$status"; then
+        printf "'%s' should have failed\\n" "$cmd"
>&2
+        return 1
+    fi
+    if test x"$err" != x"$expected_error"; then
+        printf "'%s' should have failed with '%s', got
'%s'\\n" \
+               "$cmd" "$expected_error" "$err"
>&2
+        return 1
+    fi
+}
+
+# Check if the helper binary failed in nbd_internal_fork_safe_execvpe().
+#
+# $1 is the output (the list of candidate pathnames) that
+# nbd_internal_execvpe_init() is expected to produce; with inner
<newline>
+# characters replaced with <comma> characters, and the last
<newline> stripped.
+#
+# $2 is the error number (a macro name such as ENOENT) that's expected of
+# nbd_internal_fork_safe_execvpe().
+execve_fail()
+{
+    local expected_output=$1
+    local expected_error="nbd_internal_fork_safe_execvpe: $2"
+    local cmd=$(< cmd)
+    local out=$(< out)
+    local err=$(< err)
+    local status=$(< status)
+
+    if test 0 -eq "$status"; then
+        printf "'%s' should have failed\\n" "$cmd"
>&2
+        return 1
+    fi
+    if test x"$err" != x"$expected_error"; then
+        printf "'%s' should have failed with '%s', got
'%s'\\n" \
+               "$cmd" "$expected_error" "$err"
>&2
+        return 1
+    fi
+    out=${out//$'\n'/,}
+    if test x"$out" != x"$expected_output"; then
+        printf "'%s' should have output '%s', got
'%s'\\n" \
+               "$cmd" "$expected_output" "$out"
>&2
+        return 1
+    fi
+}
+
+# Check if the helper binary and the program executed by it succeeded.
+#
+# $1 is the output (the list of candidate pathnames) that
+# nbd_internal_execvpe_init() is expected to produce, followed by any output
+# expected of the program that's executed by the helper; with inner
<newline>
+# characters replaced with <comma> characters, and the last
<newline> stripped.
+success()
+{
+    local expected_output=$1
+    local cmd=$(< cmd)
+    local out=$(< out)
+    local status=$(< status)
+
+    if test 0 -ne "$status"; then
+        printf "'%s' should have succeeded\\n"
"$cmd" >&2
+        return 1
+    fi
+    out=${out//$'\n'/,}
+    if test x"$out" != x"$expected_output"; then
+        printf "'%s' should have output '%s', got
'%s'\\n" \
+               "$cmd" "$expected_output" "$out"
>&2
+        return 1
+    fi
+}
+
+# Create a temporary directory and change the working directory to it.
+tmpd=$(mktemp -d)
+cleanup_fn rm -r -- "$tmpd"
+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 directory in it.
+mkdir subdir
+mkdir subdir/f
+
+# Create a directory with a non-executable file in it.
+mkdir nxregf
+touch nxregf/f
+
+# Create a symlink loop.
+ln -s symlink symlink
+
+# 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
+
+# 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 "" 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
+
+# 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
+
+# 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)
+    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: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 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"
+
+# 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
diff --git a/.gitignore b/.gitignore
index 54bc20173f95..9df5d9f9db4c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -130,6 +130,7 @@ Makefile.in
 /lib/states.h
 /lib/test-fork-safe-assert
 /lib/test-fork-safe-assert.err
+/lib/test-fork-safe-execvpe
 /lib/unlocked.h
 /libtool
 /ltmain.sh

Eric Blake

2023-Mar-20 19:41 UTC

head link

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

On Sun, Mar 19, 2023 at 10:41:37AM +0100, Laszlo Ersek
wrote:> 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 at redhat.com
> http://mid.mail-archive.com/20230215141158.2426855-11-lersek at redhat.com
> 
> The Notes section on each patch records the updates for that patch.
> 
> For assisting with incremental review, here's a range-diff:
Thank you; the changes compared to v3 look sane, and my R-b still stands on both
patches.
> >       ## 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"
"$tmpd" probably starts with /, such that the -- is not strictly
necessary; but keep it (good practice, and saves us in case someone
sets $TMPDIR to an unusual relative name and mktemp does not give us a
leading / after all).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Maybe Matching Threads

Search for more possibly parallel threads

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

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

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

[Libguestfs] [libnbd PATCH v4 2/2] lib/utils: add unit tests for async-signal-safe execvpe()

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

Maybe Matching Threads