The branch main has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=702423e864a5a297713566ea74a62723a9c68a72
commit 702423e864a5a297713566ea74a62723a9c68a72
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-08-03 04:15:03 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-08-03 04:15:03 +0000
libc: gen: refactor execvPe() for readability
The current incarnation of execvPe() is a bit messy, and it can be
rather difficult to reason about whether we're actually doing the right
thing with our errors. We have two cases in which we may enter the loop:
1.) We have a name that has no slashes in it, and we enter the loop
normally through our strsep() logic to process $PATH
2.) We have a name with at least one slash, in which case we jump into
the middle of the loop then bail after precisely the one iteration
if we failed
Both paths will exit the loop if we failed, either via jumping to the
`done` label to preserve an errno or into the path that clobbers errno.
Clobbering errno for case #2 above would seem to be wrong, as we did not
actually search -- this would seem to be what POSIX expects, as well,
based on expectations of the conformance test suite.
Simplify reasoning about the two paths by splitting out an execvPe_prog
that does the execve(2) call specifically, and returns based on whether
the error would be fatal in a PATH search or not. For the
relative/absolute case, we can just ignore the return value and keep
errno intact. The search case gets simplified to return early if
we hit a fatal error, or continue until the end and clobber errno if
we did not find a suitable candidate.
Another posix_spawnp() test is added to confirm that we didn't break our
EACCES behavior in the process.
Reviewed by: des, markj
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D51629
---
lib/libc/gen/exec.c | 227 ++++++++++++++++++++++------------
lib/libc/tests/gen/posix_spawn_test.c | 50 ++++++++
2 files changed, 197 insertions(+), 80 deletions(-)
diff --git a/lib/libc/gen/exec.c b/lib/libc/gen/exec.c
index 926c50d64852..12020a79f6b4 100644
--- a/lib/libc/gen/exec.c
+++ b/lib/libc/gen/exec.c
@@ -32,8 +32,10 @@
#include "namespace.h"
#include <sys/param.h>
#include <sys/stat.h>
+#include <assert.h>
#include <errno.h>
#include <unistd.h>
+#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>
@@ -139,26 +141,129 @@ execvp(const char *name, char * const *argv)
return (__libc_execvpe(name, argv, environ));
}
+/*
+ * Returns 0 if we don't consider this a terminal condition, -1 if we do.
+ */
+static int
+execvPe_prog(const char *path, char * const *argv, char * const *envp)
+{
+ struct stat sb;
+ const char **memp;
+ size_t cnt;
+ int save_errno;
+
+ (void)_execve(path, argv, envp);
+ /* Grouped roughly by never terminal vs. usually terminal conditions */
+ switch (errno) {
+ case ELOOP:
+ case ENAMETOOLONG:
+ case ENOENT:
+ case ENOTDIR:
+ /* Non-terminal: property of the path we're trying */
+ break;
+ case ENOEXEC:
+ /*
+ * Failures here are considered terminal because we must handle
+ * this via the ENOEXEC fallback path; doing any further
+ * searching would be categorically incorrect.
+ */
+
+ for (cnt = 0; argv[cnt] != NULL; ++cnt)
+ ;
+
+ /*
+ * cnt may be 0 above; always allocate at least
+ * 3 entries so that we can at least fit "sh", path, and
+ * the NULL terminator. We can rely on cnt to take into
+ * account the NULL terminator in all other scenarios,
+ * as we drop argv[0].
+ */
+ memp = alloca(MAX(3, cnt + 2) * sizeof(char *));
+ assert(memp != NULL);
+ if (cnt > 0) {
+ memp[0] = argv[0];
+ memp[1] = path;
+ memcpy(&memp[2], &argv[1], cnt * sizeof(char *));
+ } else {
+ memp[0] = "sh";
+ memp[1] = path;
+ memp[2] = NULL;
+ }
+
+ (void)_execve(_PATH_BSHELL, __DECONST(char **, memp), envp);
+ return (-1);
+ case ENOMEM:
+ case E2BIG:
+ /* Terminal: persistent condition */
+ return (-1);
+ case ETXTBSY:
+ /*
+ * Terminal: we used to retry here, but sh(1) doesn't.
+ */
+ return (-1);
+ default:
+ /*
+ * EACCES may be for an inaccessible directory or
+ * a non-executable file. Call stat() to decide
+ * which. This also handles ambiguities for EFAULT
+ * and EIO, and undocumented errors like ESTALE.
+ * We hope that the race for a stat() is unimportant.
+ */
+ save_errno = errno;
+ if (stat(path, &sb) == -1) {
+ /*
+ * We force errno to ENOENT here to disambiguate the
+ * EACCESS case; the results of execve(2) are somewhat
+ * inconclusive because either the file did not exist or
+ * we just don't have search permissions, but the caller
+ * only really wants to see EACCES if the file did exist
+ * but was not accessible.
+ */
+ if (save_errno == EACCES)
+ errno = ENOENT;
+ break;
+ }
+
+ errno = save_errno;
+
+ /*
+ * Non-terminal: the file did exist and we just didn't have
+ * access to it, so we surface the EACCES and let the search
+ * continue for a candidate that we do have access to.
+ */
+ if (errno == EACCES)
+ break;
+
+ /*
+ * All other errors here are terminal, as prescribed by exec(3).
+ */
+ return (-1);
+ }
+
+ return (0);
+}
+
static int
execvPe(const char *name, const char *path, char * const *argv,
char * const *envp)
{
- const char **memp;
- size_t cnt, lp, ln;
- int eacces, save_errno;
char buf[MAXPATHLEN];
- const char *bp, *np, *op, *p;
- struct stat sb;
+ size_t ln, lp;
+ const char *np, *op, *p;
+ bool eacces;
- eacces = 0;
+ eacces = false;
/* If it's an absolute or relative path name, it's easy. */
- if (strchr(name, '/')) {
- bp = name;
- op = NULL;
- goto retry;
+ if (strchr(name, '/') != NULL) {
+ /*
+ * We ignore non-terminal conditions because we don't have any
+ * further paths to try -- we can just bubble up the errno from
+ * execve(2) here.
+ */
+ (void)execvPe_prog(name, argv, envp);
+ return (-1);
}
- bp = buf;
/* If it's an empty path name, fail in the usual POSIX way. */
if (*name == '\0') {
@@ -192,9 +297,13 @@ execvPe(const char *name, const char *path, char * const
*argv,
op = np + 1;
/*
- * If the path is too long complain. This is a possible
- * security issue; given a way to make the path too long
- * the user may execute the wrong program.
+ * If the path is too long, then complain. This is a possible
+ * security issue: given a way to make the path too long, the
+ * user may execute the wrong program.
+ *
+ * Remember to exercise caution here with assembling our final
+ * buf and any output, as we may be running in a vfork() context
+ * via posix_spawnp().
*/
if (lp + ln + 2 > sizeof(buf)) {
(void)_write(STDERR_FILENO, execvPe_err_preamble,
@@ -202,82 +311,40 @@ execvPe(const char *name, const char *path, char * const
*argv,
(void)_write(STDERR_FILENO, p, lp);
(void)_write(STDERR_FILENO, execvPe_err_trailer,
sizeof(execvPe_err_trailer) - 1);
+
continue;
}
- bcopy(p, buf, lp);
+
+ memcpy(&buf[0], p, lp);
buf[lp] = '/';
- bcopy(name, buf + lp + 1, ln);
+ memcpy(&buf[lp + 1], name, ln);
buf[lp + ln + 1] = '\0';
-retry: (void)_execve(bp, argv, envp);
- switch (errno) {
- case E2BIG:
- goto done;
- case ELOOP:
- case ENAMETOOLONG:
- case ENOENT:
- break;
- case ENOEXEC:
- for (cnt = 0; argv[cnt]; ++cnt)
- ;
-
- /*
- * cnt may be 0 above; always allocate at least
- * 3 entries so that we can at least fit "sh", bp, and
- * the NULL terminator. We can rely on cnt to take into
- * account the NULL terminator in all other scenarios,
- * as we drop argv[0].
- */
- memp = alloca(MAX(3, cnt + 2) * sizeof(char *));
- if (memp == NULL) {
- /* errno = ENOMEM; XXX override ENOEXEC? */
- goto done;
- }
- if (cnt > 0) {
- memp[0] = argv[0];
- memp[1] = bp;
- bcopy(argv + 1, memp + 2, cnt * sizeof(char *));
- } else {
- memp[0] = "sh";
- memp[1] = bp;
- memp[2] = NULL;
- }
- (void)_execve(_PATH_BSHELL,
- __DECONST(char **, memp), envp);
- goto done;
- case ENOMEM:
- goto done;
- case ENOTDIR:
- break;
- case ETXTBSY:
- /*
- * We used to retry here, but sh(1) doesn't.
- */
- goto done;
- default:
- /*
- * EACCES may be for an inaccessible directory or
- * a non-executable file. Call stat() to decide
- * which. This also handles ambiguities for EFAULT
- * and EIO, and undocumented errors like ESTALE.
- * We hope that the race for a stat() is unimportant.
- */
- save_errno = errno;
- if (stat(bp, &sb) != 0)
- break;
- if (save_errno == EACCES) {
- eacces = 1;
- continue;
- }
- errno = save_errno;
- goto done;
- }
+ /*
+ * For terminal conditions we can just return immediately. If
+ * it was non-terminal, we just need to note if we had an
+ * EACCES -- execvPe_prog would do a stat(2) and leave us with
+ * an errno of EACCES only if the file did exist; otherwise it
+ * would coerce it to an ENOENT because we may not know if a
+ * file actually existed there or not.
+ */
+ if (execvPe_prog(buf, argv, envp) == -1)
+ return (-1);
+ if (errno == EACCES)
+ eacces = true;
}
+
+ /*
+ * We don't often preserve errors encountering during the PATH search,
+ * so we override it here. ENOENT would be misleading if we found a
+ * candidate but couldn't access it, but most of the other conditions
+ * are either terminal or indicate that nothing was there.
+ */
if (eacces)
errno = EACCES;
else
errno = ENOENT;
-done:
+
return (-1);
}
diff --git a/lib/libc/tests/gen/posix_spawn_test.c
b/lib/libc/tests/gen/posix_spawn_test.c
index 77c3b5a569d9..22133cf1d59a 100644
--- a/lib/libc/tests/gen/posix_spawn_test.c
+++ b/lib/libc/tests/gen/posix_spawn_test.c
@@ -29,8 +29,11 @@
* IEEE Std. 1003.1-2008.
*/
+#include <sys/param.h>
+#include <sys/stat.h>
#include <sys/wait.h>
#include <errno.h>
+#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -38,6 +41,10 @@
#include <atf-c.h>
+static const char true_script[] =
+ "#!/usr/bin/env\n"
+ "/usr/bin/true\n";
+
char *myenv[2] = { "answer=42", NULL };
ATF_TC_WITHOUT_HEAD(posix_spawn_simple_test);
@@ -124,6 +131,48 @@ ATF_TC_BODY(posix_spawnp_enoexec_fallback_null_argv0, tc)
ATF_REQUIRE(error == EINVAL);
}
+ATF_TC(posix_spawnp_eacces);
+ATF_TC_HEAD(posix_spawnp_eacces, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "Verify EACCES behavior in
posix_spawnp");
+ atf_tc_set_md_var(tc, "require.user", "unprivileged");
+}
+ATF_TC_BODY(posix_spawnp_eacces, tc)
+{
+ const struct spawnp_eacces_tc {
+ const char *pathvar;
+ int error_expected;
+ } spawnp_eacces_tests[] = {
+ { ".", EACCES }, /* File exists, but not +x */
+ { "unsearchable", ENOENT }, /* File exists, dir not +x */
+ };
+ char *myargs[2] = { "eacces", NULL };
+ int error;
+
+ error = mkdir("unsearchable", 0755);
+ ATF_REQUIRE(error == 0);
+ error = symlink("/usr/bin/true", "unsearchable/eacces");
+ ATF_REQUIRE(error == 0);
+
+ (void)chmod("unsearchable", 0444);
+
+ /* this will create a non-executable file */
+ atf_utils_create_file("eacces", true_script);
+
+ for (size_t i = 0; i < nitems(spawnp_eacces_tests); i++) {
+ const struct spawnp_eacces_tc *tc = &spawnp_eacces_tests[i];
+ pid_t pid;
+
+ error = setenv("PATH", tc->pathvar, 1);
+ ATF_REQUIRE_EQ(0, error);
+
+ error = posix_spawnp(&pid, myargs[0], NULL, NULL, myargs,
+ myenv);
+ ATF_CHECK_INTEQ_MSG(tc->error_expected, error,
+ "path '%s'", tc->pathvar);
+ }
+}
+
ATF_TP_ADD_TCS(tp)
{
@@ -131,6 +180,7 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, posix_spawn_no_such_command_negative_test);
ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback);
ATF_TP_ADD_TC(tp, posix_spawnp_enoexec_fallback_null_argv0);
+ ATF_TP_ADD_TC(tp, posix_spawnp_eacces);
return (atf_no_error());
}
