git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Johannes Sixt <j6t@kdbg.org>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 3/4] mingw: spawned processes need to inherit only standard handles
Date: Fri, 29 Nov 2019 21:40:14 +0100 (CET)
Message-ID: <nycvar.QRO.7.76.6.1911292017321.31080@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1911291036320.31080@tvgsbejvaqbjf.bet>

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

  reply	other threads:[~2019-11-29 20:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-22 14:41 [PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes Johannes Schindelin via GitGitGadget
2019-11-22 14:41 ` [PATCH 1/4] mingw: demonstrate that all file handles are inherited by " Johannes Schindelin via GitGitGadget
2019-11-22 14:41 ` [PATCH 2/4] mingw: work around incorrect standard handles Johannes Schindelin via GitGitGadget
2019-11-22 14:41 ` [PATCH 3/4] mingw: spawned processes need to inherit only " Johannes Schindelin via GitGitGadget
2019-11-28 21:48   ` Johannes Sixt
2019-11-29 13:52     ` Johannes Schindelin
2019-11-29 20:40       ` Johannes Schindelin [this message]
2019-11-29 22:19       ` Johannes Sixt
2019-11-29 22:37       ` Johannes Sixt
2019-11-30 22:10         ` Johannes Schindelin
2019-11-22 14:41 ` [PATCH 4/4] mingw: restrict file handle inheritance only on Windows 7 and later Johannes Schindelin via GitGitGadget
2019-11-25  5:42 ` [PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes Junio C Hamano
2019-11-25 16:29   ` Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1911292017321.31080@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git