Date: Wed, 17 Jun 2020 16:22:09 +0000 (UTC) From: Kyle Evans <kevans@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r362281 - in stable: 11/lib/libc/gen 11/lib/libc/tests/gen 12/lib/libc/gen 12/lib/libc/tests/gen Message-ID: <202006171622.05HGM9mY094884@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202006171622.05HGM9mY094884>