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

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);

lib/libc/gen/posix_spawn.c

@@ -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);