[PATCH] Fix start_command() pipe bug when stdin is closed.

[PATCH] Fix start_command() pipe bug when stdin is closed.

[Karl Chen]

I ran into what I think is a bug:
    sh$ git fetch 0<&-

(i.e. run git-fetch with stdin closed.)
It aborts with:
    fatal: read error (Bad file descriptor)

I think the problem arises from the use of dup2+close in
start_command().  It wants to rename a pipe file descriptor to 0,
so it does
    dup2(from, to);
    close(from);

... but in this case from == to == 0, so 
    dup2(0, 0);
    close(0);
just ends up closing the pipe.

The patch below fixes the problem for me.


>From 78446c82131a5ca7f22f92bc32d7f3036bba9629 Mon Sep 17 00:00:00 2001
From: Karl Chen <quarl@quarl.org>
Date: Mon, 25 Aug 2008 01:09:08 -0700
Subject: [PATCH] Fix start_command() pipe bug when stdin is closed.

When intending to rename a fd to 0, if the fd is already 0, then do nothing,
instead of dup2(0,0); close(0);

The problematic behavior could be seen thus: git-fetch 0<&-

Signed-off-by: Karl Chen <quarl@quarl.org>

---
 run-command.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/run-command.c b/run-command.c
index caab374..b4bd80f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,11 +8,18 @@ static inline void close_pair(int fd[2])
 	close(fd[1]);
 }
 
+static inline void rename_fd(int from, int to)
+{
+	if (from != to) {
+		dup2(from, to);
+		close(from);
+	}
+}
+
 static inline void dup_devnull(int to)
 {
 	int fd = open("/dev/null", O_RDWR);
-	dup2(fd, to);
-	close(fd);
+	rename_fd(fd, to);
 }
 
 int start_command(struct child_process *cmd)
@@ -74,18 +81,17 @@ int start_command(struct child_process *cmd)
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
-			dup2(fdin[0], 0);
-			close_pair(fdin);
+			rename_fd(fdin[0], 0);
+			close(fdin[1]);
 		} else if (cmd->in) {
-			dup2(cmd->in, 0);
-			close(cmd->in);
+			rename_fd(cmd->in, 0);
 		}
 
 		if (cmd->no_stderr)
 			dup_devnull(2);
 		else if (need_err) {
-			dup2(fderr[1], 2);
-			close_pair(fderr);
+			rename_fd(fderr[1], 2);
+			close(fderr[0]);
 		}
 
 		if (cmd->no_stdout)
@@ -93,11 +99,10 @@ int start_command(struct child_process *cmd)
 		else if (cmd->stdout_to_stderr)
 			dup2(2, 1);
 		else if (need_out) {
-			dup2(fdout[1], 1);
-			close_pair(fdout);
+			rename_fd(fdout[1], 1);
+			close(fdout[0]);
 		} else if (cmd->out > 1) {
-			dup2(cmd->out, 1);
-			close(cmd->out);
+			rename_fd(cmd->out, 1);
 		}
 
 		if (cmd->dir && chdir(cmd->dir))
-- 
1.5.6.2

Re: [PATCH] Fix start_command() pipe bug when stdin is closed.

[Johannes Sixt]

Karl Chen schrieb:
> I ran into what I think is a bug:
>     sh$ git fetch 0<&-
> 
> (i.e. run git-fetch with stdin closed.)
> It aborts with:
>     fatal: read error (Bad file descriptor)

When I try these instructions I don't get an error; instead the command
runs successfully.

> I think the problem arises from the use of dup2+close in
> start_command().  It wants to rename a pipe file descriptor to 0,
> so it does
>     dup2(from, to);
>     close(from);
> 
> ... but in this case from == to == 0, so 
>     dup2(0, 0);
>     close(0);
> just ends up closing the pipe.

While I do see that there is a problem, it is only half of the story, and
your patch addresses only this half.

What if stdout is closed, too? Then the ends of the first allocated pipe
would go to fds 0 and  1, and then the pipe end at 1 would be closed by a
subsequent dup2(xxx, 1).

Junio, what's your take on this?

-- Hannes

Re: [PATCH] Fix start_command() pipe bug when stdin is closed.

[Paolo Bonzini]

> While I do see that there is a problem, it is only half of the story, and
> your patch addresses only this half.
> 
> What if stdout is closed, too? Then the ends of the first allocated pipe
> would go to fds 0 and  1, and then the pipe end at 1 would be closed by a
> subsequent dup2(xxx, 1).

