| Message ID | feb197792814701bf36cb15b73e1e73aae2baa4d.1574433665.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | On Windows, limit which file handles are inherited by spawned child processes | expand |
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
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 >
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 > > >
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
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
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 >
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 '