svn commit: r361996 - head/lib/libc/gen
Kyle Evans
kevans at FreeBSD.org
Wed Jun 10 01:32:14 UTC 2020
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 <andrew_tao173.riddles.org.uk>
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 <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,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);
More information about the svn-src-head
mailing list