Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash

[Junio C Hamano <gitster@pobox.com>]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Vincent van Ravesteijn <vfr@lyx.org> writes:
>
>> When compiled with MSVC, git crashes on Windows when calling
>> fstat(stdout) when stdout is closed. fstat is being called at the end of
>
> ITYM fileno(stdout).
> 
>> run_builtin and this will thus be a problem for builtin command that close
>> stdout. This happens for 'format-patch' which closes stdout after a call to
>> freopen which directs stdout to the format patch file.
>
> It shouldn't do that in the first place.  This is an error on any
> platform.

Correct. The clean-up codepath is for built-in command implementations
that write out their result and return 0 to signal success. If we let the
crt0 to run its usual clean-ups like closing the standard output stream,
we wouldn't be able to catch errors from there.

For built-ins that perform their own clean-ups, it is their responsibility
to be careful, hence we skip this part of the code.

We have relied on fstat(-1, &st) to correctly error out, and if MSVC build
crashes, it is a bug in its fstat() emulation, I would think.

We could do something like the following patch to be extra defensive,
though.

 git.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index 8e34903..64c28e4 100644
--- a/git.c
+++ b/git.c
@@ -309,8 +309,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	if (status)
 		return status;
 
-	/* Somebody closed stdout? */
-	if (fstat(fileno(stdout), &st))
+	if (fileno(stdout) < 0 || /* Somebody closed stdout? */
+	    fstat(fileno(stdout), &st))
 		return 0;
 	/* Ignore write errors for pipes and sockets.. */
 	if (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))