Re: [PATCH 3/4] mingw: spawned processes need to inherit only standard handles
Hi Dscho,
I'm sorry for being a bit slow lately. I found time to test this patch only today.
Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
Show 34 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.)
I've done a bit of tracing, see below.
Show 95 quoted lines
>
> 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>
> ---
> 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 &&
Show 11 quoted lines
> + /*
> + * 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.)
Show 26 quoted lines
> + 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).
Show 21 quoted lines
> + 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?
And why don't you observe the failure? A coincidence?
Show 19 quoted lines
> - }
> +
> 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
> '
>
>
Johannes Schindelin via GitGitGadget
