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 <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/compat/mingw.c b/compat/mingw.c
index fe609239dd6ba22a843b243647868f887014d40e..cac18cc3daaa093788348d3dadafb67dcc3e8ddb 100644 (file)
--- 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);
 
        /*

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 473a3405efb5111eecd14ac4e8dba8b4195eb9dd..7d599675e35a75637a75ab6bfed19ff7f784a312 100755 (executable)
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -12,7 +12,7 @@ cat >hello-script <<-EOF
        cat hello-script
 EOF
 
-test_expect_failure MINGW 'subprocess inherits only std handles' '
+test_expect_success MINGW 'subprocess inherits only std handles' '
        test-tool run-command inherited-handle
 '