What about opening files (in start_command, protected by a loop that run
only once, or on startup) until you get a descriptor that is > 2?  Like
this:

  static int low_fds_reserved;
  if (!low_fds_reserved)
    {
      int fd = open("/dev/null", O_RDWR);
      while (fd >= 0 && fd <= 2)
        fd = dup (fd);
      if (fd != -1)
        close (fd);
      else
        perror ("start_command");
      low_fds_reserved = 1;
    }

Paolo

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

[Paolo Bonzini]

There is 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@gnu.org>
---
 run-command.c |   35 ++++++++++++++++++++++-------------
 1 files changed, 22 insertions(+), 13 deletions(-)

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,34 @@
 #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, open 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

Re: [PATCH v2] fix start_command() bug when stdin is closed

[Johannes Sixt]

Paolo Bonzini schrieb:
> There is a problem in the use of dup2+close in start_command()
> when one or more of file descriptors 0/1/2 are closed.

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

>  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, open 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));
> +	  }

Except for the insane GNU style indentation ;-) this makes a lot of sense.

Acked-by: Johannes Sixt <johannes.sixt@telecom.at>

The changes to the MINGW32 section are good (they pass the test suite).
Thanks for taking care of that.

-- Hannes

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

[Paolo Bonzini]

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@gnu.org>
Acknowledged-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 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

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

[Karl Chen]

>>>>> On 2008-08-25 06:37 PDT, Paolo Bonzini writes:

    Paolo> diff --git a/run-command.c b/run-command.c index
    Paolo> caab374..4619494 100644

Excellent, this also fixes the problem for me.

Acknowledged-by: Karl Chen <quarl@quarl.org>

(I guess that's the protocol...)


Wow, turnaround on this list sure is fast.  Thanks, guys!

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

[Junio C Hamano]

Karl Chen <quarl@cs.berkeley.edu> writes:

>>>>>> On 2008-08-25 06:37 PDT, Paolo Bonzini writes:
>
>     Paolo> diff --git a/run-command.c b/run-command.c index
>     Paolo> caab374..4619494 100644
>
> Excellent, this also fixes the problem for me.
>
> Acknowledged-by: Karl Chen <quarl@quarl.org>
>
> (I guess that's the protocol...)
>
>
> Wow, turnaround on this list sure is fast.  Thanks, guys!

Thanks.

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

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

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

I may be misreading the patch but, this logic always opens /dev/null, if
nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
worse, it keeps fd=3 open.

Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
clobbering fd=3 is good.

Also shouldn't this be done only on the side that dup()s fds around,
i.e. in the child process after fork()?  Why is this done for the parent?

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

[Johannes Sixt]

Junio C Hamano schrieb:
> Paolo Bonzini <bonzini@gnu.org> writes:
> 
>>  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));
>> +	}
>> +
> 
> I may be misreading the patch but, this logic always opens /dev/null, if
> nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
> worse, it keeps fd=3 open.
> 
> Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
> clobbering fd=3 is good.

It is sometimes _unnecessary_, but I don't see why it should hurt. The
effect on performance will be in the noise.

> Also shouldn't this be done only on the side that dup()s fds around,
> i.e. in the child process after fork()?  Why is this done for the parent?

Because it must be done *before* the pipe()s are created so that they
don't occupy fds 0-2.

-- Hannes

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

[Paolo Bonzini]

Hannes already answered everything, but now that I think more about it I
would actually consider putting it in main, just in case an important
file ends up in file descriptor = 2 and is corrupted by a call to die().
 Just in case, this program shows that stderr always point to fd 2 even
if it is closed upon launch:

  #include <stdio.h>
  #include <fcntl.h>

  int main()
  {
    int fd = open ("/dev/tty", O_WRONLY);
    FILE *fp;
    fp = fdopen (fd, "w");
    fprintf (fp, "file descriptor %d\n", fd);
    fflush (fp);
    fprintf (stderr, "writing on stderr now\n");
  }

  bonzinip$ ./a.out 2<&-
  file descriptor 2
  writing on stderr now
  bonzinip$

Patch coming in a moment.

Paolo

[PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

It is in general unsafe to start git with one or more of file descriptors
0/1/2 closed.  Karl Chen for example noticed that stat_command does this
in order to rename a pipe file descriptor to 0:

    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.  Another extremely rare but nasty problem
would occur if an "important" file ends up in file descriptor 2, and is
corrupted by a call to die().

This patch fixes these problems by opening all of the "low" descriptors
to /dev/null in main.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 git.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/git.c b/git.c
index 89e4645..be227b2 100644
--- a/git.c
+++ b/git.c
@@ -420,6 +420,19 @@ int main(int argc, const char **argv)
 	const char *cmd = argv[0] && *argv[0] ? argv[0] : "git-help";
 	char *slash = (char *)cmd + strlen(cmd);
 	int done_alias = 0;
+	int devnull_fd;
+
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	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));
+	close (devnull_fd);
 
 	/*
 	 * Take the basename of argv[0] as the command
-- 
1.5.5

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Johannes Sixt]

Paolo Bonzini schrieb:
> +	/*
> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
> +	 * in die().  It also avoids not messing up when the pipes are
> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
> +	 */

I see your point, but I don't have an opinion whether this stretch is
necessary.

However, *if* we do this, we must do it for all non-builtins as well!

-- Hannes

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Johannes Sixt wrote:
>Paolo Bonzini schrieb:
>> +	/*
>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>> +	 * in die().  It also avoids not messing up when the pipes are
>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>> +	 */

>I see your point, but I don't have an opinion whether this stretch is
>necessary.
>However, *if* we do this, we must do it for all non-builtins as well!

Well, in general the policy I've used in all the tools I created is that:

a. If it's a setuid tool, then you need to make sure that you don't step
   on anything unintendedly.  I.e. for setuid-something programs this is
   desirable and necessary in order to prevent securityleaks.

b. Anything else is started in an environment controlled by the user,
   and if this environment is broken, then that is the user's fault.
   You get what you wish for.  It's a similar problem you get when you
   set PATH to wrong values and then start "make" for example; it has
   the potential to break a lot; but then again there are infinitely
   more ways to shoot yourself in the foot, than there are ways to
   prevent people from shooting in some particular way.

So I'd say, if the tools are setuid (which none of git's tools are) and
are therefore potentially started from a hostile and uncontrolled
environment, please make sure filedescriptors 0, 1 and 2 are sane.
But for the git utilities, it would be a non-watertight extra safeguard
which tries to prevent a situation which rarely occurs and if it does
occur, you probably are doing some other things wrong as well; so
actually exposing those problems to you by letting you feel the pain can
be considered a favour.
-- 
Sincerely,
           Stephen R. van den Berg.

"Good moaning!"

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Avery Pennarun]

On 8/26/08, Stephen R. van den Berg <srb@cuci.nl> wrote:
> Well, in general the policy I've used in all the tools I created is that:
>
>  a. If it's a setuid tool, then you need to make sure that you don't step
>    on anything unintendedly.  I.e. for setuid-something programs this is
>    desirable and necessary in order to prevent securityleaks.
>
>  b. Anything else is started in an environment controlled by the user,
>    and if this environment is broken, then that is the user's fault.

In general I'd mostly agree with you, but fd 0/1/2 are super-special
and I've personally been bitten by insane, rare problems that occur
when programs are started with one or more of those fds closed.

