From c5a03b1e29c69f3f06c8fabd92493edb73469176 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Nov 2019 14:41:03 +0000 Subject: 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 Signed-off-by: Junio C Hamano --- compat/winansi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'compat') diff --git a/compat/winansi.c b/compat/winansi.c index 54fd701cbf..c27b20a79d 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -662,10 +662,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; } -- cgit v1.2.3 From 9a780a384de21a35866a380247b34442b5ca3bb8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Nov 2019 14:41:04 +0000 Subject: mingw: spawned processes need to inherit only standard handles By default, CreateProcess() does not inherit any open file handles, unless the bInheritHandles parameter is set to TRUE. Which we do need to set because we need to pass in stdin/stdout/stderr to talk to the child processes. Sadly, this means that all file handles (unless marked via O_NOINHERIT) are inherited. This lead to problems in VFS for Git, where a long-running read-object hook is used to hydrate missing objects, and depending on the circumstances, might only be called *after* Git opened a file handle. Ideally, we would not open files without O_NOINHERIT unless *really* necessary (i.e. when we want to pass the opened file handle as standard handle into a child process), but apparently it is all-too-easy to introduce incorrect open() calls: this happened, and prevented updating a file after the read-object hook was started because the hook still held a handle on said file. Happily, there is a solution: as described in the "Old New Thing" https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873 there is a way, starting with Windows Vista, that lets us define precisely which handles should be inherited by the child process. And since we bumped the minimum Windows version for use with Git for Windows to Vista with v2.10.1 (i.e. a *long* time ago), we can use this method. So let's do exactly that. We need to make sure that the list of handles to inherit does not contain duplicates; Otherwise CreateProcessW() would fail with ERROR_INVALID_ARGUMENT. While at it, stop setting errno to ENOENT unless it really is the correct value. Also, fall back to not limiting handle inheritance under certain error conditions (e.g. on Windows 7, which is a lot stricter in what handles you can specify to limit to). Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/mingw.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 109 insertions(+), 11 deletions(-) (limited to 'compat') diff --git a/compat/mingw.c b/compat/mingw.c index fe609239dd..cac18cc3da 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1398,8 +1398,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen const char *dir, int prepend_cmd, int fhin, int fhout, int fherr) { - STARTUPINFOW si; + static int restrict_handle_inheritance = 1; + STARTUPINFOEXW si; PROCESS_INFORMATION pi; + LPPROC_THREAD_ATTRIBUTE_LIST attr_list = NULL; + HANDLE stdhandles[3]; + DWORD stdhandles_count = 0; + SIZE_T size; struct strbuf args; wchar_t wcmd[MAX_PATH], wdir[MAX_PATH], *wargs, *wenvblk = NULL; unsigned flags = CREATE_UNICODE_ENVIRONMENT; @@ -1435,11 +1440,23 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen CloseHandle(cons); } memset(&si, 0, sizeof(si)); - si.cb = sizeof(si); - si.dwFlags = STARTF_USESTDHANDLES; - si.hStdInput = winansi_get_osfhandle(fhin); - si.hStdOutput = winansi_get_osfhandle(fhout); - si.hStdError = winansi_get_osfhandle(fherr); + si.StartupInfo.cb = sizeof(si); + si.StartupInfo.hStdInput = winansi_get_osfhandle(fhin); + si.StartupInfo.hStdOutput = winansi_get_osfhandle(fhout); + si.StartupInfo.hStdError = winansi_get_osfhandle(fherr); + + /* The list of handles cannot contain duplicates */ + if (si.StartupInfo.hStdInput != INVALID_HANDLE_VALUE) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdInput; + if (si.StartupInfo.hStdOutput != INVALID_HANDLE_VALUE && + si.StartupInfo.hStdOutput != si.StartupInfo.hStdInput) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdOutput; + if (si.StartupInfo.hStdError != INVALID_HANDLE_VALUE && + si.StartupInfo.hStdError != si.StartupInfo.hStdInput && + si.StartupInfo.hStdError != si.StartupInfo.hStdOutput) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdError; + if (stdhandles_count) + si.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; if (*argv && !strcmp(cmd, *argv)) wcmd[0] = L'\0'; @@ -1472,16 +1489,97 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen wenvblk = make_environment_block(deltaenv); memset(&pi, 0, sizeof(pi)); - ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, TRUE, - flags, wenvblk, dir ? wdir : NULL, &si, &pi); + if (restrict_handle_inheritance && stdhandles_count && + (InitializeProcThreadAttributeList(NULL, 1, 0, &size) || + GetLastError() == ERROR_INSUFFICIENT_BUFFER) && + (attr_list = (LPPROC_THREAD_ATTRIBUTE_LIST) + (HeapAlloc(GetProcessHeap(), 0, size))) && + InitializeProcThreadAttributeList(attr_list, 1, 0, &size) && + UpdateProcThreadAttribute(attr_list, 0, + PROC_THREAD_ATTRIBUTE_HANDLE_LIST, + stdhandles, + stdhandles_count * sizeof(HANDLE), + NULL, NULL)) { + si.lpAttributeList = attr_list; + flags |= EXTENDED_STARTUPINFO_PRESENT; + } + + ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, + stdhandles_count ? TRUE : FALSE, + flags, wenvblk, dir ? wdir : NULL, + &si.StartupInfo, &pi); + + /* + * On Windows 2008 R2, it seems that specifying certain types of handles + * (such as FILE_TYPE_CHAR or FILE_TYPE_PIPE) will always produce an + * error. Rather than playing finicky and fragile games, let's just try + * to detect this situation and simply try again without restricting any + * handle inheritance. This is still better than failing to create + * processes. + */ + if (!ret && restrict_handle_inheritance && stdhandles_count) { + DWORD err = GetLastError(); + struct strbuf buf = STRBUF_INIT; + + if (err != ERROR_NO_SYSTEM_RESOURCES && + /* + * On Windows 7 and earlier, handles on pipes and character + * devices are inherited automatically, and cannot be + * specified in the thread handle list. Rather than trying + * to catch each and every corner case (and running the + * chance of *still* forgetting a few), let's just fall + * back to creating the process without trying to limit the + * handle inheritance. + */ + !(err == ERROR_INVALID_PARAMETER && + GetVersion() >> 16 < 9200) && + !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) { + DWORD fl = 0; + 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"); + } + 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); + if (ret && buf.len) { + errno = err_win_to_posix(GetLastError()); + warning("failed to restrict file handles (%ld)\n\n%s", + err, buf.buf); + } + strbuf_release(&buf); + } else if (!ret) + errno = err_win_to_posix(GetLastError()); + + if (si.lpAttributeList) + DeleteProcThreadAttributeList(si.lpAttributeList); + if (attr_list) + HeapFree(GetProcessHeap(), 0, attr_list); free(wenvblk); free(wargs); - if (!ret) { - errno = ENOENT; + if (!ret) return -1; - } + CloseHandle(pi.hThread); /* -- cgit v1.2.3 From ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Nov 2019 14:41:05 +0000 Subject: mingw: restrict file handle inheritance only on Windows 7 and later Turns out that it don't work so well on Vista, see https://github.com/git-for-windows/git/issues/1742 for details. According to https://devblogs.microsoft.com/oldnewthing/?p=8873, it *should* work on Windows Vista and later. But apparently there are issues on Windows Vista when pipes are involved. Given that Windows Vista is past its end of life (official support ended on April 11th, 2017), let's not spend *too* much time on this issue and just disable the file handle inheritance restriction on any Windows version earlier than Windows 7. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/mingw.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'compat') diff --git a/compat/mingw.c b/compat/mingw.c index cac18cc3da..2b6eca2f56 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -212,6 +212,7 @@ enum hide_dotfiles_type { HIDE_DOTFILES_DOTGITONLY }; +static int core_restrict_inherited_handles = -1; static enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY; static char *unset_environment_variables; @@ -231,6 +232,15 @@ int mingw_core_config(const char *var, const char *value, void *cb) return 0; } + if (!strcmp(var, "core.restrictinheritedhandles")) { + if (value && !strcasecmp(value, "auto")) + core_restrict_inherited_handles = -1; + else + core_restrict_inherited_handles = + git_config_bool(var, value); + return 0; + } + return 0; } @@ -1398,7 +1408,7 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen const char *dir, int prepend_cmd, int fhin, int fhout, int fherr) { - static int restrict_handle_inheritance = 1; + static int restrict_handle_inheritance = -1; STARTUPINFOEXW si; PROCESS_INFORMATION pi; LPPROC_THREAD_ATTRIBUTE_LIST attr_list = NULL; @@ -1413,6 +1423,16 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen const char *(*quote_arg)(const char *arg) = is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc; + if (restrict_handle_inheritance < 0) + restrict_handle_inheritance = core_restrict_inherited_handles; + /* + * The following code to restrict which handles are inherited seems + * to work properly only on Windows 7 and later, so let's disable it + * on Windows Vista and 2008. + */ + if (restrict_handle_inheritance < 0) + restrict_handle_inheritance = GetVersion() >> 16 >= 7601; + do_unset_environment_variables(); /* Determine whether or not we are associated to a console */ -- cgit v1.2.3 From 4d0375ca24f6e317e94705e74c89f33c66c35107 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 30 Nov 2019 10:36:39 +0000 Subject: mingw: do set `errno` correctly when trying to restrict handle inheritance In 9a780a384de (mingw: spawned processes need to inherit only standard handles, 2019-11-22), we taught the Windows-specific part to restrict which file handles are passed on to the spawned processes. Since this logic seemed to be a bit fragile across Windows versions (we _still_ support Windows Vista in Git for Windows, for example), a fall-back was added to try spawning the process again, this time without restricting which file handles are to be inherited by the spawned process. In the common case (i.e. when the process could not be spawned for reasons _other_ than the file handle inheritance), the fall-back attempt would still fail, of course. Crucially, one thing we missed in that code path was to set `errno` appropriately. This should have been caught by t0061.2 which expected `errno` to be `ENOENT` after trying to start a process for a non-existing executable, but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call: while looking for the config settings for trace2, Git tries to access `xdg_config` and `user_config` via `access_or_die()`, and as neither of those config files exists when running the test case (because in Git's test suite, `HOME` points to the test directory), the `errno` has the expected value, but for the wrong reasons. Let's fix that by making sure that `errno` is set correctly. It even appears that `errno` was set in the _wrong_ case previously: `CreateProcessW()` returns non-zero upon success, but `errno` was set only in the non-zero case. It would be nice if we could somehow fix t0061 to make sure that this does not regress again. One approach that seemed like it should work, but did not, was to set `errno` to 0 in the test helper that is used by t0061.2. However, when `mingw_spawnvpe()` wants to see whether the file in question is a script, it calls `parse_interpreter()`, which in turn tries to `open()` the file. Obviously, this call fails, and sets `errno` to `ENOENT`, deep inside the call chain started from that test helper. Instead, we force re-set `errno` at the beginning of the function `mingw_spawnve_fd()`, which _should_ be safe given that callers of that function will want to look at `errno` if -1 was returned. And if that `errno` is 0 ("No error"), regression tests like t0061.2 will kick in. Reported-by: Johannes Sixt Signed-off-by: Johannes Schindelin Acked-by: Johannes Sixt Signed-off-by: Junio C Hamano --- compat/mingw.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'compat') diff --git a/compat/mingw.c b/compat/mingw.c index 2b6eca2f56..432adc1aed 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen const char *(*quote_arg)(const char *arg) = is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc; + /* Make sure to override previous errors, if any */ + errno = 0; + if (restrict_handle_inheritance < 0) restrict_handle_inheritance = core_restrict_inherited_handles; /* @@ -1580,8 +1583,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL, TRUE, flags, wenvblk, dir ? wdir : NULL, &si.StartupInfo, &pi); - if (ret && buf.len) { + if (!ret) errno = err_win_to_posix(GetLastError()); + if (ret && buf.len) { warning("failed to restrict file handles (%ld)\n\n%s", err, buf.buf); } -- cgit v1.2.3 From 3ba3720b3f187644ac0186a4637898f8e8f85a91 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 2 Dec 2019 11:33:29 +0000 Subject: mingw: forbid translating ERROR_SUCCESS to an errno value Johannes Sixt pointed out that the `err_win_to_posix()` function mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`. The only purpose of this function is to map Win32 API errors to `errno` ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea of `errno` is that it will only be set in case of an error, and left alone in case of success. Therefore, as pointed out by Junio Hamano, it is a bug to call this function when there was not even any error to map. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- compat/mingw.c | 1 + 1 file changed, 1 insertion(+) (limited to 'compat') diff --git a/compat/mingw.c b/compat/mingw.c index 432adc1aed..827065d96d 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr) case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break; case ERROR_SHARING_VIOLATION: error = EACCES; break; case ERROR_STACK_OVERFLOW: error = ENOMEM; break; + case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!"); case ERROR_SWAPERROR: error = ENOENT; break; case ERROR_TOO_MANY_MODULES: error = EMFILE; break; case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break; -- cgit v1.2.3