From owner-svn-src-head@freebsd.org Wed Jun 10 01:32:14 2020 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 6E9343442A5; Wed, 10 Jun 2020 01:32:14 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49hTxV2PyBz4HXd; Wed, 10 Jun 2020 01:32:14 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4DCF81F353; Wed, 10 Jun 2020 01:32:14 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 05A1WE5K035982; Wed, 10 Jun 2020 01:32:14 GMT (envelope-from kevans@FreeBSD.org) Received: (from kevans@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 05A1WDD7035980; Wed, 10 Jun 2020 01:32:13 GMT (envelope-from kevans@FreeBSD.org) Message-Id: <202006100132.05A1WDD7035980@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kevans set sender to kevans@FreeBSD.org using -f From: Kyle Evans Date: Wed, 10 Jun 2020 01:32:13 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r361996 - head/lib/libc/gen X-SVN-Group: head X-SVN-Commit-Author: kevans X-SVN-Commit-Paths: head/lib/libc/gen X-SVN-Commit-Revision: 361996 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Jun 2020 01:32:14 -0000 Author: kevans Date: Wed Jun 10 01:32:13 2020 New Revision: 361996 URL: https://svnweb.freebsd.org/changeset/base/361996 Log: 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 Modified: head/lib/libc/gen/exec.c head/lib/libc/gen/posix_spawn.c Modified: head/lib/libc/gen/exec.c ============================================================================== --- head/lib/libc/gen/exec.c Wed Jun 10 01:30:37 2020 (r361995) +++ head/lib/libc/gen/exec.c Wed Jun 10 01:32:13 2020 (r361996) @@ -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); Modified: head/lib/libc/gen/posix_spawn.c ============================================================================== --- head/lib/libc/gen/posix_spawn.c Wed Jun 10 01:30:37 2020 (r361995) +++ head/lib/libc/gen/posix_spawn.c Wed Jun 10 01:32:13 2020 (r361996) @@ -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);