The usual case is that you're writing a new daemon.  The generally
accepted behaviour for a daemon is to chdir("/') and then close all
unnecessary open fds, in order to minimize the chance that it will be
holding open any directories or files that would prevent unmounting a
filesystem.  On the other hand, if the daemon then needs to run git
for some reason (who knows! maybe it's a git auto-commit daemon as was
discussed earlier on the list), it needs to open file descriptors
instead.  Such a program might work 99% of the time when git doesn't
happen to print any output.  But if there's ever an error, git would
print to fd#2 on die(), and that could corrupt some random file that
the daemon *or* git was using.  Remember, the situations where the
daemon leaves fd#2 open pointing at *the wrong thing* aren't the real
problem - you could easily say that's the daemon leaving the
environment in an insane state.  The problem situation is when git
opened some random file, and it *happened* to get assigned fd#2, and
then git incorrectly assumed that writing to fd#2 would not corrupt a
file that it opened.

Does this sound rare?  It is!  But it's also hellish to debug when it
happens, precisely because of its rarity.  For example, in one case, I
had this problem because an sfdisk process started by my custom
/sbin/init ran into a minor warning, and printed it to fd#2.
Unfortunately, because /sbin/init had opened sfdisk with fds 0/1/2
closed, fd#2 ended up being the very disk it was partitioning.  The
boot sector ended up getting overwritten with a warning message in
something like 1 out of 100 cases, and the computer wouldn't boot.
ARGH.  Easy to debug, once you think to read the boot sector as
plaintext.  But that's not the first thing you think to do.

Anyway, I personally think that given how incredibly cheap this
operation is to do, and how startlingly painful it is to debug when it
*is* a problem, that it would be nice if every program just did this
by default.  I would personally feel fine if such a thing ended up in
libc or the kernel, although presumably that would violate POSIX.

Yes, it would also be fine to have every *daemon* make sure it opens
/dev/null instead of just closing fd 0/1/2.  But it's harmless to have
both.

(As for the non-builtin git commands, isn't this an advantage of
having everything get run through the main /usr/bin/git wrapper?)

Have fun,

Avery

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Avery Pennarun wrote:
>On 8/26/08, Stephen R. van den Berg <srb@cuci.nl> wrote:
>> Well, in general the policy I've used in all the tools I created is that:

>>  a. If it's a setuid tool, then you need to make sure that you don't step
>>    on anything unintendedly.  I.e. for setuid-something programs this is
>>    desirable and necessary in order to prevent securityleaks.

>>  b. Anything else is started in an environment controlled by the user,
>>    and if this environment is broken, then that is the user's fault.

>In general I'd mostly agree with you, but fd 0/1/2 are super-special
>and I've personally been bitten by insane, rare problems that occur
>when programs are started with one or more of those fds closed.

Key words: "insane, rare problems"

>The usual case is that you're writing a new daemon.  The generally
>accepted behaviour for a daemon is to chdir("/') and then close all

>the daemon *or* git was using.  Remember, the situations where the
>daemon leaves fd#2 open pointing at *the wrong thing* aren't the real
>problem - you could easily say that's the daemon leaving the
>environment in an insane state.  The problem situation is when git

Well, as you say, "you're writing a new daemon".  This means that you
need to make sure that *if* this daemon ever forks/execs it leaves the
environment in a sane state which does not open up security holes.
That means that you need to sanitise the environment, that you need to
keep tabs on all the descriptors that need to be closed on exec, and
that it needs to make sure that fd 0, 1 and 2 are pointing to somewhere
appriopriate (/dev/null, if nothing else).
The fact that you forgot to do some of those things means that it is
very helpful if you discover this fact as soon as possible.  By making
git "fix it for you" you will not notice any problems when running git
from your daemon.  Nonetheless this will not cause you to fix your
code.

>Does this sound rare?  It is!  But it's also hellish to debug when it
>happens, precisely because of its rarity.  For example, in one case, I
>had this problem because an sfdisk process started by my custom

Thing is, by making git (and some other programs) hide this problem
from you, this problem will get even *harder* to debug.  Whereas as a
daemon author you should be thankful that something breaks and shows you
your daemon needs fixing.

>Anyway, I personally think that given how incredibly cheap this
>operation is to do, and how startlingly painful it is to debug when it
>*is* a problem, that it would be nice if every program just did this
>by default.  I would personally feel fine if such a thing ended up in
>libc or the kernel, although presumably that would violate POSIX.

And then you'd have the situation that in some cases, where this
mechanism is bypassed or not present, various daemons might show security
holes in their filedescriptor management which nobody noticed before.

>Yes, it would also be fine to have every *daemon* make sure it opens
>/dev/null instead of just closing fd 0/1/2. 

It would not only be fine, it *is* required, since 1972.

> But it's harmless to have
>both.

Considering the fact that daemon authors might not get pointed at their
mistakes as soon as possible, it is harmful to try and hide those facts.

>(As for the non-builtin git commands, isn't this an advantage of
>having everything get run through the main /usr/bin/git wrapper?)

At best a program (e.g. git) could give off a warning when it finds
the filedescriptors in a bad state (and then fix it), but that would
mean that from within git we'd be trying to save the world, since anyone
not running git would not get those warnings.  You have to draw the line
somewhere, and for user-tools it ends here; for setuid-tools it's
different: they need to detect and fix it anyway, and therefore could easily
warn as well.
-- 
Sincerely,
           Stephen R. van den Berg.
"First, God created idiots.  That was just for practice.
 Then he created school boards."  --  Mark Twain

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

>> But it's harmless to have both.
> 
> Considering the fact that daemon authors might not get pointed at their
> mistakes as soon as possible, it is harmful to try and hide those facts.

Agree.  OTOH what about opening fd's 0/1/2 to /dev/null only in
git-shell.c, now that it's not a builtin anymore?

Maybe it does not fix Karl's use case, but it seems sensible to me.

Paolo

[PATCH v4] make git-shell paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

It is in general unsafe to start a program with one or more of file
descriptors 0/1/2 closed.  Karl Chen for example noticed that stat_command
does this in order to rename a pipe file descriptor to 0:

    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.  Another extremely rare but nasty problem
would occur if an "important" file ends up in file descriptor 2, and is
corrupted by a call to die().

Fixing this in git was considered to be overkill, so this patch works
around it only for git-shell.  The fix is simply to open all the "low"
descriptors to /dev/null in main.

Signed-off-by: Paolo Bonzini <bonzini@gnu.org>
---
 shell.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/shell.c b/shell.c
index 0f6a727..e339369 100644
--- a/shell.c
+++ b/shell.c
@@ -48,6 +48,19 @@ int main(int argc, char **argv)
 {
 	char *prog;
 	struct commands *cmd;
+	int devnull_fd;
+
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	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));
+	close (devnull_fd);
 
 	/*
 	 * Special hack to pretend to be a CVS server
-- 
1.5.5

Re: [PATCH v4] make git-shell paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Paolo Bonzini wrote:
>Fixing this in git was considered to be overkill, so this patch works
>around it only for git-shell.  The fix is simply to open all the "low"
>descriptors to /dev/null in main.

Since git-shell is not setuid, this strictly is not necessary, however,
I concur that git-shell is potentially started in a partially broken
environment which is not always easily fixable by the user.
And since git-shell needs to sanitise the filedescriptors anyway before
launching other programs, it might as well cleanup at startup if needed.

Acked-by: Stephen R. van den Berg <srb@cuci.nl>
-- 
Sincerely,
           Stephen R. van den Berg.
"First, God created idiots.  That was just for practice.
 Then he created school boards."  --  Mark Twain

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

>>> But it's harmless to have both.
>> 
>> Considering the fact that daemon authors might not get pointed at their
>> mistakes as soon as possible, it is harmful to try and hide those facts.
>
> Agree.  OTOH what about opening fd's 0/1/2 to /dev/null only in
> git-shell.c, now that it's not a builtin anymore?

Hmm, why git-shell?

It is either run by ssh (via command="" option in authorized_keys file),
by init/login (if in /etc/passwd), or by gitosis (and its equivalent).

Wouldn't these callers already give it a sane environment (and if a
lookalike to gitosis forgets to do so, wouldn't Stephen's argument not to
hide the issue from the daemon writers apply)?

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

> It is either run by ssh (via command="" option in authorized_keys file),
> by init/login (if in /etc/passwd), or by gitosis (and its equivalent).

It is possible to run it with file descriptors closed via ssh, using
command="git-shell 0<&- 1<&- 2<&-" in the authorized_keys file.

It's true that in this case the user is also shooting himself, but given
that git-shell is used to restrict operation to "safe" commands, this
special case might be worth being worked around.

Paolo

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Paolo Bonzini wrote:
>> It is either run by ssh (via command="" option in authorized_keys file),
>> by init/login (if in /etc/passwd), or by gitosis (and its equivalent).

>It is possible to run it with file descriptors closed via ssh, using
>command="git-shell 0<&- 1<&- 2<&-" in the authorized_keys file.

I don't consider this that relevant, however...

>It's true that in this case the user is also shooting himself, but given
>that git-shell is used to restrict operation to "safe" commands, this
>special case might be worth being worked around.

Since a programmer error in this case doesn't inflict just pain on the
user, but also is a potential security leak that can potentially be 
exploited by third party users, things are different, and it is worth
catering for.
-- 
Sincerely,
           Stephen R. van den Berg.

"Listen carefully, I shall say this only wence."

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Avery Pennarun]

On Wed, Aug 27, 2008 at 5:18 AM, Stephen R. van den Berg <srb@cuci.nl> wrote:
> Avery Pennarun wrote:
>>In general I'd mostly agree with you, but fd 0/1/2 are super-special
>>and I've personally been bitten by insane, rare problems that occur
>>when programs are started with one or more of those fds closed.
>
> Key words: "insane, rare problems"

Yes, I used those words on purpose.

> Well, as you say, "you're writing a new daemon".  This means that you
> need to make sure that *if* this daemon ever forks/execs it leaves the
> environment in a sane state which does not open up security holes.

Well, *I* know that.  But this is far from well-documented.

>>Does this sound rare?  It is!  But it's also hellish to debug when it
>>happens, precisely because of its rarity.  For example, in one case, I
>>had this problem because an sfdisk process started by my custom
>
> Thing is, by making git (and some other programs) hide this problem
> from you, this problem will get even *harder* to debug.  Whereas as a
> daemon author you should be thankful that something breaks and shows you
> your daemon needs fixing.

True enough, unless it was worked around in libc or the kernel as I
suggested.  That said, if git opens a file and writes random log
messages to it, I'd still consider that to be git's fault for doing
so.

I'm just feeling protective of the future sanity of other developers
here, hoping they don't have to go through what I did on a multi-week
bug hunt.  (We were even blaming reiserfs for a while for our boot
sector getting zapped...)  The fact that someone *other* than me has
suggested this change implies that I'm not the only one who has seen
such insanity in the wild.

It'd be fine if git simply died if fd 0, 1, or 2 isn't open when it
starts.  Printing a warning message wouldn't work, for hopefully
obvious reasons.  But it would be a shame to simply ignore this sort
of problem now that it's been brought up.

Have fun,

Avery

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Nick Andrew]

On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:
> I'm just feeling protective of the future sanity of other developers
> here, hoping they don't have to go through what I did on a multi-week
> bug hunt.  (We were even blaming reiserfs for a while for our boot
> sector getting zapped...)  The fact that someone *other* than me has
> suggested this change implies that I'm not the only one who has seen
> such insanity in the wild.

You're not alone. I've been having trouble with a combination of
fetchmail, procmail and ssmtp, in which situation the ssmtp program
_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while
calculating an SSL key?) and leaves it open, then reads the message
body from that file descriptor, resulting in an endless garbage message
being sent to the SMTP server.

I suspect the error originates in Debian's patch to ssmtp (which
added the SSL support) but I haven't been able to reproduce the bug
in controlled circumstances. It's possible that fetchmail or procmail
is doing something stupid - but a little more defensive programming
in ssmtp could avoid the total disaster area of sending an endless
binary stream to an SMTP server.

So although I'm not experiencing any problems with git due to incorrect
file descriptor usage, I'm sensitive to the general issue.

Nick.

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Nick Andrew wrote:
>On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:
>> I'm just feeling protective of the future sanity of other developers
>> here, hoping they don't have to go through what I did on a multi-week

>You're not alone. I've been having trouble with a combination of
>fetchmail, procmail and ssmtp, in which situation the ssmtp program
>_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while

>in controlled circumstances. It's possible that fetchmail or procmail
>is doing something stupid - but a little more defensive programming
>in ssmtp could avoid the total disaster area of sending an endless
>binary stream to an SMTP server.

Procmail I can vouch for, it basically assumes your OS is broken and
fights it's way back to sanity (it can be setuid root, so it has to
be rather careful).
Nonetheless, I still maintain that hiding problems doesn't help, it
only makes the bugs even rarer and more difficult to find.

The filedescriptor problem is a programmer-error, not a user-error,
which is why not hiding it should be preferred.  If it were a
user-error, thing would be different, assisting the user is a Good
Thing.
-- 
Sincerely,
           Stephen R. van den Berg.

"Listen carefully, I shall say this only wence."

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Paolo Bonzini schrieb:
>> +	/*
>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>> +	 * in die().  It also avoids not messing up when the pipes are
>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>> +	 */
>
> I see your point, but I don't have an opinion whether this stretch is
> necessary.

This is going too far.  Have you seen any other sane program that do this?

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Paolo Bonzini schrieb:
>>> +	/*
>>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>>> +	 * in die().  It also avoids not messing up when the pipes are
>>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>>> +	 */
>> I see your point, but I don't have an opinion whether this stretch is
>> necessary.
> 
> This is going too far.  Have you seen any other sane program that do this?

Busybox.  But it runs setuid, as Steven pointed out.

I say it's all (i.e. be this paranoid), or nothing.

Paolo

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> Junio C Hamano wrote:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>>> Paolo Bonzini schrieb:
>>>> +	/*
>>>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>>>> +	 * in die().  It also avoids not messing up when the pipes are
>>>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>>>> +	 */
>>> I see your point, but I don't have an opinion whether this stretch is
>>> necessary.
>> 
>> This is going too far.  Have you seen any other sane program that do this?
>
> Busybox.  But it runs setuid, as Steven pointed out.
>
> I say it's all (i.e. be this paranoid), or nothing.

I tend to agree, and I think what Stephen R. van den Berg said earlier in
the thread makes perfect sense.

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Paolo Bonzini <bonzini@gnu.org> writes:
>
>> Junio C Hamano wrote:
>>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>> 
>>>> Paolo Bonzini schrieb:
>>>>> +	/*
>>>>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>>>>> +	 * in die().  It also avoids not messing up when the pipes are
>>>>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>>>>> +	 */
>>>> I see your point, but I don't have an opinion whether this stretch is
>>>> necessary.
>>> 
>>> This is going too far.  Have you seen any other sane program that do this?
>>
>> Busybox.  But it runs setuid, as Steven pointed out.
>>
>> I say it's all (i.e. be this paranoid), or nothing.
>
> I tend to agree, and I think what Stephen R. van den Berg said earlier in
> the thread makes perfect sense.

So going back to the very original in the thread.

I think

	$ git fetch 0<&-

from the command line is a mere user stupidity.

On the other hand, if a cron/at job that contains "git fetch" is launched
in an environment with fd#0 (or #1 or #2 for that matter) closed, it would
certainly be problematic.  It can easily be worked around by redirecting
file descriptors appropriately in the script that is launched, though.

On a related note, we should make sure that we run our hooks with the set
of low file descriptors opened sensibly.  It would be a bug if we are
running them in a weird environment and forcing them to do funky
redirection themselves.  I think we are already Ok in this regard, but I
didn't check.

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Junio C Hamano wrote:
>Junio C Hamano <gitster@pobox.com> writes:
>I think
>	$ git fetch 0<&-
>from the command line is a mere user stupidity.

>On the other hand, if a cron/at job that contains "git fetch" is launched
>in an environment with fd#0 (or #1 or #2 for that matter) closed, it would
>certainly be problematic.  It can easily be worked around by redirecting
>file descriptors appropriately in the script that is launched, though.

A sane cron environment always has proper 0, 1 and 2 descriptors.
This basically goes with rule #2: if your cron doesn't have 0, 1 and 2
open, you have big problems already, so camouflaging those problems
is not going to help the user.

>On a related note, we should make sure that we run our hooks with the set
>of low file descriptors opened sensibly.  It would be a bug if we are
>running them in a weird environment and forcing them to do funky
>redirection themselves.  I think we are already Ok in this regard, but I
>didn't check.

Agreed, but this is the responsibility of anyone launching other
processes (cleanup, then launch).
-- 
Sincerely,
           Stephen R. van den Berg.

"Good moaning!"

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Karl Chen]

