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

From: Avery Pennarun <apenwarr@gmail.com>
Date: Aug 27, 2008, 05:01 UTC
Message-ID: <32541b130808262201v4d7c1aa5r781720a80b79fcd0@mail.gmail.com>
In-Reply-To: <20080826074044.GA22694@cuci.nl>

On 8/26/08, Stephen R. van den Berg <srb@cuci.nl> wrote:

Show 8 quoted lines

> 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