patchFix start_command() pipe bug when stdin is closed.

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: 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(-)

Karl Chen schrieb:

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

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

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

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(-)

Paolo Bonzini schrieb:

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

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

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

    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!

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

Thanks.

Paolo Bonzini <bonzini@gnu.org> writes:

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?

Junio C Hamano schrieb:

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

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

-- Hannes

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

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(-)

Paolo Bonzini schrieb:

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

Johannes Sixt 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.
   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.

On 8/26/08, Stephen R. van den Berg <srb@cuci.nl> 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.

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

Avery Pennarun wrote:

Key words: "insane, rare problems"

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.

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.

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.

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

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.

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.

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

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(-)

Paolo Bonzini wrote:

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>

Paolo Bonzini <bonzini@gnu.org> writes:

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)?

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

Paolo Bonzini wrote:

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

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.

On Wed, Aug 27, 2008 at 5:18 AM, Stephen R. van den Berg <srb@cuci.nl> wrote:

Yes, I used those words on purpose.

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

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

On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:

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.

Nick Andrew wrote:

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.

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

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

Junio C Hamano wrote:

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

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

Paolo

Paolo Bonzini <bonzini@gnu.org> writes:

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

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

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.

Junio C Hamano wrote:

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.

Agreed, but this is the responsibility of anyone launching other processes (cleanup, then launch).

    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.

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

Karl Chen wrote:

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".

Paolo Bonzini schrieb:

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

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

On Tue, Aug 26, 2008 at 10:38:35AM -0700, Junio C Hamano wrote:

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.

    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.