>>>>> On 2008-08-26 16:04 PDT, Junio C Hamano writes:

    Junio> I think

    Junio> 	$ git fetch 0<&-

    Junio> from the command line is a mere user stupidity.

    Junio> On the other hand, if a cron/at job that contains "git
    Junio> fetch" is launched in an environment with fd#0 (or #1
    Junio> or #2 for that matter) closed, it would certainly be
    Junio> problematic.  It can easily be worked around by
    Junio> redirecting file descriptors appropriately in the
    Junio> script that is launched, though.

I agree command-line 'git fetch 0<&-' is silly.  The example I
gave was minimized to show the symptom.  I ran across this with a
more complicated cron-ish setup that closes stdin.  I actually had
to look up the shell syntax for closing file descriptors.

Yes, I can work around this issue with sh -c 'git fetch
0</dev/null', and maybe it shouldn't close(0) in the first place.
But I don't see the harm in being safe.  It's one less potential
surprise for users.  This is the first program I've encountered
that broke due to stdin being closed, and it took debugging to
figure out that was the reason.

Re security, it's actually a good idea to be safe early on if it
could ever become an issue.  I keep /etc on my systems in version
control, and I've worked in production environments where some
users have access only via version control commands.

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

> Yes, I can work around this issue with sh -c 'git fetch
> 0</dev/null', and maybe it shouldn't close(0) in the first place.
> But I don't see the harm in being safe.  It's one less potential
> surprise for users.  This is the first program I've encountered
> that broke due to stdin being closed

