Hi Hannes, On Thu, 28 Nov 2019, Johannes Sixt wrote: > I'm sorry for being a bit slow lately. I found time to test this patch > only today. Out of curiosity: did you apply the patch on `master`, or on anything different? I ask because... > Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget: > > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > > > 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. > > I think the new code does not do that correctly. I observe a failure in > test #2 in t0061, which is this one: > > test_expect_success 'start_command reports ENOENT (slash)' ' > test-tool run-command start-command-ENOENT ./does-not-exist 2>err && > test_i18ngrep "\./does-not-exist" err > ' > > It does not even get to test_i18ngrep (test-tool fails), and err > contains this: > > error: cannot spawn ./does-not-exist: Result too large > FAIL start-command-ENOENT > > That "Result too large" is ERANGE. > > Don't you observe that, too? (I'm testing on Windows 10, BTW.) No, I don't observe that. And neither does the CI build: https://github.com/gitgitgadget/git/runs/317137275 This CI build tested the `js/mingw-inherit-only-std-handles` branch that is mirrored into gitgitgadget/git from https://github.com/gitster/git. Could you try with that branch, to see whether it "magically" fixes the issue you are seeing? > I've done a bit of tracing, see below. Much appreciated. > > 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@xxxxxx> > > --- > > compat/mingw.c | 120 +++++++++++++++++++++++++++++++++++++---- > > t/t0061-run-command.sh | 2 +- > > 2 files changed, 110 insertions(+), 12 deletions(-) > > > > 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(); > > CreateProcessW failed, so we arrive here. At this point, err is 2 > (ERROR_FILE_NOT_FOUND) as expected. > > > + struct strbuf buf = STRBUF_INIT; > > + > > + if (err != ERROR_NO_SYSTEM_RESOURCES && > > Then this is true, ... > > > + /* > > + * 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) && > > ... the bracketed expression is false, but it's negated, so it's true > again, ... > > > + !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) { > > ... and the variable isn't set, so we continue here. (But I don't think > it is important.) > > > + 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); > > Then this one fails again (with GetLastError() == 2, too, as expected). > > > + 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; > > And then we take this error exist. At this point, GetLastError() == 0 > (probably from the successful cleanup functions), but errno == 34 > (ERANGE), probably a fallout from one of the xutftowcs that we do > earlier (I didn't check). The point is, we leave the function with a > failure indication, but without having set errno. > > Any ideas? Strange. When I debug this, errno is indeed still set from before, but to ENOENT. I wonder how you get that ERANGE when I get an ENOENT (and so do all the CI/PR builds that did not catch this). Will send a fix shortly. Thanks, Dscho > And why don't you observe the failure? A coincidence? > > > - } > > + > > CloseHandle(pi.hThread); > > > > /* > > diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh > > index 473a3405ef..7d599675e3 100755 > > --- 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 > > ' > > > > > > -- Hannes >
