threads / patch / 28980

patch, 2 partsMSVC: Do not close stdout to prevent a crash

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

## tl;dr

14 messages between Nov 19, 2011 and Nov 22, 2011. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Vincent van Ravesteijn· Nov 19, 2011, 13:45 UTC · lore

When compiled with MSVC, git crashes on Windows when calling fstat(stdout) when stdout is closed. fstat is being called at the end of 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.

To prevent the crash and to prevent git from writing cruft into the patch file, we do not close stdout, but redirect it to "nul" instead.

Signed-off-by: Vincent van Ravesteijn <vfr@lyx.org>
---
 compat/mingw.c |    8 ++++++++
 compat/mingw.h |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)
Show changes to 2 files +11 −0

compat/mingw.c, compat/mingw.h

diff --git a/compat/mingw.c b/compat/mingw.c
index efdc703..8943df5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -319,6 +319,14 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
 	return write(fd, buf, min(count, 31 * 1024 * 1024));
 }
 
+#undef fclose
+int mingw_fclose (FILE *stream)
+{
+	if (fileno(stream) == 1 && freopen("nul", "w", stream))
+		return 0;
+	return fclose(stream);
+}
+
 #undef fopen
 FILE *mingw_fopen (const char *filename, const char *otype)
 {
diff --git a/compat/mingw.h b/compat/mingw.h
index ff18401..80a6015 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);
 ssize_t mingw_write(int fd, const void *buf, size_t count);
 #define write mingw_write
 
+int mingw_fclose(FILE *stream);
+#define fclose mingw_fclose
+
 FILE *mingw_fopen (const char *filename, const char *otype);
 #define fopen mingw_fopen
 
-- 
1.7.4.1
Vincent van Ravesteijn· Nov 19, 2011, 13:52 UTC · re: Vincent van Ravesteijn · lore

[PATCH 2/2] MSVC: Use _putenv instead of putenv to prevent a crash

Git crashes when trying to clear a nonexistent environment variable using the putenv function. The crash occurs when freeing the option string. In debug mode the assert "CrtIsValidHeapPointer" fails.

Using _putenv instead of putenv makes the crash and assert disappear.
Signed-off-by: Vincent van Ravesteijn <vfr@lyx.org>
---
The strange thing is that all over the internet people are claiming
that there is no difference between putenv and _putenv. Still, using
_putenv fixes the crash for me. 

If there is someone around who is more knowledgeable in this area, please comment.

 compat/setenv.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to compat/setenv.c +1 −1
diff --git a/compat/setenv.c b/compat/setenv.c
index 3a22ea7..b23937d 100644
--- a/compat/setenv.c
+++ b/compat/setenv.c
@@ -23,7 +23,7 @@ int gitsetenv(const char *name, const char *value, int replace)
 	memcpy(envstr + namelen + 1, value, valuelen);
 	envstr[namelen + valuelen + 1] = 0;
 
