On Windows, limit which file handles are inherited by spawned child processes
cover On Windows, limit which file handles are inherited by spawned child processes
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
Signature · 1 line
gitgitgadget
PATCH 2/4 mingw: work around incorrect standard handles
From: Johannes Schindelin <[email protected]>
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
--- compat/winansi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
compat/winansi.c +11 -1
@@ -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; }
Signature · 1 line
gitgitgadget
PATCH 1/4 mingw: demonstrate that all file handles are inherited by child processes
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- t/helper/test-run-command.c | 44 +++++++++++++++++++++++++++++++++++++ t/t0061-run-command.sh | 4 ++++ 2 files changed, 48 insertions(+)
t/helper/test-run-command.c +44 -0
@@ -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 ;
t/t0061-run-command.sh +4 -0
@@ -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
Signature · 1 line
gitgitgadget
PATCH 3/4 mingw: spawned processes need to inherit only standard handles
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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).
--- compat/mingw.c | 120 +++++++++++++++++++++++++++++++++++++---- t/t0061-run-command.sh | 2 +- 2 files changed, 110 insertions(+), 12 deletions(-)
compat/mingw.c +109 -11
@@ -1398,8 +1398,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaenconst 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 **deltaenCloseHandle (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);/*
t/t0061-run-command.sh +1 -1
@@ -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 '
Signature · 1 line
gitgitgadget
PATCH 4/4 mingw: restrict file handle inheritance only on Windows 7 and later
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- Documentation/config/core.txt | 6 ++++++ compat/mingw.c | 22 +++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-)
Documentation/config/core.txt +6 -0
@@ -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
compat/mingw.c +21 -1
@@ -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 **deltaenconst 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 **deltaenconst 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 */
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · 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.
Hi Junio,
Quoted 11 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · 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.)
Quoted 2 lines
Makes me wonder if we should be marking these fds with
FD_CLOEXECto 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
Quoted 9 lines
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
In reply to Johannes Schindelin via GitGitGadgetHi 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:
Quoted 28 lines
From: Johannes Schindelin <[email protected]>
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 viaO_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_NOINHERITunless *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 incorrectopen()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.
Quoted 6 lines
We need to make sure that the list of handles to inherit does not contain duplicates; Otherwise
CreateProcessW()would fail withERROR_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.
Quoted 85 lines
Also, fall back to not limiting handle inheritance under certain error conditions (e.g. on Windows 7, which is a lot stricter in what handles you can specify to limit to).
Signed-off-by Johannes Schindelin <[email protected]>--- compat/mingw.c | 120 +++++++++++++++++++++++++++++++++++++---- t/t0061-run-command.sh | 2 +- 2 files changed, 110 insertions(+), 12 deletions(-)compat/mingw.c +43
-8@@ -1398,8 +1398,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaenconst 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 **deltaenCloseHandle (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); +
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1521
Quoted 10 lines
compat/mingw.c +10
-0+/* +* 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.
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1524
Quoted 3 lines
compat/mingw.c +3
-0+struct strbuf buf =STRBUF_INIT ; + + if (err != ERROR_NO_SYSTEM_RESOURCES &&
Then this is true, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1535
Quoted 11 lines
compat/mingw.c +11
-0+/* +* 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, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1536
Quoted 1 line
compat/mingw.c +1
-0+ !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {
... and the variable isn't set, so we continue here. (But I don't think it is important.)
Quoted 16 lines
compat/mingw.c +16
-0+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"
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1562
Quoted 10 lines
compat/mingw.c +10
-0+"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).
Quoted 11 lines
compat/mingw.c +11
-0+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);
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1581
Quoted 10 lines
compat/mingw.c +3
-2+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?
Quoted 11 lines
compat/mingw.c +1
-1- } +CloseHandle (pi.hThread);/* t/t0061-run-command.sh +0
-0
t/t0061-run-command.sh cat >hello-script <<-EOF · line 18
Quoted 8 lines
t/t0061-run-command.sh +1
-1@@ -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
Hi Hannes,
Quoted 2 lines · Johannes Sixt
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...
Quoted 47 lines
Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
Quoted 28 lines
From: Johannes Schindelin <[email protected]>
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 viaO_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_NOINHERITunless *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 incorrectopen()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.
Quoted 6 lines
We need to make sure that the list of handles to inherit does not contain duplicates; Otherwise
CreateProcessW()would fail withERROR_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_commandreports 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:
Quoted 6 lines
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?
Quoted 1 line
I've done a bit of tracing, see below.
Much appreciated.
Quoted 181 lines
Quoted 84 lines
Also, fall back to not limiting handle inheritance under certain error conditions (e.g. on Windows 7, which is a lot stricter in what handles you can specify to limit to).
Signed-off-by Johannes Schindelin <[email protected]>--- compat/mingw.c | 120 +++++++++++++++++++++++++++++++++++++---- t/t0061-run-command.sh | 2 +- 2 files changed, 110 insertions(+), 12 deletions(-)compat/mingw.c +43
-8@@ -1398,8 +1398,13 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaenconst 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 **deltaenCloseHandle (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); +compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1521
Quoted 10 lines
compat/mingw.c +10
-0+/* +* 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.compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1524
Quoted 3 lines
compat/mingw.c +3
-0+struct strbuf buf =STRBUF_INIT ; + + if (err != ERROR_NO_SYSTEM_RESOURCES &&Then this is true, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1535
Quoted 11 lines
compat/mingw.c +11
-0+/* +* 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, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1536
Quoted 1 line
compat/mingw.c +1
-0+ !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {... and the variable isn't set, so we continue here. (But I don't think it is important.)
Quoted 16 lines
compat/mingw.c +16
-0+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"compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1562
Quoted 10 lines
compat/mingw.c +10
-0+"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).Quoted 11 lines
compat/mingw.c +11
-0+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);compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1581
Quoted 10 lines
compat/mingw.c +3
-2+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.
Quoted 1 line
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
Quoted 24 lines
And why don't you observe the failure? A coincidence?
Quoted 11 lines
compat/mingw.c +1
-1- } +CloseHandle (pi.hThread);/* t/t0061-run-command.sh +0
-0t/t0061-run-command.sh cat >hello-script <<-EOF · line 18
Quoted 8 lines
t/t0061-run-command.sh +1
-1@@ -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
Hi Hannes,
Quoted 129 lines · Johannes Schindelin
On Thu, 28 Nov 2019, Johannes Sixt wrote:
[ ... analyzing a t0061.2 failure ...]
Quoted 125 lines
Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
Quoted 26 lines
compat/mingw.c +20
-2@@ -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); +compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1521
Quoted 10 lines
compat/mingw.c +10
-0+/* +* 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.compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1524
Quoted 3 lines
compat/mingw.c +3
-0+struct strbuf buf =STRBUF_INIT ; + + if (err != ERROR_NO_SYSTEM_RESOURCES &&Then this is true, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1535
Quoted 11 lines
compat/mingw.c +11
-0+/* +* 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, ...
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1536
Quoted 1 line
compat/mingw.c +1
-0+ !getenv("SUPPRESS_HANDLE_INHERITANCE_WARNING")) {... and the variable isn't set, so we continue here. (But I don't think it is important.)
Quoted 16 lines
compat/mingw.c +16
-0+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"compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1562
Quoted 10 lines
compat/mingw.c +10
-0+"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).Quoted 11 lines
compat/mingw.c +11
-0+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);compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1581
Quoted 10 lines
compat/mingw.c +3
-2+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.
Quoted 7 lines
Quoted 1 line
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.
Quoted 1 line
Will send a fix shortly.
Or not so shortly, after all ;-)
Ciao,
Dscho
Quoted 29 lines
Thanks,
DschoQuoted 24 lines
And why don't you observe the failure? A coincidence?
Quoted 11 lines
compat/mingw.c +1
-1- } +CloseHandle (pi.hThread);/* t/t0061-run-command.sh +0
-0t/t0061-run-command.sh cat >hello-script <<-EOF · line 18
Quoted 8 lines
t/t0061-run-command.sh +1
-1@@ -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
In reply to Johannes SchindelinAm 29.11.19 um 14:52 schrieb Johannes Schindelin:
Quoted 3 lines · Johannes Sixt
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).
Quoted 2 lines
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
In reply to Johannes SchindelinAm 29.11.19 um 14:52 schrieb Johannes Schindelin:
Quoted 6 lines · Johannes Sixt
Quoted 5 lines
Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1536
Quoted 1 line
compat/mingw.c +1
-0+ !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 ...
Quoted 13 lines
Quoted 13 lines
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1548
Quoted 13 lines
compat/mingw.c +12
-0+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.
Quoted 9 lines
Quoted 9 lines
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1557
Quoted 9 lines
compat/mingw.c +9
-0+ } +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
Hi Hannes,
Quoted 11 lines · Johannes Sixt
Am 29.11.19 um 14:52 schrieb Johannes Schindelin:
Quoted 6 lines · Johannes Sixt
Quoted 5 lines
Am 22.11.19 um 15:41 schrieb Johannes Schindelin via GitGitGadget:
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1536
Quoted 1 line
compat/mingw.c +1
-0+ !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 ...
Quoted 1 line
Quoted 1 line
Quoted 15 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
compat/mingw.c +4
-0+DWORD fl =0 ; +int i; + +setenv ("SUPPRESS_HANDLE_INHERITANCE_WARNING" ,"1" ,1 );Quoted 8 lines
Quoted 8 lines
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1548
Quoted 8 lines
compat/mingw.c +8
-0+ +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_vaddfneeds 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
Quoted 12 lines
Quoted 9 lines
Quoted 9 lines
compat/mingw.c static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen · line 1557
Quoted 9 lines
compat/mingw.c +9
-0+ } +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