[PATCH v2 properly indented] fix start_command() bug when stdin is closed

[Paolo Bonzini <bonzini@xxxxxxx>]

Karl Chen pointed out a problem in the use of dup2+close in
start_command() when one or more of file descriptors 0/1/2 are closed.
In order to rename a pipe file descriptor to 0, it does

    dup2(from, 0);
    close(from);

... but if stdin was closed (for example) from == 0, so that

    dup2(0, 0);
    close(0);

just ends up closing the pipe.  This patch fixes it by opening all of
the "low" descriptors to /dev/null.

In most cases this patch will not cause any additional system calls;
actually by reusing the /dev/null descriptor when possible (instead
of opening a fresh one in dup_devnull) it may even save a handful in
some cases. :-)

Signed-off-by: Paolo Bonzini <bonzini@xxxxxxx>
Acknowledged-by: Johannes Sixt <johannes.sixt@xxxxxxxxxx>
---
 run-command.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

	> "Karl Chen pointed out a problem..." (just to give due credit).

	Of course.

	> Except for the insane GNU style indentation  ;-)

	*blush* -- both problems deriving from too hasty e-mail cut&paste.

	> this makes a lot of sense. [...] MINGW32 [...] pass the test suite.

	Thanks, also for testing Windows.

	Paolo

diff --git a/run-command.c b/run-command.c
index caab374..4619494 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,25 +2,33 @@
 #include "run-command.h"
 #include "exec_cmd.h"
 
+static int devnull_fd = -1;
+
 static inline void close_pair(int fd[2])
 {
 	close(fd[0]);
 	close(fd[1]);
 }
 
-static inline void dup_devnull(int to)
-{
-	int fd = open("/dev/null", O_RDWR);
-	dup2(fd, to);
-	close(fd);
-}
-
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
 
 	/*
+	 * Make sure that all file descriptors <= 2 are open, otherwise we
+	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
+	 * we are at it, save a file descriptor on /dev/null to use it later.
+	 */
+	if (devnull_fd == -1) {
+		devnull_fd = open("/dev/null", O_RDWR);
+		while (devnull_fd >= 0 && devnull_fd <= 2)
+			devnull_fd = dup(devnull_fd);
+		if (devnull_fd == -1)
+			die("opening /dev/null failed (%s)", strerror(errno));
+	}
+
+	/*
 	 * In case of errors we must keep the promise to close FDs
 	 * that have been passed in via ->in and ->out.
 	 */
@@ -72,7 +81,7 @@ int start_command(struct child_process *cmd)
 	cmd->pid = fork();
 	if (!cmd->pid) {
 		if (cmd->no_stdin)
-			dup_devnull(0);
+			dup2(devnull_fd, 0);
 		else if (need_in) {
 			dup2(fdin[0], 0);
 			close_pair(fdin);
@@ -82,14 +91,14 @@ int start_command(struct child_process *cmd)
 		}
 
 		if (cmd->no_stderr)
-			dup_devnull(2);
+			dup2(devnull_fd, 2);
 		else if (need_err) {
 			dup2(fderr[1], 2);
 			close_pair(fderr);
 		}
 
 		if (cmd->no_stdout)
-			dup_devnull(1);
+			dup2(devnull_fd, 1);
 		else if (cmd->stdout_to_stderr)
 			dup2(2, 1);
 		else if (need_out) {
@@ -127,7 +136,7 @@ int start_command(struct child_process *cmd)
 
 	if (cmd->no_stdin) {
 		s0 = dup(0);
-		dup_devnull(0);
+		dup2(devnull_fd, 0);
 	} else if (need_in) {
 		s0 = dup(0);
 		dup2(fdin[0], 0);
@@ -138,7 +147,7 @@ int start_command(struct child_process *cmd)
 
 	if (cmd->no_stderr) {
 		s2 = dup(2);
-		dup_devnull(2);
+		dup2(devnull_fd, 2);
 	} else if (need_err) {
 		s2 = dup(2);
 		dup2(fderr[1], 2);
@@ -146,7 +155,7 @@ int start_command(struct child_process *cmd)
 
 	if (cmd->no_stdout) {
 		s1 = dup(1);
-		dup_devnull(1);
+		dup2(devnull_fd, 1);
 	} else if (cmd->stdout_to_stderr) {
 		s1 = dup(1);
 		dup2(2, 1);
-- 
1.5.5

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html