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

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

[Vincent van Ravesteijn]

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(-)

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

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

[Vincent van Ravesteijn]

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(-)

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

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

[Andreas Schwab]

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."

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

[Junio C Hamano]

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))

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

[Johannes Sixt]

Am 19.11.2011 14:45, schrieb Vincent van Ravesteijn:
> 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

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

[Andreas Schwab]

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."

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

[Vincent van Ravesteijn]

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

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

[Junio C Hamano]

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.

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> 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."

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

[Johannes Sixt]

Am 20.11.2011 04:27, schrieb Junio C Hamano:
> 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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> 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.

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

[Vincent van Ravesteijn]

>> 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

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

[Johannes Sixt]

Am 22.11.2011 07:45, schrieb Vincent van Ravesteijn:
>>> 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

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

[Vincent van Ravesteijn]

>> 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