svn commit: r362281 - in stable: 11/lib/libc/gen 11/lib/libc/tests/gen 12/lib/libc/gen 12/lib/libc/tests/gen
Kyle Evans
kevans at FreeBSD.org
Wed Jun 17 16:22:11 UTC 2020
Author: kevans
Date: Wed Jun 17 16:22:08 2020
New Revision: 362281
URL: https://svnweb.freebsd.org/changeset/base/362281
Log:
MFC r361995-r361996, r361999, r362111: posix_spawnp fixes
r361995:
execvp: fix up the ENOEXEC fallback
If execve fails with ENOEXEC, execvp is expected to rebuild the command
with /bin/sh instead and try again.
The previous version did this, but overlooked two details:
argv[0] can conceivably be NULL, in which case memp would never get
terminated. We must allocate no less than three * sizeof(char *) so we can
properly terminate at all times. For the non-NULL argv standard case, we
count all the non-NULL elements and actually skip the first argument, so we
end up capturing the NULL terminator in our bcopy().
The second detail is that the spec is actually worded such that we should
have been preserving argv[0] as passed to execvp:
"[...] executed command shall be as if the process invoked the sh utility
using execl() as follows:
execl(<shell path>, arg0, file, arg1, ..., (char *)0);
where <shell path> is an unspecified pathname for the sh utility, file is
the process image file, and for execvp(), where arg0, arg1, and so on
correspond to the values passed to execvp() in argv[0], argv[1], and so on."
So we make this change at this time as well, while we're already touching
it. We decidedly can't preserve a NULL argv[0] as this would be incredibly,
incredibly fragile, so we retain our legacy behavior of using "sh" for
argv[] in this specific instance.
Some light tests are added to try and detect some components of handling the
ENOEXEC fallback; posix_spawnp_enoexec_fallback_null_argv0 is likely not
100% reliable, but it at least won't raise false-alarms and it did result in
useful failures with pre-change libc on my machine.
This is a secondary change in D25038.
r361996:
execvPe: obviate the need for potentially large stack allocations
Some environments in which execvPe may be called have a limited amount of
stack available. Currently, it avoidably allocates a segment on the stack
large enough to hold PATH so that it may be mutated and use strsep() for
easy parsing. This logic is now rewritten to just operate on the immutable
string passed in and do the necessary math to extract individual paths,
since it will be copying out those segments to another buffer anyways and
piecing them together with the name for a full path.
Additional size is also needed for the stack in posix_spawnp(), because it
may need to push all of argv to the stack and rebuild the command with sh in
front of it. We'll make sure it's properly aligned for the new thread, but
future work should likely make rfork_thread a little easier to use by
ensuring proper alignment.
Some trivial cleanup has been done with a couple of error writes, moving
strings into char arrays for use with the less fragile sizeof().
r361999:
Add missing shell script from r361995
r362111:
posix_spawn: fix for some custom allocator setups
libc cannot assume that aligned_alloc and free come from jemalloc, or that
any application providing its own malloc and free is actually providing
aligned_alloc.
Switch back to malloc and just make sure we're passing a properly aligned
stack into rfork_thread, as an application perhaps can't reasonably replace
just malloc or just free without headaches.
This unbreaks ksh93 after r361996, which provides malloc/free but no
aligned_alloc.
Added:
stable/12/lib/libc/tests/gen/spawnp_enoexec.sh
- copied unchanged from r361999, head/lib/libc/tests/gen/spawnp_enoexec.sh
Modified:
stable/12/lib/libc/gen/exec.c
stable/12/lib/libc/gen/posix_spawn.c
stable/12/lib/libc/tests/gen/Makefile
stable/12/lib/libc/tests/gen/posix_spawn_test.c
Directory Properties:
stable/12/ (props changed)
Changes in other areas also in this revision:
Added:
stable/11/lib/libc/tests/gen/spawnp_enoexec.sh
- copied unchanged from r361999, head/lib/libc/tests/gen/spawnp_enoexec.sh
Modified:
stable/11/lib/libc/gen/exec.c
stable/11/lib/libc/gen/posix_spawn.c
stable/11/lib/libc/tests/gen/Makefile
stable/11/lib/libc/tests/gen/posix_spawn_test.c
Directory Properties:
stable/11/ (props changed)
Modified: stable/12/lib/libc/gen/exec.c
==============================================================================
--- stable/12/lib/libc/gen/exec.c Wed Jun 17 16:20:19 2020 (r362280)
+++ stable/12/lib/libc/gen/exec.c Wed Jun 17 16:22:08 2020 (r362281)
@@ -49,6 +49,9 @@ __FBSDID("$FreeBSD$");
extern char **environ;
+static const char execvPe_err_preamble[] = "execvP: ";
+static const char execvPe_err_trailer[] = ": path too long\n";
+
int
execl(const char *name, const char *arg, ...)
{
@@ -149,8 +152,8 @@ execvPe(const char *name, const char *path, char * con
const char **memp;
size_t cnt, lp, ln;
int eacces, save_errno;
- char *cur, buf[MAXPATHLEN];
- const char *p, *bp;
+ char buf[MAXPATHLEN];
+ const char *bp, *np, *op, *p;
struct stat sb;
eacces = 0;
@@ -158,7 +161,7 @@ execvPe(const char *name, const char *path, char * con
/* If it's an absolute or relative path name, it's easy. */
if (strchr(name, '/')) {
bp = name;
- cur = NULL;
+ op = NULL;
goto retry;
}
bp = buf;
@@ -169,34 +172,42 @@ execvPe(const char *name, const char *path, char * con
return (-1);
}
- cur = alloca(strlen(path) + 1);
- if (cur == NULL) {
- errno = ENOMEM;
- return (-1);
- }
- strcpy(cur, path);
- while ((p = strsep(&cur, ":")) != NULL) {
+ op = path;
+ ln = strlen(name);
+ while (op != NULL) {
+ np = strchrnul(op, ':');
+
/*
* It's a SHELL path -- double, leading and trailing colons
* mean the current directory.
*/
- if (*p == '\0') {
+ if (np == op) {
+ /* Empty component. */
p = ".";
lp = 1;
- } else
- lp = strlen(p);
- ln = strlen(name);
+ } else {
+ /* Non-empty component. */
+ p = op;
+ lp = np - op;
+ }
+ /* Advance to the next component or terminate after this. */
+ if (*np == '\0')
+ op = NULL;
+ else
+ 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 (lp + ln + 2 > sizeof(buf)) {
- (void)_write(STDERR_FILENO, "execvP: ", 8);
+ (void)_write(STDERR_FILENO, execvPe_err_preamble,
+ sizeof(execvPe_err_preamble) - 1);
(void)_write(STDERR_FILENO, p, lp);
- (void)_write(STDERR_FILENO, ": path too long\n",
- 16);
+ (void)_write(STDERR_FILENO, execvPe_err_trailer,
+ sizeof(execvPe_err_trailer) - 1);
continue;
}
bcopy(p, buf, lp);
@@ -215,14 +226,28 @@ retry: (void)_execve(bp, argv, envp);
case ENOEXEC:
for (cnt = 0; argv[cnt]; ++cnt)
;
- memp = alloca((cnt + 2) * sizeof(char *));
+
+ /*
+ * 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;
}
- memp[0] = "sh";
- memp[1] = bp;
- bcopy(argv + 1, memp + 2, cnt * sizeof(char *));
+ 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;
Modified: stable/12/lib/libc/gen/posix_spawn.c
==============================================================================
--- stable/12/lib/libc/gen/posix_spawn.c Wed Jun 17 16:20:19 2020 (r362280)
+++ stable/12/lib/libc/gen/posix_spawn.c Wed Jun 17 16:22:08 2020 (r362281)
@@ -30,6 +30,7 @@
__FBSDID("$FreeBSD$");
#include "namespace.h"
+#include <sys/param.h>
#include <sys/queue.h>
#include <sys/wait.h>
@@ -204,8 +205,20 @@ struct posix_spawn_args {
volatile int error;
};
+#define PSPAWN_STACK_ALIGNMENT 16
+#define PSPAWN_STACK_ALIGNBYTES (PSPAWN_STACK_ALIGNMENT - 1)
+#define PSPAWN_STACK_ALIGN(sz) \
+ (((sz) + PSPAWN_STACK_ALIGNBYTES) & ~PSPAWN_STACK_ALIGNBYTES)
+
#if defined(__i386__) || defined(__amd64__)
+/*
+ * Below we'll assume that _RFORK_THREAD_STACK_SIZE is appropriately aligned for
+ * the posix_spawn() case where we do not end up calling _execvpe and won't ever
+ * try to allocate space on the stack for argv[].
+ */
#define _RFORK_THREAD_STACK_SIZE 4096
+_Static_assert((_RFORK_THREAD_STACK_SIZE % PSPAWN_STACK_ALIGNMENT) == 0,
+ "Inappropriate stack size alignment");
#endif
static int
@@ -246,10 +259,36 @@ do_posix_spawn(pid_t *pid, const char *path,
pid_t p;
#ifdef _RFORK_THREAD_STACK_SIZE
char *stack;
+ size_t cnt, stacksz;
- stack = malloc(_RFORK_THREAD_STACK_SIZE);
+ stacksz = _RFORK_THREAD_STACK_SIZE;
+ if (use_env_path) {
+ /*
+ * We need to make sure we have enough room on the stack for the
+ * potential alloca() in execvPe if it gets kicked back an
+ * ENOEXEC from execve(2), plus the original buffer we gave
+ * ourselves; this protects us in the event that the caller
+ * intentionally or inadvertently supplies enough arguments to
+ * make us blow past the stack we've allocated from it.
+ */
+ for (cnt = 0; argv[cnt] != NULL; ++cnt)
+ ;
+ stacksz += MAX(3, cnt + 2) * sizeof(char *);
+ stacksz = PSPAWN_STACK_ALIGN(stacksz);
+ }
+
+ /*
+ * aligned_alloc is not safe to use here, because we can't guarantee
+ * that aligned_alloc and free will be provided by the same
+ * implementation. We've actively hit at least one application that
+ * will provide its own malloc/free but not aligned_alloc leading to
+ * a free by the wrong allocator.
+ */
+ stack = malloc(stacksz);
if (stack == NULL)
return (ENOMEM);
+ stacksz = (((uintptr_t)stack + stacksz) & ~PSPAWN_STACK_ALIGNBYTES) -
+ (uintptr_t)stack;
#endif
psa.path = path;
psa.fa = fa;
@@ -273,8 +312,7 @@ do_posix_spawn(pid_t *pid, const char *path,
* parent. Because of this, we must use rfork_thread instead while
* almost every other arch stores the return address in a register.
*/
- p = rfork_thread(RFSPAWN, stack + _RFORK_THREAD_STACK_SIZE,
- _posix_spawn_thr, &psa);
+ p = rfork_thread(RFSPAWN, stack + stacksz, _posix_spawn_thr, &psa);
free(stack);
#else
p = rfork(RFSPAWN);
Modified: stable/12/lib/libc/tests/gen/Makefile
==============================================================================
--- stable/12/lib/libc/tests/gen/Makefile Wed Jun 17 16:20:19 2020 (r362280)
+++ stable/12/lib/libc/tests/gen/Makefile Wed Jun 17 16:22:08 2020 (r362281)
@@ -24,6 +24,15 @@ ATF_TESTS_C+= wordexp_test
# TODO: t_siginfo (fixes require further inspection)
# TODO: t_sethostname_test (consistently screws up the hostname)
+FILESGROUPS+= posix_spawn_test_FILES
+
+posix_spawn_test_FILES= spawnp_enoexec.sh
+posix_spawn_test_FILESDIR= ${TESTSDIR}
+posix_spawn_test_FILESMODE= 0755
+posix_spawn_test_FILESOWN= root
+posix_spawn_test_FILESGRP= wheel
+posix_spawn_test_FILESPACKAGE= ${PACKAGE}
+
CFLAGS+= -DTEST_LONG_DOUBLE
# Not sure why this isn't defined for all architectures, since most
Modified: stable/12/lib/libc/tests/gen/posix_spawn_test.c
==============================================================================
--- stable/12/lib/libc/tests/gen/posix_spawn_test.c Wed Jun 17 16:20:19 2020 (r362280)
+++ stable/12/lib/libc/tests/gen/posix_spawn_test.c Wed Jun 17 16:22:08 2020 (r362281)
@@ -93,11 +93,50 @@ ATF_TC_BODY(posix_spawn_no_such_command_negative_test,
}
}
+ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback);
+ATF_TC_BODY(posix_spawnp_enoexec_fallback, tc)
+{
+ char buf[FILENAME_MAX];
+ char *myargs[2];
+ int error, status;
+ pid_t pid, waitres;
+
+ snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh",
+ atf_tc_get_config_var(tc, "srcdir"));
+ myargs[0] = buf;
+ myargs[1] = NULL;
+ error = posix_spawnp(&pid, myargs[0], NULL, NULL, myargs, myenv);
+ ATF_REQUIRE(error == 0);
+ waitres = waitpid(pid, &status, 0);
+ ATF_REQUIRE(waitres == pid);
+ ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42);
+}
+
+ATF_TC_WITHOUT_HEAD(posix_spawnp_enoexec_fallback_null_argv0);
+ATF_TC_BODY(posix_spawnp_enoexec_fallback_null_argv0, tc)
+{
+ char buf[FILENAME_MAX];
+ char *myargs[1];
+ int error, status;
+ pid_t pid, waitres;
+
+ snprintf(buf, sizeof(buf), "%s/spawnp_enoexec.sh",
+ atf_tc_get_config_var(tc, "srcdir"));
+ myargs[0] = NULL;
+ error = posix_spawnp(&pid, buf, NULL, NULL, myargs, myenv);
+ ATF_REQUIRE(error == 0);
+ waitres = waitpid(pid, &status, 0);
+ ATF_REQUIRE(waitres == pid);
+ ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == 42);
+}
+
ATF_TP_ADD_TCS(tp)
{
ATF_TP_ADD_TC(tp, posix_spawn_simple_test);
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);
return (atf_no_error());
}
Copied: stable/12/lib/libc/tests/gen/spawnp_enoexec.sh (from r361999, head/lib/libc/tests/gen/spawnp_enoexec.sh)
==============================================================================
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ stable/12/lib/libc/tests/gen/spawnp_enoexec.sh Wed Jun 17 16:22:08 2020 (r362281, copy of r361999, head/lib/libc/tests/gen/spawnp_enoexec.sh)
@@ -0,0 +1,4 @@
+# $FreeBSD$
+# Intentionally no interpreter
+
+exit 42
More information about the svn-src-stable-12
mailing list