Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 19, 2011, 19:11 UTC
- Message-ID
- <7v39dkj5ad.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <m2sjlkcgyl.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
Show 13 quoted lines
> 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))