{"thread":{"id":"28980","subject":"[PATCH 1/2] MSVC: Do not close stdout to prevent a crash","startedAt":"2011-11-19T13:45:44Z","lastAt":"2011-11-22T21:27:41Z","messageCount":14,"participants":["Vincent van Ravesteijn","Andreas Schwab","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"179731","messageId":"1321710345-2299-1-git-send-email-vfr@lyx.org","threadId":"28980","inReplyTo":null,"subject":"[PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Vincent van Ravesteijn","fromEmail":"vfr@lyx.org","sentAt":"2011-11-19T13:45:44Z","receivedAt":"2011-11-19T13:45:44Z","isPatch":true,"sender":{"key":"vfr@lyx.org","avatar":"https://avatars.githubusercontent.com/u/687868?v=4"},"body":"When compiled with MSVC, git crashes on Windows when calling\nfstat(stdout) when stdout is closed. fstat is being called at the end of\nrun_builtin and this will thus be a problem for builtin command that close\nstdout. This happens for 'format-patch' which closes stdout after a call to\nfreopen which directs stdout to the format patch file.\n\nTo prevent the crash and to prevent git from writing cruft into the patch\nfile, we do not close stdout, but redirect it to \"nul\" instead.\n\nSigned-off-by: Vincent van Ravesteijn \n---\n compat/mingw.c | 8 ++++++++\n compat/mingw.h | 3 +++\n 2 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex efdc703..8943df5 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -319,6 +319,14 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)\n \treturn write(fd, buf, min(count, 31 * 1024 * 1024));\n }\n \n+#undef fclose\n+int mingw_fclose (FILE *stream)\n+{\n+\tif (fileno(stream) == 1 && freopen(\"nul\", \"w\", stream))\n+\t\treturn 0;\n+\treturn fclose(stream);\n+}\n+\n #undef fopen\n FILE *mingw_fopen (const char *filename, const char *otype)\n {\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex ff18401..80a6015 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);\n ssize_t mingw_write(int fd, const void *buf, size_t count);\n #define write mingw_write\n \n+int mingw_fclose(FILE *stream);\n+#define fclose mingw_fclose\n+\n FILE *mingw_fopen (const char *filename, const char *otype);\n #define fopen mingw_fopen\n \n-- \n1.7.4.1\n"},{"id":"179732","messageId":"1321710745-2341-1-git-send-email-vfr@lyx.org","threadId":"28980","inReplyTo":"1321710345-2299-1-git-send-email-vfr@lyx.org","subject":"[PATCH 2/2] MSVC: Use _putenv instead of putenv to prevent a crash","fromName":"Vincent van Ravesteijn","fromEmail":"vfr@lyx.org","sentAt":"2011-11-19T13:52:25Z","receivedAt":"2011-11-19T13:52:25Z","isPatch":true,"sender":{"key":"vfr@lyx.org","avatar":"https://avatars.githubusercontent.com/u/687868?v=4"},"body":"Git crashes when trying to clear a nonexistent environment variable using\nthe putenv function. The crash occurs when freeing the option string. In\ndebug mode the assert \"CrtIsValidHeapPointer\" fails.\n\nUsing _putenv instead of putenv makes the crash and assert disappear.\n\nSigned-off-by: Vincent van Ravesteijn \n---\nThe strange thing is that all over the internet people are claiming\nthat there is no difference between putenv and _putenv. Still, using\n_putenv fixes the crash for me. \n\nIf there is someone around who is more knowledgeable in this area,\nplease comment.\n\n compat/setenv.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/compat/setenv.c b/compat/setenv.c\nindex 3a22ea7..b23937d 100644\n--- a/compat/setenv.c\n+++ b/compat/setenv.c\n@@ -23,7 +23,7 @@ int gitsetenv(const char *name, const char *value, int replace)\n \tmemcpy(envstr + namelen + 1, value, valuelen);\n \tenvstr[namelen + valuelen + 1] = 0;\n \n-\tout = putenv(envstr);\n+\tout = _putenv(envstr);\n \t/* putenv(3) makes the argument string part of the environment,\n \t * and changing that string modifies the environment --- which\n \t * means we do not own that storage anymore. Do not free\n-- \n1.7.4.1\n"},{"id":"179733","messageId":"m2sjlkcgyl.fsf@igel.home","threadId":"28980","inReplyTo":"1321710345-2299-1-git-send-email-vfr@lyx.org","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-19T14:41:38Z","receivedAt":"2011-11-19T14:41:38Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Vincent van Ravesteijn writes:\n\n> When compiled with MSVC, git crashes on Windows when calling\n> fstat(stdout) when stdout is closed. fstat is being called at the end of\n\nITYM fileno(stdout).\n\n> run_builtin and this will thus be a problem for builtin command that close\n> stdout. This happens for 'format-patch' which closes stdout after a call to\n> freopen which directs stdout to the format patch file.\n\nIt shouldn't do that in the first place. This is an error on any\nplatform.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179737","messageId":"7v39dkj5ad.fsf@alter.siamese.dyndns.org","threadId":"28980","inReplyTo":"m2sjlkcgyl.fsf@igel.home","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-19T19:11:54Z","receivedAt":"2011-11-19T19:11:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> Vincent van Ravesteijn writes:\n>\n>> When compiled with MSVC, git crashes on Windows when calling\n>> fstat(stdout) when stdout is closed. fstat is being called at the end of\n>\n> ITYM fileno(stdout).\n> \n>> run_builtin and this will thus be a problem for builtin command that close\n>> stdout. This happens for 'format-patch' which closes stdout after a call to\n>> freopen which directs stdout to the format patch file.\n>\n> It shouldn't do that in the first place. This is an error on any\n> platform.\n\nCorrect. The clean-up codepath is for built-in command implementations\nthat write out their result and return 0 to signal success. If we let the\ncrt0 to run its usual clean-ups like closing the standard output stream,\nwe wouldn't be able to catch errors from there.\n\nFor built-ins that perform their own clean-ups, it is their responsibility\nto be careful, hence we skip this part of the code.\n\nWe have relied on fstat(-1, &st) to correctly error out, and if MSVC build\ncrashes, it is a bug in its fstat() emulation, I would think.\n\nWe could do something like the following patch to be extra defensive,\nthough.\n\n git.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 8e34903..64c28e4 100644\n--- a/git.c\n+++ b/git.c\n@@ -309,8 +309,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tif (status)\n \t\treturn status;\n \n-\t/* Somebody closed stdout? */\n-\tif (fstat(fileno(stdout), &st))\n+\tif (fileno(stdout) < 0 || /* Somebody closed stdout? */\n+\t fstat(fileno(stdout), &st))\n \t\treturn 0;\n \t/* Ignore write errors for pipes and sockets.. */\n \tif (S_ISFIFO(st.st_mode) || S_ISSOCK(st.st_mode))\n"},{"id":"179744","messageId":"4EC80D84.2010202@kdbg.org","threadId":"28980","inReplyTo":"1321710345-2299-1-git-send-email-vfr@lyx.org","subject":"Re: [msysGit] [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-11-19T20:11:48Z","receivedAt":"2011-11-19T20:11:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 19.11.2011 14:45, schrieb Vincent van Ravesteijn:\n> When compiled with MSVC, git crashes on Windows when calling\n> fstat(stdout) when stdout is closed. fstat is being called at the end of\n> run_builtin and this will thus be a problem for builtin command that close\n> stdout. This happens for 'format-patch' which closes stdout after a call to\n> freopen which directs stdout to the format patch file.\n\nThis crash happens because of the some drainbramage in the MS's newer C\nruntime: Its functions never return EINVAL (like fstat should in this\ncase), but it is assumed that cases where EINVAL should be returned are\nprogramming errors, and the application is redirected, via an \"invalid\nagument handler\" to abort() instead. (It is advertized as a security\nfeature.)\n\n> To prevent the crash and to prevent git from writing cruft into the patch\n> file, we do not close stdout, but redirect it to \"nul\" instead.\n\nA more robust solution is to add invalidcontinue.obj to the linker\ncommand line. This installs an invalid argument handler that does not\nabort, and restores a sane behavior.\n\n-- Hannes\n"},{"id":"179745","messageId":"m2obw7dg1e.fsf@igel.home","threadId":"28980","inReplyTo":"7v39dkj5ad.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-19T20:16:13Z","receivedAt":"2011-11-19T20:16:13Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano writes:\n\n> We have relied on fstat(-1, &st) to correctly error out, and if MSVC build\n> crashes, it is a bug in its fstat() emulation, I would think.\n\nfileno(stdout) is alread wrong if stdout was closed.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179749","messageId":"4EC8170D.3070906@lyx.org","threadId":"28980","inReplyTo":"7v39dkj5ad.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Vincent van Ravesteijn","fromEmail":"vfr@lyx.org","sentAt":"2011-11-19T20:52:29Z","receivedAt":"2011-11-19T20:52:29Z","isPatch":true,"sender":{"key":"vfr@lyx.org","avatar":"https://avatars.githubusercontent.com/u/687868?v=4"},"body":"Op 19-11-2011 20:11, Junio C Hamano schreef:\n> ... This happens for 'format-patch' which closes stdout after a call to\n> freopen which directs stdout to the format patch file.\n>> It shouldn't do that in the first place. This is an error on any\n>> platform.\n> Correct. The clean-up codepath is for built-in command implementations\n> that write out their result and return 0 to signal success. If we let the\n> crt0 to run its usual clean-ups like closing the standard output stream,\n> we wouldn't be able to catch errors from there.\n>\n> For built-ins that perform their own clean-ups, it is their responsibility\n> to be careful, hence we skip this part of the code.\n>\n> We have relied on fstat(-1,&st) to correctly error out, and if MSVC build\n> crashes, it is a bug in its fstat() emulation, I would think.\n\nThis doesn't work because fileno(stdout) returns 1 no matter whether we \nclosed stdout or not.\n\nMy reasoning for the proposed patch is that we call freopen(..., stdout) \nfor each patch file to write, but we only call fclose(stdout) once. Sp, \nI concluded that it is not necessary to close stdout and that we just \nneed to redirect any new output to stdout.\n\nVincent\n"},{"id":"179755","messageId":"7vpqgniid5.fsf@alter.siamese.dyndns.org","threadId":"28980","inReplyTo":"m2obw7dg1e.fsf@igel.home","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-20T03:27:02Z","receivedAt":"2011-11-20T03:27:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> Junio C Hamano writes:\n>\n>> We have relied on fstat(-1, &st) to correctly error out, and if MSVC build\n>> crashes, it is a bug in its fstat() emulation, I would think.\n>\n> fileno(stdout) is alread wrong if stdout was closed.\n\nThe \"-1\" in my message comes from here:\n\n DESCRIPTION\n\n The fileno() function shall return the integer file descriptor\n associated with the stream pointed to by stream.\n\n RETURN VALUE\n\n Upon successful completion, fileno() shall return the integer value of\n the file descriptor associated with stream. Otherwise, the value -1\n shall be returned and errno set to indicate the error.\n"},{"id":"179758","messageId":"m2k46vcgem.fsf@igel.home","threadId":"28980","inReplyTo":"7vpqgniid5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2011-11-20T09:05:53Z","receivedAt":"2011-11-20T09:05:53Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Junio C Hamano writes:\n\n> Andreas Schwab writes:\n>\n>> Junio C Hamano writes:\n>>\n>>> We have relied on fstat(-1, &st) to correctly error out, and if MSVC build\n>>> crashes, it is a bug in its fstat() emulation, I would think.\n>>\n>> fileno(stdout) is alread wrong if stdout was closed.\n>\n> The \"-1\" in my message comes from here:\n>\n> DESCRIPTION\n>\n> The fileno() function shall return the integer file descriptor\n> associated with the stream pointed to by stream.\n\nAfter fclose(stdout) the stream does not exist any more. Thus the use\nof fileno(stdout) is undefined, in fact *any* use of stdout is undefined\nafterwards.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"179759","messageId":"4EC8C7FE.5050903@kdbg.org","threadId":"28980","inReplyTo":"7vpqgniid5.fsf@alter.siamese.dyndns.org","subject":"Re: [msysGit] Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-11-20T09:27:26Z","receivedAt":"2011-11-20T09:27:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.11.2011 04:27, schrieb Junio C Hamano:\n> Andreas Schwab writes:\n> \n>> Junio C Hamano writes:\n>>\n>>> We have relied on fstat(-1, &st) to correctly error out, and if MSVC build\n>>> crashes, it is a bug in its fstat() emulation, I would think.\n>>\n>> fileno(stdout) is alread wrong if stdout was closed.\n> \n> The \"-1\" in my message comes from here:\n> \n> DESCRIPTION\n> \n> The fileno() function shall return the integer file descriptor\n> associated with the stream pointed to by stream.\n> \n> RETURN VALUE\n> \n> Upon successful completion, fileno() shall return the integer value of\n> the file descriptor associated with stream. Otherwise, the value -1\n> shall be returned and errno set to indicate the error.\n\nBut in the description of fclose() there is also:\n\n After the call to fclose(), any use of stream results in undefined\n behavior.\n\nAnd we do call fclose(stdout) in cmd_format_patch.\n\n-- Hannes\n"},{"id":"179765","messageId":"7vipmeijp0.fsf@alter.siamese.dyndns.org","threadId":"28980","inReplyTo":"4EC8C7FE.5050903@kdbg.org","subject":"Re: [msysGit] Re: [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-20T21:10:35Z","receivedAt":"2011-11-20T21:10:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> But in the description of fclose() there is also:\n>\n> After the call to fclose(), any use of stream results in undefined\n> behavior.\n>\n> And we do call fclose(stdout) in cmd_format_patch.\n\nYeah; this means the \"in case builtin implementation is too lazy to clean\nup after themselves, run_builtin() will clean things up for them\" approach\ntaken by 0f15731 (Check for IO errors after running a command, 2007-06-24)\nfundamentally does not work, which is a bit sad.\n"},{"id":"179816","messageId":"4ECB4501.1060100@lyx.org","threadId":"28980","inReplyTo":"4EC80D84.2010202@kdbg.org","subject":"Re: [msysGit] [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Vincent van Ravesteijn","fromEmail":"vfr@lyx.org","sentAt":"2011-11-22T06:45:21Z","receivedAt":"2011-11-22T06:45:21Z","isPatch":true,"sender":{"key":"vfr@lyx.org","avatar":"https://avatars.githubusercontent.com/u/687868?v=4"},"body":"\n\n>> To prevent the crash and to prevent git from writing cruft into the patch\n>> file, we do not close stdout, but redirect it to \"nul\" instead.\n> A more robust solution is to add invalidcontinue.obj to the linker\n> command line. This installs an invalid argument handler that does not\n> abort, and restores a sane behavior.\n\nThis seems to work only for release builds or did I miss something ?\n\nIs there any argument to not redirect stdout to \"/dev/null\" on all \nplatforms ?\n\nVincent\n"},{"id":"179848","messageId":"4ECC04E3.70407@kdbg.org","threadId":"28980","inReplyTo":"4ECB4501.1060100@lyx.org","subject":"Re: [msysGit] [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-11-22T20:24:03Z","receivedAt":"2011-11-22T20:24:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.11.2011 07:45, schrieb Vincent van Ravesteijn:\n>>> To prevent the crash and to prevent git from writing cruft into the\n>>> patch\n>>> file, we do not close stdout, but redirect it to \"nul\" instead.\n>> A more robust solution is to add invalidcontinue.obj to the linker\n>> command line. This installs an invalid argument handler that does not\n>> abort, and restores a sane behavior.\n> \n> This seems to work only for release builds or did I miss something ?\n\nI cannot tell, but I would be surprised if that were the case. I didn't\ntest with debug builds (only our 'make MSVC=1' build). A cursory look at\nthe CRT source code does not show anything that would be different\nbetween a debug and a release build regarding invalidcontinue.obj.\n\n> Is there any argument to not redirect stdout to \"/dev/null\" on all\n> platforms ?\n\nYou paper over a crack in the wall. You hide a bug.\n\n-- Hannes\n"},{"id":"179849","messageId":"4ECC13CD.5000608@lyx.org","threadId":"28980","inReplyTo":"4ECC04E3.70407@kdbg.org","subject":"Re: [msysGit] [PATCH 1/2] MSVC: Do not close stdout to prevent a crash","fromName":"Vincent van Ravesteijn","fromEmail":"vfr@lyx.org","sentAt":"2011-11-22T21:27:41Z","receivedAt":"2011-11-22T21:27:41Z","isPatch":true,"sender":{"key":"vfr@lyx.org","avatar":"https://avatars.githubusercontent.com/u/687868?v=4"},"body":"\n>> Is there any argument to not redirect stdout to \"/dev/null\" on all\n>> platforms ?\n> You paper over a crack in the wall. You hide a bug.\n\nLuckily I'm still trying to find the crack and not ready to tape \nanything yet.\n\nVincent\n"}]}