[PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes

[PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes

[Johannes Schindelin via GitGitGadget]

This is yet another of those patch series that matured in Git for Windows
for over a year before being "upstreamed".

The problem to be solved: files cannot be deleted on Windows when even one
process has an open file handle to it. So when a process opens a temporary
file, then spawns a child process that inherits that file handle by mistake,
and then the parent process tries to delete the temporary file while the
child process is still running, the deletion will fail. (This description is
slightly simplified, see the commit message "spawned processes need to
inherit only standard handles" for more detail.)

Technically, we might want to squash "restrict file handle inheritance only
on Windows 7 and later" into "spawned processes need to inherit only
standard handles", but while preparing this patch series, I found the story
easier to follow with them still being separate.

The real reason why I submit this now is that I needed some ready-to-submit
patch series as an excuse to test GitGitGadget on https://github.com/git/git
.

Johannes Schindelin (4):
  mingw: demonstrate that all file handles are inherited by child
    processes
  mingw: work around incorrect standard handles
  mingw: spawned processes need to inherit only standard handles
  mingw: restrict file handle inheritance only on Windows 7 and later

 Documentation/config/core.txt |   6 ++
 compat/mingw.c                | 140 +++++++++++++++++++++++++++++++---
 compat/winansi.c              |  12 ++-
 t/helper/test-run-command.c   |  44 +++++++++++
 t/t0061-run-command.sh        |   4 +
 5 files changed, 194 insertions(+), 12 deletions(-)


base-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-670%2Fdscho%2Finherit-only-stdhandles-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-670/dscho/inherit-only-stdhandles-v1
Pull-Request: https://github.com/git/git/pull/670
-- 
gitgitgadget

[PATCH 1/4] mingw: demonstrate that all file handles are inherited by child processes

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When spawning child processes, we really should be careful which file
handles we let them inherit.

This is doubly important on Windows, where we cannot rename, delete, or
modify files if there is still a file handle open.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/helper/test-run-command.c | 44 +++++++++++++++++++++++++++++++++++++
 t/t0061-run-command.sh      |  4 ++++
 2 files changed, 48 insertions(+)

diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index ead6dc611a..40ec4dbb6e 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -200,6 +200,46 @@ static int testsuite(int argc, const char **argv)
 	return !!ret;
 }
 
+static int inherit_handle(const char *argv0)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char path[PATH_MAX];
+	int tmp;
+
+	/* First, open an inheritable handle */
+	xsnprintf(path, sizeof(path), "out-XXXXXX");
+	tmp = xmkstemp(path);
+
+	argv_array_pushl(&cp.args,
+			 "test-tool", argv0, "inherited-handle-child", NULL);
+	cp.in = -1;
+	cp.no_stdout = cp.no_stderr = 1;
+	if (start_command(&cp) < 0)
+		die("Could not start child process");
+
+	/* Then close it, and try to delete it. */
+	close(tmp);
+	if (unlink(path))
+		die("Could not delete '%s'", path);
+
+	if (close(cp.in) < 0 || finish_command(&cp) < 0)
+		die("Child did not finish");
+
+	return 0;
+}
+
+static int inherit_handle_child(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (strbuf_read(&buf, 0, 0) < 0)
+		die("Could not read stdin");
+	printf("Received %s\n", buf.buf);
+	strbuf_release(&buf);
+
+	return 0;
+}
+
 int cmd__run_command(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
@@ -207,6 +247,10 @@ int cmd__run_command(int argc, const char **argv)
 
 	if (argc > 1 && !strcmp(argv[1], "testsuite"))
 		exit(testsuite(argc - 1, argv + 1));
+	if (!strcmp(argv[1], "inherited-handle"))
+		exit(inherit_handle(argv[0]));
+	if (!strcmp(argv[1], "inherited-handle-child"))
+		exit(inherit_handle_child());
 
 	if (argc < 3)
 		return 1;
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 17c9c0f3bb..473a3405ef 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh
@@ -12,6 +12,10 @@ cat >hello-script <<-EOF
 	cat hello-script
 EOF
 
+test_expect_failure MINGW 'subprocess inherits only std handles' '
+	test-tool run-command inherited-handle
+'
+
 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
-- 
gitgitgadget

[PATCH 2/4] mingw: work around incorrect standard handles

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

For some reason, when being called via TortoiseGit the standard handles,
or at least what is returned by _get_osfhandle(0) for standard input,
can take on the value (HANDLE)-2 (which is not a legal value, according
to the documentation).

Even if this value is not documented anywhere, CreateProcess() seems to
work fine without complaints if hStdInput set to this value.

In contrast, the upcoming code to restrict which file handles get
inherited by spawned processes would result in `ERROR_INVALID_PARAMETER`
when including such handle values in the list.

To help this, special-case the value (HANDLE)-2 returned by
_get_osfhandle() and replace it with INVALID_HANDLE_VALUE, which will
hopefully let the handle inheritance restriction work even when called
from TortoiseGit.

This fixes https://github.com/git-for-windows/git/issues/1481

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index 54fd701cbf..c27b20a79d 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -662,10 +662,20 @@ void winansi_init(void)
  */
 HANDLE winansi_get_osfhandle(int fd)
 {
+	HANDLE ret;
+
 	if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
 		return hconsole1;
 	if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
 		return hconsole2;
 
-	return (HANDLE)_get_osfhandle(fd);
+	ret = (HANDLE)_get_osfhandle(fd);
+
+	/*
+	 * There are obviously circumstances under which _get_osfhandle()
+	 * returns (HANDLE)-2. This is not documented anywhere, but that is so
+	 * clearly an invalid handle value that we can just work around this
+	 * and return the correct value for invalid handles.
+	 */
+	return ret == (HANDLE)-2 ? INVALID_HANDLE_VALUE : ret;
 }
-- 
gitgitgadget

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

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

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();
+		struct strbuf buf = STRBUF_INIT;
+
+		if (err != ERROR_NO_SYSTEM_RESOURCES &&
+		    /*
+		     * 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) &&
+		    !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {
+			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);
+		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;
-	}
+
 	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
 '
 
-- 
gitgitgadget

[PATCH 4/4] mingw: restrict file handle inheritance only on Windows 7 and later

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Turns out that it don't work so well on Vista, see
https://github.com/git-for-windows/git/issues/1742 for details.

According to https://devblogs.microsoft.com/oldnewthing/?p=8873, it
*should* work on Windows Vista and later.

But apparently there are issues on Windows Vista when pipes are
involved. Given that Windows Vista is past its end of life (official
support ended on April 11th, 2017), let's not spend *too* much time on
this issue and just disable the file handle inheritance restriction on
any Windows version earlier than Windows 7.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/config/core.txt |  6 ++++++
 compat/mingw.c                | 22 +++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 852d2ba37a..ad4fa4dccd 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -559,6 +559,12 @@ core.unsetenvvars::
 	Defaults to `PERL5LIB` to account for the fact that Git for
 	Windows insists on using its own Perl interpreter.
 
+core.restrictinheritedhandles::
+	Windows-only: override whether spawned processes inherit only standard
+	file handles (`stdin`, `stdout` and `stderr`) or all handles. Can be
+	`auto`, `true` or `false`. Defaults to `auto`, which means `true` on
+	Windows 7 and later, and `false` on older Windows versions.
+
 core.createObject::
 	You can set this to 'link', in which case a hardlink followed by
 	a delete of the source are used to make sure that object creation
diff --git a/compat/mingw.c b/compat/mingw.c
index cac18cc3da..2b6eca2f56 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -212,6 +212,7 @@ enum hide_dotfiles_type {
 	HIDE_DOTFILES_DOTGITONLY
 };
 
+static int core_restrict_inherited_handles = -1;
 static enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
 static char *unset_environment_variables;
 
@@ -231,6 +232,15 @@ int mingw_core_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.restrictinheritedhandles")) {
+		if (value && !strcasecmp(value, "auto"))
+			core_restrict_inherited_handles = -1;
+		else
+			core_restrict_inherited_handles =
+				git_config_bool(var, value);
+		return 0;
+	}
+
 	return 0;
 }
 
@@ -1398,7 +1408,7 @@ 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)
 {
-	static int restrict_handle_inheritance = 1;
+	static int restrict_handle_inheritance = -1;
 	STARTUPINFOEXW si;
 	PROCESS_INFORMATION pi;
 	LPPROC_THREAD_ATTRIBUTE_LIST attr_list = NULL;
@@ -1413,6 +1423,16 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 	const char *(*quote_arg)(const char *arg) =
 		is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;
 
+	if (restrict_handle_inheritance < 0)
+		restrict_handle_inheritance = core_restrict_inherited_handles;
+	/*
+	 * The following code to restrict which handles are inherited seems
+	 * to work properly only on Windows 7 and later, so let's disable it
+	 * on Windows Vista and 2008.
+	 */
+	if (restrict_handle_inheritance < 0)
+		restrict_handle_inheritance = GetVersion() >> 16 >= 7601;
+
 	do_unset_environment_variables();
 
 	/* Determine whether or not we are associated to a console */
-- 
gitgitgadget

Re: [PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> The problem to be solved: files cannot be deleted on Windows when even one
> process has an open file handle to it. So when a process opens a temporary
> file, then spawns a child process that inherits that file handle by mistake,
> and then the parent process tries to delete the temporary file while the
> child process is still running, the deletion will fail. (This description is
> slightly simplified, see the commit message "spawned processes need to
> inherit only standard handles" for more detail.)

Makes me wonder if we should be marking these fds with FD_CLOEXEC to
solve the issue in a way that is platform agnostic.  It may turn out
that we'd be better off to make it an explicit choice by the parent
when it leaves a FD open while spawning a child process (and by that
spawned child when it runs another executable---but I undertand that
it is a single-step operation, not a two-step one, on Windows).

In any case, synchronizing the differences in compat/ between our
trees is good.  Queued.

Re: [PATCH 0/4] On Windows, limit which file handles are inherited by spawned child processes

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Nov 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > The problem to be solved: files cannot be deleted on Windows when even one
> > process has an open file handle to it. So when a process opens a temporary
> > file, then spawns a child process that inherits that file handle by mistake,
> > and then the parent process tries to delete the temporary file while the
> > child process is still running, the deletion will fail. (This description is
> > slightly simplified, see the commit message "spawned processes need to
> > inherit only standard handles" for more detail.)
>
> Makes me wonder if we should be marking these fds with FD_CLOEXEC to
> solve the issue in a way that is platform agnostic.

I would like that a lot.

Having said that, it is too easy to miss when a patch forgets to do that
and when the contributor only tests on Linux. So I really also would like
to keep the current patch in addition.

Thanks,
Dscho

> It may turn out that we'd be better off to make it an explicit choice by
> the parent when it leaves a FD open while spawning a child process (and
> by that spawned child when it runs another executable---but I undertand
> that it is a single-step operation, not a two-step one, on Windows).
>
> In any case, synchronizing the differences in compat/ between our
> trees is good.  Queued.
>
>

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

[Johannes Sixt]

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

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

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

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

[Johannes Schindelin]

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
>

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

[Johannes Schindelin]

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

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

[Johannes Sixt]

Am 29.11.19 um 14:52 schrieb Johannes Schindelin:
> On Thu, 28 Nov 2019, Johannes Sixt wrote:
> Out of curiosity: did you apply the patch on `master`, or on anything
> different?

I tested this on top of e1874e422f3a ("Merge branch 'sg/test-bool-env'
into next", 2019-11-27), which is the commit before 4736894af6a1 ("Merge
branch 'js/mingw-inherit-only-std-handles' into next", 2019-11-27). I
merged your branch up to 9a780a384de2 ("mingw: spawned processes need to
inherit only standard handles", 2019-11-22) (plus one of my own patches
with Makefile adjustments).

> 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'll dig into this now.

-- Hannes

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

[Johannes Sixt]

Am 29.11.19 um 14:52 schrieb Johannes Schindelin:
> On Thu, 28 Nov 2019, Johannes Sixt wrote:
>> Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
>>> +		    !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {
>>
>> ... and the variable isn't set, so we continue here. (But I don't think
>> it is important.)

It's actually not that unimportant because ...

>>
>>> +			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);

... ERANGE happens here in the second iteration, in particular, when
strbuf_vaddf needs to grow the buffer. vsnprintf generates it.

>>> +			}
>>> +			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");
>>> +		}

-- Hannes

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

[Johannes Schindelin]

Hi Hannes,

On Fri, 29 Nov 2019, Johannes Sixt wrote:

> Am 29.11.19 um 14:52 schrieb Johannes Schindelin:
> > On Thu, 28 Nov 2019, Johannes Sixt wrote:
> >> Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
> >>> +		    !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {
> >>
> >> ... and the variable isn't set, so we continue here. (But I don't think
> >> it is important.)
>
> It's actually not that unimportant because ...
>
> >>
> >>> +			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);
>
> ... ERANGE happens here in the second iteration, in particular, when
> strbuf_vaddf needs to grow the buffer. vsnprintf generates it.

Ooops. I meant to ask you about using `NO_GETTEXT` in _this_ context. I
could imagine that gettext's `vsnprintf()` version behaves at least
slightly differently here, and probably different enough to cause an
`ERANGE` in your setup but not in mine.

Ciao,
Dscho

> >>> +			}
> >>> +			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");
> >>> +		}
>
> -- Hannes
>