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

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

Hi Hannes,

On Fri, 29 Nov 2019, Johannes Schindelin wrote:

> On Thu, 28 Nov 2019, Johannes Sixt wrote:
>
> [ ... analyzing a t0061.2 failure ...]
>
> > Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
>
> > > @@ -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).

I am really curious about this now...

As to why it passes on this here machine, the reason is that while looking
for the trace2 config, Git tries to access the xdg and the user config in
https://github.com/git/git/blob/v2.24.0/config.c#L1716 and in
https://github.com/git/git/blob/v2.24.0/config.c#L1719, respectively. In
Git's test suite, `HOME` is re-set to the test directory, and it does not
contain those config files, therefore `errno` is set to `ENOENT` by both
calls, and in my hands, Git does not re-set `errno` in the meantime.

Even after that trace2 code set `errno` (which we could re-set easily in
`t/helper/test-run-command.c`, as the trace2 initialization happens in
`common-main.c`, long before `cmd_main()` is called), there is _yet_
another part of the code path that sets `errno` to `ENOENT`, though: when
`mingw_spawnvpe()` wants to see whether the file in question is a script,
it calls `parse_interpreter()`, which in turn tries to `open()` the file:
https://github.com/git/git/blob/v2.24.0/compat/mingw.c#L1134. Obviously,
this call fails, and sets `errno` to `ENOENT`.

So it would appear that _something_ in your setup sets `errno` to a
different value _after_ the call to `parse_interpreter()`. Or maybe you
disabled that somehow in custom patches? Then that `ERANGE` still must
happen somewhere after the trace2 startup. I wonder what that something
would be that causes that `ERANGE`, but seemingly without causing even so
much as a warning.

> Will send a fix shortly.

Or not so shortly, after all ;-)

Ciao,
Dscho

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