[1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance

Commit Message

Linus Arver via GitGitGadget Nov. 29, 2019, 9:44 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In 9a780a384de (mingw: spawned processes need to inherit only standard
handles, 2019-11-22), we taught the Windows-specific part to restrict
which file handles are passed on to the spawned processes.

Since this logic seemed to be a bit fragile across Windows versions (we
_still_ support Windows Vista in Git for Windows, for example), a
fall-back was added to try spawning the process again, this time without
restricting which file handles are to be inherited by the spawned
process.

In the common case (i.e. when the process could not be spawned for
reasons _other_ than the file handle inheritance), the fall-back attempt
would still fail, of course.

Crucially, one thing we missed in that code path was to set `errno`
appropriately.

This should have been caught by t0061.2 which expected `errno` to be
`ENOENT` after trying to start a process for a non-existing executable,
but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:
while looking for the config settings for trace2, Git tries to access
`xdg_config` and `user_config` via `access_or_die()`, and as neither of
those config files exists when running the test case (because in Git's
test suite, `HOME` points to the test directory), the `errno` has the
expected value, but for the wrong reasons.

Let's fix that by making sure that `errno` is set correctly.

It would be nice if we could somehow fix t0061 to make sure that this
does not regress again. One approach that seemed like it should work,
but did not, was to set `errno` to 0 in the test helper that is used by
t0061.2.

However, 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.0/compat/mingw.c#L1134. Obviously,
this call fails, and sets `errno` to `ENOENT`, deep inside the call
chain started from that test helper.

Instead, we force re-set `errno` at the beginning of the function
`mingw_spawnve_fd()`, which _should_ be safe given that callers of that
function will want to look at `errno` if -1 was returned. And if that
`errno` is 0 ("No error"), regression tests like t0061.2 will kick in.

Reported-by: Johannes Sixt <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 compat/mingw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Johannes Sixt Nov. 29, 2019, 11:02 p.m. UTC | #1

Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <[e-mail]>
> 
> In 9a780a384de (mingw: spawned processes need to inherit only standard
> handles, 2019-11-22), we taught the Windows-specific part to restrict
> which file handles are passed on to the spawned processes.
> 
> Since this logic seemed to be a bit fragile across Windows versions (we
> _still_ support Windows Vista in Git for Windows, for example), a
> fall-back was added to try spawning the process again, this time without
> restricting which file handles are to be inherited by the spawned
> process.
> 
> In the common case (i.e. when the process could not be spawned for
> reasons _other_ than the file handle inheritance), the fall-back attempt
> would still fail, of course.
> 
> Crucially, one thing we missed in that code path was to set `errno`
> appropriately.
> 
> This should have been caught by t0061.2 which expected `errno` to be
> `ENOENT` after trying to start a process for a non-existing executable,
> but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:
> while looking for the config settings for trace2, Git tries to access
> `xdg_config` and `user_config` via `access_or_die()`, and as neither of
> those config files exists when running the test case (because in Git's
> test suite, `HOME` points to the test directory), the `errno` has the
> expected value, but for the wrong reasons.
> 
> Let's fix that by making sure that `errno` is set correctly.
> 
> It would be nice if we could somehow fix t0061 to make sure that this
> does not regress again. One approach that seemed like it should work,
> but did not, was to set `errno` to 0 in the test helper that is used by
> t0061.2.
> 
> However, 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.0/compat/mingw.c#L1134. Obviously,

Copy-and-paste garbage?

> this call fails, and sets `errno` to `ENOENT`, deep inside the call
> chain started from that test helper.
> 
> Instead, we force re-set `errno` at the beginning of the function
> `mingw_spawnve_fd()`, which _should_ be safe given that callers of that
> function will want to look at `errno` if -1 was returned. And if that
> `errno` is 0 ("No error"), regression tests like t0061.2 will kick in.
> 
> Reported-by: Johannes Sixt <[e-mail]>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  compat/mingw.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 2b6eca2f56..bb4eb4211a 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
>  	const char *(*quote_arg)(const char *arg) =
>  		is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;
>  
> +	/* Make sure to override previous errors, if any */
> +	errno = 0;
> +
>  	if (restrict_handle_inheritance < 0)
>  		restrict_handle_inheritance = core_restrict_inherited_handles;
>  	/*
> @@ -1580,8 +1583,8 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
>  		ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,
>  				     TRUE, flags, wenvblk, dir ? wdir : NULL,
>  				     &si.StartupInfo, &pi);
> +		errno = err_win_to_posix(GetLastError());

I think this should be protected by 'if (!ret)' because
err_win_to_posix() does not handle ERROR_SUCCESS and turns it into
ENOSYS. It's not that bad because in the case of success we do not
guarantee any value of errno anyway.

>  		if (ret && buf.len) {
> -			errno = err_win_to_posix(GetLastError());
>  			warning("failed to restrict file handles (%ld)\n\n%s",
>  				err, buf.buf);
>  		}
> 

That said, this fixes the failure.

-- Hannes

Johannes Schindelin Nov. 30, 2019, 10:06 p.m. UTC | #2

Hi Hannes,

On Sat, 30 Nov 2019, Johannes Sixt wrote:

> Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:
> > From: Johannes Schindelin <[e-mail]>
> >
> > In 9a780a384de (mingw: spawned processes need to inherit only standard
> > handles, 2019-11-22), we taught the Windows-specific part to restrict
> > which file handles are passed on to the spawned processes.
> >
> > Since this logic seemed to be a bit fragile across Windows versions (we
> > _still_ support Windows Vista in Git for Windows, for example), a
> > fall-back was added to try spawning the process again, this time without
> > restricting which file handles are to be inherited by the spawned
> > process.
> >
> > In the common case (i.e. when the process could not be spawned for
> > reasons _other_ than the file handle inheritance), the fall-back attempt
> > would still fail, of course.
> >
> > Crucially, one thing we missed in that code path was to set `errno`
> > appropriately.
> >
> > This should have been caught by t0061.2 which expected `errno` to be
> > `ENOENT` after trying to start a process for a non-existing executable,
> > but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:
> > while looking for the config settings for trace2, Git tries to access
> > `xdg_config` and `user_config` via `access_or_die()`, and as neither of
> > those config files exists when running the test case (because in Git's
> > test suite, `HOME` points to the test directory), the `errno` has the
> > expected value, but for the wrong reasons.
> >
> > Let's fix that by making sure that `errno` is set correctly.
> >
> > It would be nice if we could somehow fix t0061 to make sure that this
> > does not regress again. One approach that seemed like it should work,
> > but did not, was to set `errno` to 0 in the test helper that is used by
> > t0061.2.
> >
> > However, 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.0/compat/mingw.c#L1134. Obviously,
>
> Copy-and-paste garbage?

Yes :-(

> > this call fails, and sets `errno` to `ENOENT`, deep inside the call
> > chain started from that test helper.
> >
> > Instead, we force re-set `errno` at the beginning of the function
> > `mingw_spawnve_fd()`, which _should_ be safe given that callers of that
> > function will want to look at `errno` if -1 was returned. And if that
> > `errno` is 0 ("No error"), regression tests like t0061.2 will kick in.
> >
> > Reported-by: Johannes Sixt <[e-mail]>
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >  compat/mingw.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 2b6eca2f56..bb4eb4211a 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
> >  	const char *(*quote_arg)(const char *arg) =
> >  		is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;
> >
> > +	/* Make sure to override previous errors, if any */
> > +	errno = 0;
> > +
> >  	if (restrict_handle_inheritance < 0)
> >  		restrict_handle_inheritance = core_restrict_inherited_handles;
> >  	/*
> > @@ -1580,8 +1583,8 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
> >  		ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,
> >  				     TRUE, flags, wenvblk, dir ? wdir : NULL,
> >  				     &si.StartupInfo, &pi);
> > +		errno = err_win_to_posix(GetLastError());
>
> I think this should be protected by 'if (!ret)' because
> err_win_to_posix() does not handle ERROR_SUCCESS and turns it into
> ENOSYS. It's not that bad because in the case of success we do not
> guarantee any value of errno anyway.

Ah, that's the reason!

Are you building with `NO_GETTEXT` perchance? I ask because gettext
overrides `vsnprintf()` with their own version, which is obviously quite
different from the version MSVCRT provides... and the former version is
what I use, and what all those CI/PR builds use.

> >  		if (ret && buf.len) {
> > -			errno = err_win_to_posix(GetLastError());
> >  			warning("failed to restrict file handles (%ld)\n\n%s",
> >  				err, buf.buf);
> >  		}
> >
>
> That said, this fixes the failure.

Thanks,
Dscho

Johannes Sixt Nov. 30, 2019, 10:16 p.m. UTC | #3

Am 30.11.19 um 23:06 schrieb Johannes Schindelin:
> Are you building with `NO_GETTEXT` perchance? I ask because gettext
> overrides `vsnprintf()` with their own version, which is obviously quite
> different from the version MSVCRT provides... and the former version is
> what I use, and what all those CI/PR builds use.

Indeed, I build with NO_GETTEXT.

-- Hannes

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 2b6eca2f56..bb4eb4211a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1423,6 +1423,9 @@  static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 	const char *(*quote_arg)(const char *arg) =
 		is_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;
 
+	/* Make sure to override previous errors, if any */
+	errno = 0;
+
 	if (restrict_handle_inheritance < 0)
 		restrict_handle_inheritance = core_restrict_inherited_handles;
 	/*
@@ -1580,8 +1583,8 @@  static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen
 		ret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,
 				     TRUE, flags, wenvblk, dir ? wdir : NULL,
 				     &si.StartupInfo, &pi);
+		errno = err_win_to_posix(GetLastError());
 		if (ret && buf.len) {
-			errno = err_win_to_posix(GetLastError());
 			warning("failed to restrict file handles (%ld)\n\n%s",
 				err, buf.buf);
 		}