utils: fix lxc_popen()/lxc_pclose()

- rework and fix pipe fd leak

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

src/lxc/utils.c

@@ -470,152 +470,105 @@ const char** lxc_va_arg_list_to_argv_const(va_list ap, size_t skip)
 	return (const char**)lxc_va_arg_list_to_argv(ap, skip, 0);
 }
 
-extern struct lxc_popen_FILE *lxc_popen(const char *command)
+struct lxc_popen_FILE *lxc_popen(const char *command)
 {
 	int ret;
-	struct lxc_popen_FILE *fp = NULL;
-	int parent_end = -1, child_end = -1;
 	int pipe_fds[2];
 	pid_t child_pid;
+	struct lxc_popen_FILE *fp = NULL;
 
-	int r = pipe2(pipe_fds, O_CLOEXEC);
-
-	if (r < 0) {
-		ERROR("pipe2 failure");
+	ret = pipe2(pipe_fds, O_CLOEXEC);
+	if (ret < 0)
 		return NULL;
-	}
-
-	parent_end = pipe_fds[0];
-	child_end = pipe_fds[1];
 
 	child_pid = fork();
+	if (child_pid < 0)
+		goto on_error;
 
-	if (child_pid == 0) {
-		/* child */
-
-		close(parent_end);
+	if (!child_pid) {
+		sigset_t mask;
 
-		if (child_end != STDOUT_FILENO) {
-			/* dup2() doesn't dup close-on-exec flag */
-			ret = dup2(child_end, STDOUT_FILENO);
-			if (ret < 0)
-				WARN("Failed to duplicate stdout fd");
-		} else {
-			/*
-			 * The descriptor is already the one we will use.
-			 * But it must not be marked close-on-exec.
-			 * Undo the effects.
-			 */
-			ret = fcntl(child_end, F_SETFD, 0);
-			if (ret < 0) {
-				SYSERROR("Failed to remove FD_CLOEXEC from fd.");
-				exit(127);
-			}
-		}
+		close(pipe_fds[0]);
 
-		if (child_end != STDERR_FILENO) {
-			/* dup2() doesn't dup close-on-exec flag */
-			ret = dup2(child_end, STDERR_FILENO);
-			if (ret < 0)
-				WARN("Failed to duplicate stdout fd");
-		} else {
-			/*
-			 * The descriptor is already the one we will use.
-			 * But it must not be marked close-on-exec.
-			 * Undo the effects.
-			 */
-			ret = fcntl(child_end, F_SETFD, 0);
-			if (ret < 0) {
-				SYSERROR("Failed to remove FD_CLOEXEC from fd.");
-				exit(127);
-			}
-		}
-
-		/*
-		 * Unblock signals.
-		 * This is the main/only reason
-		 * why we do our lousy popen() emulation.
-		 */
-		{
-			sigset_t mask;
-			sigfillset(&mask);
-			sigprocmask(SIG_UNBLOCK, &mask, NULL);
+		/* duplicate stdout */
+		if (pipe_fds[1] != STDOUT_FILENO)
+			ret = dup2(pipe_fds[1], STDOUT_FILENO);
+		else
+			ret = fcntl(pipe_fds[1], F_SETFD, 0);
+		if (ret < 0) {
+			close(pipe_fds[1]);
+			exit(EXIT_FAILURE);
 		}
 
-		execl("/bin/sh", "sh", "-c", command, (char *) NULL);
-		exit(127);
-	}
+		/* duplicate stderr */
+		if (pipe_fds[1] != STDERR_FILENO)
+			ret = dup2(pipe_fds[1], STDERR_FILENO);
+		else
+			ret = fcntl(pipe_fds[1], F_SETFD, 0);
+		close(pipe_fds[1]);
+		if (ret < 0)
+			exit(EXIT_FAILURE);
 
-	/* parent */
+		/* unblock all signals */
+		ret = sigfillset(&mask);
+		if (ret < 0)
+			exit(EXIT_FAILURE);
 
-	close(child_end);
+		ret = sigprocmask(SIG_UNBLOCK, &mask, NULL);
+		if (ret < 0)
+			exit(EXIT_FAILURE);
 
-	if (child_pid < 0) {
-		ERROR("fork failure");
-		goto error;
+		execl("/bin/sh", "sh", "-c", command, (char *)NULL);
+		exit(127);
 	}
 
-	fp = calloc(1, sizeof(*fp));
-	if (!fp) {
-		ERROR("failed to allocate memory");
-		goto error;
-	}
+	close(pipe_fds[1]);
+	pipe_fds[1] = -1;
 
-	fp->f = fdopen(parent_end, "r");
-	if (!fp->f) {
-		ERROR("fdopen failure");
-		goto error;
-	}
+	fp = malloc(sizeof(*fp));
+	if (!fp)
+		goto on_error;
 
 	fp->child_pid = child_pid;
+	fp->pipe = pipe_fds[0];
 
-	return fp;
-
-error:
+	fp->f = fdopen(pipe_fds[0], "r");
+	if (!fp->f)
+		goto on_error;
 
-	if (fp) {
-		if (fp->f) {
-			fclose(fp->f);
-			parent_end = -1; /* so we do not close it second time */
-		}
+	return fp;
 
+on_error:
+	if (fp)
 		free(fp);
-	}
 
-	if (parent_end != -1)
-		close(parent_end);
+	if (pipe_fds[0] >= 0)
+		close(pipe_fds[0]);
+
+	if (pipe_fds[1] >= 0)
+		close(pipe_fds[1]);
 
 	return NULL;
 }
 
-extern int lxc_pclose(struct lxc_popen_FILE *fp)
+int lxc_pclose(struct lxc_popen_FILE *fp)
 {
-	FILE *f = NULL;
-	pid_t child_pid = 0;
-	int wstatus = 0;
 	pid_t wait_pid;
+	int wstatus = 0;
 
-	if (fp) {
-		f = fp->f;
-		child_pid = fp->child_pid;
-		/* free memory (we still need to close file stream) */
-		free(fp);
-		fp = NULL;
-	}
-
-	if (!f || fclose(f)) {
-		ERROR("fclose failure");
+	if (!fp)
 		return -1;
-	}
 
 	do {
-		wait_pid = waitpid(child_pid, &wstatus, 0);
-	} while (wait_pid == -1 && errno == EINTR);
+		wait_pid = waitpid(fp->child_pid, &wstatus, 0);
+	} while (wait_pid < 0 && errno == EINTR);
+
+	close(fp->pipe);
+	fclose(fp->f);
+	free(fp);
 
-	if (wait_pid == -1) {
-		ERROR("waitpid failure");
+	if (wait_pid < 0)
 		return -1;
-	}
 
 	return wstatus;
 }

src/lxc/utils.h

@@ -190,6 +190,7 @@ static inline int signalfd(int fd, const sigset_t *mask, int flags)
  * without additional wrappers.
  */
 struct lxc_popen_FILE {
+	int pipe;
 	FILE *f;
 	pid_t child_pid;
 };