Merge branch 'inherit-only-stdhandles'

When spawning child processes, we do want them to inherit the standard
handles so that we can talk to them. We do *not* want them to inherit
any other handle, as that would hold a lock to the respective files
(preventing them from being renamed, modified or deleted), and the child
process would not know how to access that handle anyway.

Happily, there is an API to make that happen. It is supported in Windows
Vista and later, which is exactly what we promise to support in Git for
Windows for the time being.

This also means that we lift, at long last, the target Windows version
from Windows XP to Windows Vista.

Signed-off-by: Johannes Schindelin <[email protected]>

Author: dscho

compat/mingw.c

@@ -1583,8 +1583,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;
@@ -1621,11 +1626,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;
 
 	/* executables and the current directory don't support long paths */
 	if (*argv && !strcmp(cmd, *argv))
@@ -1684,16 +1701,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);
 
 	/*

compat/winansi.c

@@ -660,10 +660,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;
 }

t/helper/test-run-command.c

@@ -50,11 +50,58 @@ static int task_finished(int result,
 	return 1;
 }
 
+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;
 	int jobs;
 
+	if (argc < 2)
+		return 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;
 	while (!strcmp(argv[1], "env")) {

t/t0061-run-command.sh

@@ -12,6 +12,10 @@ cat >hello-script <<-EOF
 	cat hello-script
 EOF
 
+test_expect_success 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
 '