Not really.  I suspect every program that uses pipe/dup to fork a child
could be wrong (the only one I ever wrote breaks), and I wonder if the
higher-level popen(3) interface works properly.

Paolo

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Stephen R. van den Berg]

Karl Chen wrote:
>Yes, I can work around this issue with sh -c 'git fetch
>0</dev/null', and maybe it shouldn't close(0) in the first place.
>But I don't see the harm in being safe.  It's one less potential
>surprise for users.  This is the first program I've encountered
>that broke due to stdin being closed, and it took debugging to
>figure out that was the reason.

I understand the reasoning, and there sure is a valid point in here
(principle of least surprise), but there is also the case of hiding
problems.
It's a bit unclear which should prevail here.
The point is that if you actually make git detect and correct
closed descriptors which should have been open, then you are merely
passing the buck to all other programs the user is starting which might
or might not break.

Maybe the breakage of other programs is only in conjunction with
full-moon and FD 0 closed, in that case you make the problems/bugs even
*harder* to find for the user by making git "fix it for you".
-- 
Sincerely,
           Stephen R. van den Berg.
"First, God created idiots.  That was just for practice.
 Then he created school boards."  --  Mark Twain

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Johannes Sixt]

Paolo Bonzini schrieb:
> Junio C Hamano wrote:
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>> Paolo Bonzini schrieb:
>>>> +	/*
>>>> +	 * Always open file descriptors 0/1/2 to avoid clobbering files
>>>> +	 * in die().  It also avoids not messing up when the pipes are
>>>> +	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
>>>> +	 */
>>> I see your point, but I don't have an opinion whether this stretch is
>>> necessary.
>> This is going too far.  Have you seen any other sane program that do this?
> 
> Busybox.  But it runs setuid, as Steven pointed out.

