OS-6802 zoneadmd child process pollutes environment

Reviewed by: Patrick Mooney <patrick.mooney@joyent.com>
Reviewed by: John Levon <john.levon@joyent.com>
Approved by: Patrick Mooney <patrick.mooney@joyent.com>

2 files changed, 111 insertions, 9 deletions

diff --git a/usr/src/cmd/zoneadmd/log.c b/usr/src/cmd/zoneadmd/log.c
index e5df6bfd46..754df3c043 100644
--- a/usr/src/cmd/zoneadmd/log.c
+++ b/usr/src/cmd/zoneadmd/log.c
@@ -161,6 +161,7 @@ static logstream_t streams[MAX_LOG_STREAMS];
 static logfile_t logfiles[MAX_LOG_STREAMS];
 
 static boolean_t logging_initialized = B_FALSE;
+static boolean_t logging_poisoned = B_FALSE;
 static uint64_t logging_rot_size;		/* See ZLOG_MAXSZ */
 static uint64_t logging_rot_keep;		/* See ZLOG_KEEP */
 static int logging_pending_sig = 0;		/* Signal recvd while logging */
@@ -206,7 +207,7 @@ logstream_lock(void)
 {
 	int ret;
 
-	assert(logging_initialized);
+	assert(logging_initialized && !logging_poisoned);
 
 	ret = mutex_lock(&logging_lock);
 	assert(ret == 0);
@@ -325,6 +326,10 @@ logstream_sighandler(int sig)
 	}
 
 	logstream_lock();
