libc: popen: work around data corruption

vfork() would not save the registers that the parent would expect
to have restored after execl() completed.
Specially that execl() would call execve() underneath, further messing
up the stack of the parent.
To avoid that, we fork() for now. Later we will revisit and cleanup
vfork()+execve() to actually have vfork() store all the register
that the parent expects to see, and not those left by execve().

In the original code, looking at the registers just before the call to
popen(), and after the call showed that r7 would get clobbered.
This would leave the caller with an invalid pointer, leading to all
kinds of data corruptions.
execve() is simpler that execl() in this case.


Bug: 5336252
Change-Id: I3bf718c0bb4c0439f6f2753f153cdea14175be9c

1 files changed, 11 insertions, 14 deletions

diff --git a/libc/unistd/popen.c b/libc/unistd/popen.c
index 15f8325..c2355c1 100644
--- a/libc/unistd/popen.c
+++ b/libc/unistd/popen.c
@@ -48,6 +48,8 @@ static struct pid {
 	pid_t pid;
 } *pidlist;
 
+extern char **environ;
+
 FILE *
 popen(const char *program, const char *type)
 {
@@ -55,6 +57,7 @@ popen(const char *program, const char *type)
 	FILE *iop;
 	int pdes[2];
 	pid_t pid;
+	char *argp[] = {"sh", "-c", NULL, NULL};
 
 	if ((*type != 'r' && *type != 'w') || type[1] != '\0') {
 		errno = EINVAL;
@@ -69,7 +72,7 @@ popen(const char *program, const char *type)
 		return (NULL);
 	}
 
-	switch (pid = vfork()) {
+	switch (pid = fork()) {
 	case -1:			/* Error. */
 		(void)close(pdes[0]);
 		(void)close(pdes[1]);
@@ -80,24 +83,17 @@ popen(const char *program, const char *type)
 	    {
 		struct pid *pcur;
 		/*
-		 * because vfork() instead of fork(), must leak FILE *,
-		 * but luckily we are terminally headed for an execl()
+		 * We fork()'d, we got our own copy of the list, no
+		 * contention.
 		 */
 		for (pcur = pidlist; pcur; pcur = pcur->next)
 			close(fileno(pcur->fp));
 
 		if (*type == 'r') {
-			int tpdes1 = pdes[1];
-
 			(void) close(pdes[0]);
-			/*
-			 * We must NOT modify pdes, due to the
-			 * semantics of vfork.
-			 */
-			if (tpdes1 != STDOUT_FILENO) {
-				(void)dup2(tpdes1, STDOUT_FILENO);
-				(void)close(tpdes1);
-				tpdes1 = STDOUT_FILENO;
+			if (pdes[1] != STDOUT_FILENO) {
+				(void)dup2(pdes[1], STDOUT_FILENO);
+				(void)close(pdes[1]);
 			}
 		} else {
 			(void)close(pdes[1]);
@@ -106,7 +102,8 @@ popen(const char *program, const char *type)
 				(void)close(pdes[0]);
 			}
 		}
-		execl(_PATH_BSHELL, "sh", "-c", program, (char *)NULL);
+		argp[2] = (char *)program;
+		execve(_PATH_BSHELL, argp, environ);
 		_exit(127);
 		/* NOTREACHED */
 	    }