I straced tee (it was the only tool I found that opens files for writing
without also opening some for reading). If one of 0,1,2 is closed, it
*does* dup() the fd that it is going to write.

Don't you now feel like Reg in "Life of Brian":

"All right, but apart from the sanitation, the medicine, education, wine,
public order, irrigation, roads, a fresh water system, and public health,
what have the Romans ever done for us?"

;)

-- Hannes

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Paolo Bonzini]

>> Busybox.  But it runs setuid, as Steven pointed out.
> 
> I straced tee (it was the only tool I found that opens files for writing
> without also opening some for reading). If one of 0,1,2 is closed, it
> *does* dup() the fd that it is going to write.

To be precise, it does a blind "dup2 (fd, 3)" and goes on with file
descriptor 3.

open("foo", O_WRONLY|O_CREAT|O_TRUNC|O_LARGEFILE, 0666) = 0
fcntl64(0, F_DUPFD, 3)                  = 3
close(0)                                = 0

Paolo

Re: [PATCH] be paranoid about closed stdin/stdout/stderr

[Nick Andrew]

On Tue, Aug 26, 2008 at 10:38:35AM -0700, Junio C Hamano wrote:
> This is going too far.  Have you seen any other sane program that do this?

Hmm. I posted a patch to the "sane" project (scanner daemon) to avoid a
similar problem. The patch was rejected.

