From f0fbdf1f4f2153f4c036f5ebd7c7717525437e25 Mon Sep 17 00:00:00 2001 From: Kyle Evans Date: Wed, 10 Jun 2020 01:32:13 +0000 Subject: [PATCH] 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(). Reported by: Andrew Gierth Reviewed by: jilles, kib, Andrew Gierth MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D25038 --- lib/libc/gen/exec.c | 45 ++++++++++++++++++++++++-------------- lib/libc/gen/posix_spawn.c | 34 +++++++++++++++++++++++++--- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/lib/libc/gen/exec.c b/lib/libc/gen/exec.c index 76c0454600eb..2c703bb4e73e 100644 --- a/lib/libc/gen/exec.c +++ b/lib/libc/gen/exec.c @@ -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 * const *argv, 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 * const *argv, /* 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,23 +172,30 @@ execvPe(const char *name, const char *path, char * const *argv, 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 @@ -193,10 +203,11 @@ execvPe(const char *name, const char *path, char * const *argv, * 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); diff --git a/lib/libc/gen/posix_spawn.c b/lib/libc/gen/posix_spawn.c index 02b13dc190e5..581d057c82ed 100644 --- a/lib/libc/gen/posix_spawn.c +++ b/lib/libc/gen/posix_spawn.c @@ -30,6 +30,7 @@ __FBSDID("$FreeBSD$"); #include "namespace.h" +#include #include #include @@ -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,8 +259,24 @@ 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); + } + stack = aligned_alloc(PSPAWN_STACK_ALIGNMENT, stacksz); if (stack == NULL) return (ENOMEM); #endif @@ -273,8 +302,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);