+	if (logging_poisoned) {
+		logstream_unlock();
+		return;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(logfiles); i++) {
 		/* Inactive logfile slot */
@@ -379,6 +384,32 @@ get_attr_uint64(zlog_t *zlogp, zone_dochandle_t handle, const char *name,
 	*valp = val;
 }
 
+static void
+logstream_atfork_prepare(void)
+{
+	logstream_lock();
+}
+
+static void
+logstream_atfork_parent(void)
+{
+	logstream_unlock();
+}
+
+static void
+logstream_atfork_child(void)
+{
+	/*
+	 * This does just enough to cause any errant logging call in the child
+	 * to lead to a failed assertion.  logstream_init() must not be called
+	 * in the child.  It is expected that the child will be calling exec()
+	 * real soon.
+	 */
+	logging_poisoned = B_TRUE;
+	logging_pending_sig = 0;
+	logstream_unlock();
+}
+
 void
 logstream_init(zlog_t *zlogp)
 {
@@ -395,6 +426,10 @@ logstream_init(zlog_t *zlogp)
 		logfiles[i].lf_fd = -1;
 	}
 
+	err = pthread_atfork(logstream_atfork_prepare,
+	    logstream_atfork_parent, logstream_atfork_child);
+	assert(err == 0);
+
 	logging_initialized = B_TRUE;
 
 	/* Now it is safe to use zlogp */
diff --git a/usr/src/cmd/zoneadmd/zoneadmd.c b/usr/src/cmd/zoneadmd/zoneadmd.c
index 0ecba91a01..4599453d86 100644
--- a/usr/src/cmd/zoneadmd/zoneadmd.c
+++ b/usr/src/cmd/zoneadmd/zoneadmd.c
@@ -1018,6 +1018,8 @@ do_subproc(zlog_t *zlogp, char *cmdbuf, char **retstr, boolean_t debug)
 	FILE *file;
 	int status;
 	int rd_cnt;
+	int fds[2];
+	pid_t child;
 
 	if (retstr != NULL) {
 		if ((*retstr = malloc(1024)) == NULL) {
@@ -1030,17 +1032,73 @@ do_subproc(zlog_t *zlogp, char *cmdbuf, char **retstr, boolean_t debug)
 		inbuf = buf;
 	}
 
-	if (setup_subproc_env(debug) != Z_OK) {
-		zerror(zlogp, B_FALSE, "failed to setup environment");
+	if (pipe(fds) != 0) {
+		zerror(zlogp, B_TRUE, "failed to create pipe for subprocess");
 		return (-1);
 	}
 
-	file = popen(cmdbuf, "r");
-	if (file == NULL) {
-		zerror(zlogp, B_TRUE, "could not launch: %s", cmdbuf);
+	if ((child = fork()) == 0) {
+		int in;
+		struct sigaction sa = { 0 };
+
+		/*
+		 * SIGINT is currently ignored.  It probably shouldn't be so
+		 * hard to kill errant children, so we revert to SIG_DFL.
+		 * SIGHUP and SIGUSR1 are used to perform log rotation.  We
+		 * leave those as-is because we don't want a 'pkill -HUP
+		 * zoneadmd' to kill this child process before exec().  On
+		 * exec(), SIGHUP and SIGUSR1 will become SIG_DFL.
+		 */
+		sigset(SIGINT, SIG_DFL);
+
+		/*
+		 * Do not call zerror() in child process as neither zerror() nor
+		 * logstream_*() functions are designed to handle multiple
+		 * processes logging.  Rather, write all errors to the pipe.
+		 */
+		if (dup2(fds[1], STDERR_FILENO) == -1) {
+			(void) snprintf(buf, sizeof (buf),
+			    "subprocess failed to dup2(STDERR_FILENO): %s\n",
+			    strerror(errno));
+			(void) write(fds[1], buf, strlen(buf));
+			_exit(127);
+		}
+		if (dup2(fds[1], STDOUT_FILENO) == -1) {
+			perror("subprocess failed to dup2(STDOUT_FILENO)");
+			_exit(127);
+		}
+		/*
+		 * Some naughty children may try to read from stdin.  Be sure
+		 * that the first file that a child opens doesn't get stdin's
+		 * file descriptor.
+		 */
+		if ((in = open("/dev/null", O_RDONLY)) == -1 ||
+		    dup2(in, STDIN_FILENO) == -1) {
+			perror("subprocess failed to set up STDIN_FILENO");
+			_exit(127);
+		}
+		closefrom(STDERR_FILENO + 1);
+
+		if (setup_subproc_env(debug) != Z_OK) {
+			(void) fprintf(stderr, "failed to setup environment");
+			_exit(127);
+		}
+
+		(void) execl("/bin/sh", "sh", "-c", cmdbuf, NULL);
+
+		perror("subprocess execl failed");
+		_exit(127);
+	} else if (child == -1) {
+		zerror(zlogp, B_TRUE, "failed to create subprocess for '%s'",
+		    cmdbuf);
+		(void) close(fds[0]);
+		(void) close(fds[1]);
 		return (-1);
 	}
 
+	(void) close(fds[1]);
+
+	file = fdopen(fds[0], "r");
 	while (fgets(inbuf, 1024, file) != NULL) {
 		if (retstr == NULL) {
 			if (zlogp != &logsys) {
@@ -1056,15 +1114,24 @@ do_subproc(zlog_t *zlogp, char *cmdbuf, char **retstr, boolean_t debug)
 			rd_cnt += 1024 - 1;
 			if ((p = realloc(*retstr, rd_cnt + 1024)) == NULL) {
 				zerror(zlogp, B_FALSE, "out of memory");
-				(void) pclose(file);
-				return (-1);
+				break;
 			}
 
 			*retstr = p;
 			inbuf = *retstr + rd_cnt;
 		}
 	}
-	status = pclose(file);
+
+	while (fclose(file) != 0) {
+		assert(errno == EINTR);
+	}
+	while (waitpid(child, &status, 0) == -1) {
+		if (errno != EINTR) {
+			zerror(zlogp, B_TRUE,
+			    "failed to get exit status of '%s'", cmdbuf);
+			return (-1);
+		}
+	}
 
 	if (WIFSIGNALED(status)) {
 		zerror(zlogp, B_FALSE, "%s unexpectedly terminated due to "