From 664ca487bb5c6daf54a7042f7efbabb9195f6d9b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 7 Feb 2018 13:50:03 +0100 Subject: [PATCH 1/3] reorder-before! mingw: spawned processes need to inherit only standard handles This patch should logically come before the patch which tries to limit the list of file handles to be inherited by spawned processes, to avoid introducing a regression before resolving it. mingw: work around incorrect standard handles For some reason, when being called via TortoiseGit the standard handles, or at least what is returned by _get_osfhandle(0) for standard input, can take on the value (HANDLE)-2 (which is not a legal value, according to the documentation). Even if this value is not documented anywhere, CreateProcess() seems to work fine without complaints if hStdInput set to this value. In contrast, the upcoming code to restrict which file handles get inherited by spawned processes would result in `ERROR_INVALID_PARAMETER` when including such handle values in the list. To help this, special-case the value (HANDLE)-2 returned by _get_osfhandle() and replace it with INVALID_HANDLE_VALUE, which will hopefully let the handle inheritance restriction work even when called from TortoiseGit. This fixes https://github.com/git-for-windows/git/issues/1481 Signed-off-by: Johannes Schindelin --- compat/winansi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/compat/winansi.c b/compat/winansi.c index efc0abcdac..38cd332b9d 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -660,10 +660,20 @@ void winansi_init(void) */ HANDLE winansi_get_osfhandle(int fd) { + HANDLE ret; + if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED)) return hconsole1; if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED)) return hconsole2; - return (HANDLE)_get_osfhandle(fd); + ret = (HANDLE)_get_osfhandle(fd); + + /* + * There are obviously circumstances under which _get_osfhandle() + * returns (HANDLE)-2. This is not documented anywhere, but that is so + * clearly an invalid handle value that we can just work around this + * and return the correct value for invalid handles. + */ + return ret == (HANDLE)-2 ? INVALID_HANDLE_VALUE : ret; } From 8a71352f386ff2c9cca7ce6c98cfcae6e42a130b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 5 Feb 2018 21:31:34 +0100 Subject: [PATCH 2/3] fixup! mingw: spawned processes need to inherit only standard handles The conditional fall-back when file handle inheritance restricting failed will always need to pass bInheritHandles = TRUE. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index eef8df5a50..081c39e29a 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1755,8 +1755,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen restrict_handle_inheritance = 0; flags &= ~EXTENDED_STARTUPINFO_PRESENT; ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, - stdhandles_count ? TRUE : FALSE, - flags, wenvblk, dir ? wdir : NULL, + TRUE, flags, wenvblk, dir ? wdir : NULL, &si.StartupInfo, &pi); } From 8b533f0b020393f4d65e402d2c81027c998e60b8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 5 Feb 2018 21:31:34 +0100 Subject: [PATCH 3/3] fixup! mingw: spawned processes need to inherit only standard handles Let's make the code a little more robust: when `CreateProcess()` fails and we tried to restrict handle inheritance, and when that failure was *still* not anticipated by the current code, warn loudly but fall back to not restricting the handle inheritance. Use an environment variable to avoid warning more than once per Git operation, because it would get very annoying if a simple `git fetch` reported the same type of warning about six times. This should avoid blocking users if there are still bugs left, but still give us the benefit of proper bug reports. While at it, reformat the CreateProcess() call to conform to Git's conventions. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 081c39e29a..89a516501e 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1750,13 +1750,40 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen * handle inheritance. This is still better than failing to create * processes. */ - if (!ret && GetLastError() == ERROR_NO_SYSTEM_RESOURCES && - restrict_handle_inheritance && stdhandles_count) { + if (!ret && restrict_handle_inheritance && stdhandles_count) { + DWORD err = GetLastError(); + if (err != ERROR_NO_SYSTEM_RESOURCES && + !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) { + struct strbuf buf = STRBUF_INIT; + DWORD fl; + int i; + + setenv("SUPPRESS_HANDLE_INHERITANCE_WARNING", "1", 1); + + for (i = 0; i < stdhandles_count; i++) { + HANDLE h = stdhandles[i]; + strbuf_addf(&buf, "handle #%d: %p (type %lx, " + "handle info (%d) %lx\n", i, h, + GetFileType(h), + GetHandleInformation(h, &fl), + fl); + } + strbuf_addstr(&buf, "\nThis is a bug; please report it " + "at\nhttps://github.com/git-for-windows/" + "git/issues/new\n\n" + "To suppress this warning, please set " + "the environment variable\n\n" + "\tSUPPRESS_HANDLE_INHERITANCE_WARNING=1" + "\n"); + warning("failed to restrict file handles (%ld)\n\n%s", + err, buf.buf); + strbuf_release(&buf); + } restrict_handle_inheritance = 0; flags &= ~EXTENDED_STARTUPINFO_PRESENT; ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, - TRUE, flags, wenvblk, dir ? wdir : NULL, - &si.StartupInfo, &pi); + TRUE, flags, wenvblk, dir ? wdir : NULL, + &si.StartupInfo, &pi); } if (!ret)