Re: [PATCH 0/2] open() error checking

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2013, 17:29 UTC
Message-ID: <7vtxjzlmaf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <cover.1373618940.git.trast@inf.ethz.ch>

Thomas Rast <trast@inf.ethz.ch> writes:

Show 12 quoted lines

> #1 is Dale's suggested change.  Dale, to include it we'd need your
> Signed-off-by as per Documentation/SubmittingPatches.
>
> #2 is a similar error-checking fix; I reviewed 'git grep "\bopen\b"'
> and found one case where the return value was obviously not tested.
> The corresponding Windows code path has the same problem, but I dare
> not touch it; perhaps someone from the Windows side can look into it?
>
> I originally had a four-patch series to open 0/1/2 from /dev/null, but
> then I noticed that this was shot down in 2008:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/93605/focus=93896

The way I recall the thread was not "shot down" but more like "fizzled out without seeing a clear consensus". As a normal POSIX program, we do rely on fd#2 connected to an error stream, and I do agree with the general sentiment of that old thread that it is very wrong for warning() or die() to write to a pipe or file descriptor we opened for some other purpose, corrupting the destination.

I briefly wondered if we can do the sanity check lazily (e.g. upon first warning() see of fd#2 is open and otherwise die silently), but we may open a fd (e.g. to create a new loose object) that may happen to grab fd#2 and then it is too late for us to do anything about it, so...

> Do you want to resurrect this?
>
> The worst part about it is that because we don't have a stderr to rely
> on, we can't simply die("stop playing mind games").

Right.