Re: [PATCH 3/4] mingw: spawned processes need to inherit only standard handles

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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@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.)

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@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 &&
>
> 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
>