From cf2f73537ce5777f761bf4cc42667ce7dd45088b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Jan 2018 15:37:38 +0100 Subject: [PATCH] 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 GVFS 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. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 50 +++++++++++++++++++++++++++++++++++------- t/t0061-run-command.sh | 2 +- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 58db9cc356d3f9..a90acb0cd7b12b 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1594,8 +1594,12 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen int fhin, int fhout, int fherr) { static int atexit_handler_initialized; - STARTUPINFOW si; + 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; @@ -1643,11 +1647,19 @@ 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); + + if (si.StartupInfo.hStdInput != INVALID_HANDLE_VALUE) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdInput; + if (si.StartupInfo.hStdOutput != INVALID_HANDLE_VALUE) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdOutput; + if (si.StartupInfo.hStdError != INVALID_HANDLE_VALUE) + stdhandles[stdhandles_count++] = si.StartupInfo.hStdError; + if (stdhandles_count) + si.StartupInfo.dwFlags |= STARTF_USESTDHANDLES; /* executables and the current directory don't support long paths */ if (*argv && !strcmp(cmd, *argv)) @@ -1706,8 +1718,30 @@ 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 (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); + + if (si.lpAttributeList) + DeleteProcThreadAttributeList(si.lpAttributeList); + if (attr_list) + HeapFree(GetProcessHeap(), 0, attr_list); free(wenvblk); free(wargs); diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 5b66135f69d241..74f125d680a119 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -13,7 +13,7 @@ cat >hello-script <<-EOF EOF >empty -test_expect_failure MINGW 'subprocess inherits only std handles' ' +test_expect_success MINGW 'subprocess inherits only std handles' ' test-run-command inherited-handle '