git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Sixt <j6t@kdbg.org>
Date
Nov 28, 2019, 21:48 UTC
Message-ID
<00ef72f5-b1fa-f449-0e00-23713bed0187@kdbg.org>
In-Reply-To
<feb197792814701bf36cb15b73e1e73aae2baa4d.1574433665.git.gitgitgadget@gmail.com>
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 &&
Then this is true, ...
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
>  '
>  
> 
-- Hannes
Previous: Johannes Schindelin via GitGitGadgetNext: Johannes Schindelin
Message 6 of 13 in “On Windows, limit which file handles are inherited by spawned child processes”
  1. 0/4 On Windows, limit which file handles are inherited by spawned child processesJohannes Schindelin via GitGitGadget, Nov 22, 2019
  2. 2/4 mingw: work around incorrect standard handlesJohannes Schindelin via GitGitGadget, Nov 22, 2019
  3. 1/4 mingw: demonstrate that all file handles are inherited by child processesJohannes Schindelin via GitGitGadget, Nov 22, 2019
  4. 4/4 mingw: restrict file handle inheritance only on Windows 7 and laterJohannes Schindelin via GitGitGadget, Nov 22, 2019
  5. 3/4 mingw: spawned processes need to inherit only standard handlesJohannes Schindelin via GitGitGadget, Nov 22, 2019
  6. Johannes SixtNov 28, 2019
  7. Johannes SchindelinNov 29, 2019
  8. Johannes SchindelinNov 29, 2019
  9. Johannes SixtNov 29, 2019
  10. Johannes SixtNov 29, 2019
  11. Johannes SchindelinNov 30, 2019
  12. Junio C HamanoNov 25, 2019
  13. Johannes SchindelinNov 25, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.