-	out = putenv(envstr);
+	out = _putenv(envstr);
 	/* putenv(3) makes the argument string part of the environment,
 	 * and changing that string modifies the environment --- which
 	 * means we do not own that storage anymore.  Do not free
-- 
1.7.4.1
Andreas Schwab· Nov 19, 2011, 14:41 UTC · re: Vincent van Ravesteijn · lore

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

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.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· Nov 19, 2011, 19:11 UTC · re: Andreas Schwab · lore

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

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(-)
Show changes to git.c +2 −2
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))
Andreas Schwab· Nov 19, 2011, 20:16 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
> 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.
fileno(stdout) is alread wrong if stdout was closed.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Junio C Hamano· Nov 20, 2011, 03:27 UTC · re: Andreas Schwab · lore

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

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 6 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> fileno(stdout) is alread wrong if stdout was closed.
The "-1" in my message comes from here:
    DESCRIPTION
    The fileno() function shall return the integer file descriptor
    associated with the stream pointed to by stream.
    RETURN VALUE
    Upon successful completion, fileno() shall return the integer value of
    the file descriptor associated with stream. Otherwise, the value -1
    shall be returned and errno set to indicate the error.
Andreas Schwab· Nov 20, 2011, 09:05 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> 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.
>>
>> fileno(stdout) is alread wrong if stdout was closed.
>
> The "-1" in my message comes from here:
>
>     DESCRIPTION
>
>     The fileno() function shall return the integer file descriptor
>     associated with the stream pointed to by stream.

After fclose(stdout) the stream does not exist any more. Thus the use of fileno(stdout) is undefined, in fact *any* use of stdout is undefined afterwards.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Johannes Sixt· Nov 20, 2011, 09:27 UTC · re: Junio C Hamano · lore

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

Am 20.11.2011 04:27, schrieb Junio C Hamano:
Show 21 quoted lines
> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> 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.
>>
>> fileno(stdout) is alread wrong if stdout was closed.
> 
> The "-1" in my message comes from here:
> 
>     DESCRIPTION
> 
>     The fileno() function shall return the integer file descriptor
>     associated with the stream pointed to by stream.
> 
>     RETURN VALUE
> 
>     Upon successful completion, fileno() shall return the integer value of
>     the file descriptor associated with stream. Otherwise, the value -1
>     shall be returned and errno set to indicate the error.
But in the description of fclose() there is also:
  After the call to fclose(), any use of stream results in undefined
  behavior.
And we do call fclose(stdout) in cmd_format_patch.
-- Hannes
Junio C Hamano· Nov 20, 2011, 21:10 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 6 quoted lines
> But in the description of fclose() there is also:
>
>   After the call to fclose(), any use of stream results in undefined
>   behavior.
>
> And we do call fclose(stdout) in cmd_format_patch.

Yeah; this means the "in case builtin implementation is too lazy to clean up after themselves, run_builtin() will clean things up for them" approach taken by 0f15731 (Check for IO errors after running a command, 2007-06-24) fundamentally does not work, which is a bit sad.

Vincent van Ravesteijn· Nov 19, 2011, 20:52 UTC · re: Junio C Hamano · lore

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

Op 19-11-2011 20:11, Junio C Hamano schreef:
Show 14 quoted lines
> ... 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.

This doesn't work because fileno(stdout) returns 1 no matter whether we closed stdout or not.

My reasoning for the proposed patch is that we call freopen(..., stdout) for each patch file to write, but we only call fclose(stdout) once. Sp, I concluded that it is not necessary to close stdout and that we just need to redirect any new output to stdout.

Vincent
Johannes Sixt· Nov 19, 2011, 20:11 UTC · re: Vincent van Ravesteijn · lore

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

Am 19.11.2011 14:45, schrieb Vincent van Ravesteijn:
Show 5 quoted lines
> When compiled with MSVC, git crashes on Windows when calling
> fstat(stdout) when stdout is closed. fstat is being called at the end of
> 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.

This crash happens because of the some drainbramage in the MS's newer C runtime: Its functions never return EINVAL (like fstat should in this case), but it is assumed that cases where EINVAL should be returned are programming errors, and the application is redirected, via an "invalid agument handler" to abort() instead. (It is advertized as a security feature.)

> To prevent the crash and to prevent git from writing cruft into the patch
> file, we do not close stdout, but redirect it to "nul" instead.

A more robust solution is to add invalidcontinue.obj to the linker command line. This installs an invalid argument handler that does not abort, and restores a sane behavior.

-- Hannes
Vincent van Ravesteijn· Nov 22, 2011, 06:45 UTC · re: Johannes Sixt · lore

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

Show 5 quoted lines
>> To prevent the crash and to prevent git from writing cruft into the patch
>> file, we do not close stdout, but redirect it to "nul" instead.
> A more robust solution is to add invalidcontinue.obj to the linker
> command line. This installs an invalid argument handler that does not
> abort, and restores a sane behavior.
This seems to work only for release builds or did I miss something ?

Is there any argument to not redirect stdout to "/dev/null" on all platforms ?

Vincent
Johannes Sixt· Nov 22, 2011, 20:24 UTC · re: Vincent van Ravesteijn · lore

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

Am 22.11.2011 07:45, schrieb Vincent van Ravesteijn:
Show 8 quoted lines
>>> To prevent the crash and to prevent git from writing cruft into the
>>> patch
>>> file, we do not close stdout, but redirect it to "nul" instead.
>> A more robust solution is to add invalidcontinue.obj to the linker
>> command line. This installs an invalid argument handler that does not
>> abort, and restores a sane behavior.
> 
> This seems to work only for release builds or did I miss something ?

I cannot tell, but I would be surprised if that were the case. I didn't test with debug builds (only our 'make MSVC=1' build). A cursory look at the CRT source code does not show anything that would be different between a debug and a release build regarding invalidcontinue.obj.

> Is there any argument to not redirect stdout to "/dev/null" on all
> platforms ?
You paper over a crack in the wall. You hide a bug.
-- Hannes
Vincent van Ravesteijn· Nov 22, 2011, 21:27 UTC · re: Johannes Sixt · lore

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

>> Is there any argument to not redirect stdout to "/dev/null" on all
>> platforms ?
> You paper over a crack in the wall. You hide a bug.

Luckily I'm still trying to find the crack and not ready to tape anything yet.

Vincent

← back to recent threads