saned tries to sanitise its environment, specifically low order fds:

fd = open("/dev/null", O_RDWR);
dup2(fd, 0);
dup2(fd, 1);
fup2(fd, 2);
close(fd);

And I pointed out that if the fds aren't sanitary (all fds open) before
the code snippet, they won't be sanitary after it. My patch was only:

if (fd > 2)
    close(fd);

If closing fd 0/1/2 and then forking a subprocess is the unix equivalent
of delayed shooting yourself in the foot then I can agree; git doesn't
need it.

Nick.

Re: [PATCH] Fix start_command() pipe bug when stdin is closed.

[Karl Chen]

>>>>> On 2008-08-25 03:44 PDT, Johannes Sixt writes:

    Johannes> When I try these instructions I don't get an error;
    Johannes> instead the command runs successfully.

Weird.  I see the symptom on two machines, both 1.5.6 and tracking
master.  The 1.5.6 system installation could be interfering even
though I used PATH=/path/to/git:$PATH.

    Johannes> While I do see that there is a problem, it is only
    Johannes> half of the story, and your patch addresses only
    Johannes> this half.

    Johannes> What if stdout is closed, too? Then the ends of the
    Johannes> first allocated pipe would go to fds 0 and 1, and
    Johannes> then the pipe end at 1 would be closed by a
    Johannes> subsequent dup2(xxx, 1).

My patch was intended to fix the problem for any renaming where
fd_from==fd_to, including target stdout.  I didn't say so in the
changelog though.