From 85140f3d35d9822597fb655b3870bfde49d4b978 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Jan 2018 16:34:59 +0100 Subject: [PATCH 1/6] mingw: demonstrate that all file handles are inherited by child processes 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. Sadly, we have to guard this test inside #ifdef WIN32: we need to use the value of the HANDLE directly, and that concept does not exist on Linux/Unix. Signed-off-by: Johannes Schindelin --- t/helper/test-run-command.c | 45 +++++++++++++++++++++++++++++++++++++ t/t0061-run-command.sh | 4 ++++ 2 files changed, 49 insertions(+) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 3170608517..6a8e6d1813 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -188,6 +188,45 @@ 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 */ + sprintf(path, "out-XXXXXX"); + tmp = xmkstemp(path); + + argv_array_pushl(&cp.args, 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_main(int argc, const char **argv) { struct child_process proc = CHILD_PROCESS_INIT; @@ -195,6 +234,12 @@ int cmd_main(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; proc.argv = (const char **)argv + 2; diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index c3087af0ea..5b66135f69 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -13,6 +13,10 @@ cat >hello-script <<-EOF EOF >empty +test_expect_failure MINGW 'subprocess inherits only std handles' ' + test-run-command inherited-handle +' + test_expect_success 'start_command reports ENOENT' ' test-run-command start-command-ENOENT ./does-not-exist ' From 035784a332b9cc8d942d4957497a4dcf36a06ebc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 25 Jan 2018 15:03:05 +0100 Subject: [PATCH 2/6] compat/poll: prepare for targeting Windows Vista Windows Vista (and later) actually have a working poll(), but we still cannot use it because it only works on sockets. So let's detect when we are targeting Windows Vista and undefine those constants, and define `pollfd` so that we can declare our own pollfd struct. Signed-off-by: Johannes Schindelin --- compat/poll/poll.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/compat/poll/poll.h b/compat/poll/poll.h index cd1995292a..1e1597360f 100644 --- a/compat/poll/poll.h +++ b/compat/poll/poll.h @@ -21,6 +21,21 @@ #ifndef _GL_POLL_H #define _GL_POLL_H +#if defined(_WIN32_WINNT) && _WIN32_WINNT >= 0x600 +/* Vista has its own, socket-only poll() */ +#undef POLLIN +#undef POLLPRI +#undef POLLOUT +#undef POLLERR +#undef POLLHUP +#undef POLLNVAL +#undef POLLRDNORM +#undef POLLRDBAND +#undef POLLWRNORM +#undef POLLWRBAND +#define pollfd compat_pollfd +#endif + /* fake a poll(2) environment */ #define POLLIN 0x0001 /* any readable data available */ #define POLLPRI 0x0002 /* OOB/Urgent readable data */ From 33e66736eebc3e38d52c019cdb8e15db91f7adf6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 25 Jan 2018 14:19:52 +0100 Subject: [PATCH 3/6] mingw: set _WIN32_WINNT explicitly for Git for Windows Previously, we only ever declared a target Windows version if compiling with Visual C. Which meant that we were relying on the MinGW headers to guess which Windows version we want to target... Let's be explicit about it, in particular because we actually want to bump the target Windows version to Vista (which we will do in the next commit). Signed-off-by: Johannes Schindelin --- git-compat-util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-compat-util.h b/git-compat-util.h index aebb3e2b1c..9108bb21b0 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -172,7 +172,7 @@ #define _SGI_SOURCE 1 #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ -# if defined (_MSC_VER) && !defined(_WIN32_WINNT) +# if !defined(_WIN32_WINNT) # define _WIN32_WINNT 0x0502 # endif #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ From 10f9247c1262672d98de50e836203309da5336b2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 25 Jan 2018 14:21:51 +0100 Subject: [PATCH 4/6] mingw: bump the minimum Windows version to Vista Quite some time ago, a last plea to the XP users out there who want to see Windows XP support in Git for Windows, asking them to get engaged and help, vanished into the depths of the universe. It is time to codify the ascent by the "silent majority" of XP users, and mark the minimum Windows version required for Git for Windows as Windows Vista. This, incidentally, lets us use quite a few nice new APIs. This also means that we no longer need the inet_pton() and inet_ntop() emulation, and we no longer need to do the PROC_ADDR dance with the `CreateSymbolicLinkW()` function, either. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 3 +-- config.mak.uname | 4 ---- git-compat-util.h | 2 +- 3 files changed, 2 insertions(+), 7 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index cccb19a69a..58db9cc356 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -274,7 +274,6 @@ int mingw_core_config(const char *var, const char *value, void *cb) } static DWORD symlink_file_flags = 0, symlink_directory_flags = 1; -DECLARE_PROC_ADDR(kernel32.dll, BOOLEAN, CreateSymbolicLinkW, LPCWSTR, LPCWSTR, DWORD); enum phantom_symlink_result { PHANTOM_SYMLINK_RETRY, @@ -2687,7 +2686,7 @@ int symlink(const char *target, const char *link) int len; /* fail if symlinks are disabled or API is not supported (WinXP) */ - if (!has_symlinks || !INIT_PROC_ADDR(CreateSymbolicLinkW)) { + if (!has_symlinks) { errno = ENOSYS; return -1; } diff --git a/config.mak.uname b/config.mak.uname index 63ab6b7ed0..ac904c8a5c 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -398,8 +398,6 @@ ifeq ($(uname_S),Windows) NO_GETTEXT = YesPlease NO_PYTHON = YesPlease ETAGS_TARGET = ETAGS - NO_INET_PTON = YesPlease - NO_INET_NTOP = YesPlease NO_POSIX_GOODIES = UnfortunatelyYes NATIVE_CRLF = YesPlease DEFAULT_HELP_FORMAT = html @@ -567,8 +565,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_REGEX = YesPlease NO_PYTHON = YesPlease ETAGS_TARGET = ETAGS - NO_INET_PTON = YesPlease - NO_INET_NTOP = YesPlease NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 diff --git a/git-compat-util.h b/git-compat-util.h index 9108bb21b0..d413068183 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -173,7 +173,7 @@ #if defined(WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */ # if !defined(_WIN32_WINNT) -# define _WIN32_WINNT 0x0502 +# define _WIN32_WINNT 0x0600 # endif #define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */ #include From 6ff52e9e3d61843d83257c76d6cf388a98720f22 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 7 Feb 2018 13:50:03 +0100 Subject: [PATCH 5/6] mingw: spawned processes need to inherit only standard handles This patch should logically come before the patch which tries to limit the list of file handles to be inherited by spawned processes, to avoid introducing a regression before resolving it. mingw: work around incorrect standard handles 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 --- compat/winansi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/compat/winansi.c b/compat/winansi.c index efc0abcdac..38cd332b9d 100644 --- a/compat/winansi.c +++ b/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; } From e1cd96287644462fc84ee2f13c810def87b32e4c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 26 Jan 2018 15:37:38 +0100 Subject: [PATCH 6/6] mingw: spawned processes need to inherit only standard handles 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 GVFS 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 --- compat/mingw.c | 121 +++++++++++++++++++++++++++++++++++++---- t/t0061-run-command.sh | 2 +- 2 files changed, 110 insertions(+), 13 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index 58db9cc356..7503e5c291 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1593,9 +1593,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen const char *dir, const char *prepend_cmd, int fhin, int fhout, int fherr) { - static int atexit_handler_initialized; - STARTUPINFOW si; + static int atexit_handler_initialized, 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; @@ -1643,11 +1647,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)) @@ -1706,16 +1722,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) { + 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 5b66135f69..74f125d680 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -13,7 +13,7 @@ cat >hello-script <<-EOF EOF >empty -test_expect_failure MINGW 'subprocess inherits only std handles' ' +test_expect_success MINGW 'subprocess inherits only std handles' ' test-run-command inherited-handle '