[PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin via GitGitGadget]

Johannes "Hannes" Sixt pointed out that this patch series (which already
made it to next) introduces a problem: when falling back to spawning the
process without restricting the file handles, errno is not set accurately.

Sadly, the regression test failure observed by Hannes did not show up over
here, nor in the PR builds (or, for that matter, the literally hundreds of
CI builds that patch series underwent as part of Git for Windows' master 
branch). The cause was that errno is set to the expected ENOENT by another 
part of the code, too, that happens right before the calls to 
CreateProcessW(): the test whether a given path refers to a script that
needs to be executed via an interpreter (such as sh.exe) obviously needs to
open the file, and that obviously fails, setting errno = ENOENT!

I have currently no idea what function call could be responsible for
re-setting errno to the reported ERANGE... But at least over here, when I
partially apply this patch, the part that sets errno = 0;, t0061.2 fails for
me, too.

Johannes Schindelin (1):
  mingw: do set `errno` correctly when trying to restrict handle
    inheritance

 compat/mingw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


base-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/480
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <j6t@kdbg.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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());
 		if (ret && buf.len) {
-			errno = err_win_to_posix(GetLastError());
 			warning("failed to restrict file handles (%ld)\n\n%s",
 				err, buf.buf);
 		}
-- 
gitgitgadget

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

[Johannes Sixt]

Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <j6t@kdbg.org>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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

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

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
> >
> > 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 <j6t@kdbg.org>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  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

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

[Johannes Sixt]

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 v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin via GitGitGadget]

Johannes "Hannes" Sixt pointed out that this patch series (which already
made it to next) introduces a problem: when falling back to spawning the
process without restricting the file handles, errno is not set accurately.

Sadly, the regression test failure observed by Hannes did not show up over
here, nor in the PR builds (or, for that matter, the literally hundreds of
CI builds that patch series underwent as part of Git for Windows' master 
branch). The cause was that errno is set to the expected ENOENT by another 
part of the code, too, that happens right before the calls to 
CreateProcessW(): the test whether a given path refers to a script that
needs to be executed via an interpreter (such as sh.exe) obviously needs to
open the file, and that obviously fails, setting errno = ENOENT!

I have currently no idea what function call could be responsible for
re-setting errno to the reported ERANGE... But at least over here, when I
partially apply this patch, the part that sets errno = 0;, t0061.2 fails for
me, too.

Changes since v1:

 * A copy/edit fail in the commit message was fixed.
 * We now assign errno only when the call to CreateProcessW() failed.
 * For good measure, we teach the err_win_to_posix() function to translate 
   ERROR_SUCCESS into the errno value 0.

Johannes Schindelin (2):
  mingw: do set `errno` correctly when trying to restrict handle
    inheritance
  mingw: translate ERROR_SUCCESS to errno = 0

 compat/mingw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


base-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/480

Range-diff vs v1:

 1:  685360f735 ! 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
     @@ -28,7 +28,10 @@
          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.
     +    Let's fix that by making sure that `errno` is set correctly. It even
     +    appears that `errno` was set in the _wrong_ case previously:
     +    `CreateProcessW()` returns non-zero upon success, but `errno` was set
     +    only in the non-zero case.
      
          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,
     @@ -37,9 +40,8 @@
      
          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.
     +    tries to `open()` the file. 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
     @@ -66,9 +68,10 @@
       		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());
     +-		if (ret && buf.len) {
     ++		if (!ret)
     + 			errno = err_win_to_posix(GetLastError());
     ++		if (ret && buf.len) {
       			warning("failed to restrict file handles (%ld)\n\n%s",
       				err, buf.buf);
       		}
 -:  ---------- > 2:  c3dea00fb1 mingw: translate ERROR_SUCCESS to errno = 0

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 even
appears that `errno` was set in the _wrong_ case previously:
`CreateProcessW()` returns non-zero upon success, but `errno` was set
only in the non-zero case.

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. 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 <j6t@kdbg.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2b6eca2f56..432adc1aed 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,9 @@ 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);
-		if (ret && buf.len) {
+		if (!ret)
 			errno = err_win_to_posix(GetLastError());
+		if (ret && buf.len) {
 			warning("failed to restrict file handles (%ld)\n\n%s",
 				err, buf.buf);
 		}
-- 
gitgitgadget

[PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Johannes Sixt pointed out that the `err_win_to_posix()` function
mishandles `ERROR_SUCCESS`. This commit fixes that.

Technically, we try to only assign `errno` to the corresponding value of
`GetLastError()` (which translation is performed by that function) when
a Win32 API call failed, so this change is purely defensive and is not
expected to fix an existing bug in our code base.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 432adc1aed..8dc45808a3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr)
 	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
 	case ERROR_SHARING_VIOLATION: error = EACCES; break;
 	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
+	case ERROR_SUCCESS: error = 0; break;
 	case ERROR_SWAPERROR: error = ENOENT; break;
 	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
 	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
-- 
gitgitgadget

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  * We now assign errno only when the call to CreateProcessW() failed.

Meaning the global variable 'errno' is left as it was (instead of
getting cleared) when a system call succeeds?  That I think is the
correct behaviour people who use the variable expect.

>  * For good measure, we teach the err_win_to_posix() function to translate 
>    ERROR_SUCCESS into the errno value 0.

So, I am not sure if this is a good idea---who are the callers of
this function and why do they call it?  I would imagine that a
caller who makes a system call, upon seeing a failure from the
system call, calls this helper with the Windows error code it
received from the system call so that errno can be updated with a
POSIXy value.  If my imagination is correct, such a caller should
not be assigning anything to errno if the underlying system call
succeeds, i.e. returns ERROR_SUCCESS.  So a better solution might
be for the function to BUG() when fed ERROR_SUCCESS to point fingers
at the caller, no?

If my imagination is not correct, then ignore the whole paragraph
above ;-).

Thanks.

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Sixt]

Am 30.11.19 um 19:04 schrieb Junio C Hamano:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>>  * We now assign errno only when the call to CreateProcessW() failed.
> 
> Meaning the global variable 'errno' is left as it was (instead of
> getting cleared) when a system call succeeds?  That I think is the
> correct behaviour people who use the variable expect.

I hope you mean people who read the code. You cannot possibly mean
developers who expect that the run-command API keeps errno unchanged if
the calls were successful. I'm pretty sure they do not provide such a
guarantee.

> 
>>  * For good measure, we teach the err_win_to_posix() function to translate 
>>    ERROR_SUCCESS into the errno value 0.
> 
> So, I am not sure if this is a good idea---who are the callers of
> this function and why do they call it?  I would imagine that a
> caller who makes a system call, upon seeing a failure from the
> system call, calls this helper with the Windows error code it
> received from the system call so that errno can be updated with a
> POSIXy value.  If my imagination is correct, such a caller should
> not be assigning anything to errno if the underlying system call
> succeeds, i.e. returns ERROR_SUCCESS.  So a better solution might
> be for the function to BUG() when fed ERROR_SUCCESS to point fingers
> at the caller, no?

Just like on POSIX the value of errno is indeterminate after a
successful system call, the value of GetLastError() indeterminate after
a successful Windows API call. Therefore, the err_win_to_posix() would
not be able to point at a bogus caller reliably. For this reason, let's
consider the function as a simple error code translator, and then
translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.

-- Hannes

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Andreas Schwab]

On Nov 30 2019, Johannes Sixt wrote:

> Am 30.11.19 um 19:04 schrieb Junio C Hamano:
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>> 
>>>  * We now assign errno only when the call to CreateProcessW() failed.
>> 
>> Meaning the global variable 'errno' is left as it was (instead of
>> getting cleared) when a system call succeeds?  That I think is the
>> correct behaviour people who use the variable expect.
>
> I hope you mean people who read the code. You cannot possibly mean
> developers who expect that the run-command API keeps errno unchanged if
> the calls were successful. I'm pretty sure they do not provide such a
> guarantee.

POSIX guarantees that no library function sets errno to zero.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Nov 30 2019, Johannes Sixt wrote:
>
>> Am 30.11.19 um 19:04 schrieb Junio C Hamano:
>>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>>> writes:
>>> 
>>>>  * We now assign errno only when the call to CreateProcessW() failed.
>>> 
>>> Meaning the global variable 'errno' is left as it was (instead of
>>> getting cleared) when a system call succeeds?  That I think is the
>>> correct behaviour people who use the variable expect.
>>
>> I hope you mean people who read the code. You cannot possibly mean
>> developers who expect that the run-command API keeps errno unchanged if
>> the calls were successful. I'm pretty sure they do not provide such a
>> guarantee.
>
> POSIX guarantees that no library function sets errno to zero.

It is true, but the rest of the Git code works on top of an
abstraction a bit higher than C/POSIX library.  The run-command API
J6t cites is an example.

IOW, we cannot take advantage of that POSIX guarantee in the
codepaths that use our internal API.  So...

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Andreas Schwab]

On Nov 30 2019, Junio C Hamano wrote:

> IOW, we cannot take advantage of that POSIX guarantee in the
> codepaths that use our internal API.  So...

But it's still a good practice to follow.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin]

Hi Andreas,

On Sat, 30 Nov 2019, Andreas Schwab wrote:

> On Nov 30 2019, Junio C Hamano wrote:
>
> > IOW, we cannot take advantage of that POSIX guarantee in the
> > codepaths that use our internal API.  So...
>
> But it's still a good practice to follow.

An even better practice is to focus on what's important.

You could, for example, review the original patches again that needed this
fixup, to make sure that nothing similar lurks in them. That would seem to
me certainly more meaningful than harping on POSIX semantics in a context
where they do not actually matter.

Ciao,
Johannes

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Just like on POSIX the value of errno is indeterminate after a
> successful system call, the value of GetLastError() indeterminate after
> a successful Windows API call. Therefore, the err_win_to_posix() would
> not be able to point at a bogus caller reliably. For this reason, let's
> consider the function as a simple error code translator, and then
> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.

OK, that makes sense.

Thanks.

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Just like on POSIX the value of errno is indeterminate after a
>> successful system call, the value of GetLastError() indeterminate after
>> a successful Windows API call. Therefore, the err_win_to_posix() would
>> not be able to point at a bogus caller reliably. For this reason, let's
>> consider the function as a simple error code translator, and then
>> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.
>
> OK, that makes sense.

Actually, I do not think it makes that much sense, especially in the
context of this patch that claims to map success to 0 (assuming that
no E_ANYTHING has the value of zero---I am not sure POSIX gives such
a guarantee) "for good measure".

Even if Windows API makes the GetLastError() unusable after a
success call (unlike POSIX, where errno is left alone), the API
calls themselves would be signaling their own success or failure,
right?  So I would have imagined that any kosher caller would be
doing this:

	if (SomeWinAPI() != SUCCESS) {
		errno = err_win_to_posix(GetLastError());
		... possibly other reactions to the error ...
	}

If using a value grabbed from GetLastError() after a successfull
Windows API call is a wrong thing to do as you taught us in your
message, then an unconditional

	SomeWinAPI();
	errno = err_win_to_posix(GetLastError());

would be wrong anyway, and touching errno unconditionally, when the
previous call may have succeeded without checking, makes the pattern
doubly wrong, no?

Having said that, I do not expect myself to be looking into and
fixing anything in compat/*win*.[ch], so I do not care too deeply
either way, but I thought that it would help keep the sanity of
developers involved if we touched errno only upon a failure from an
underlying system.

Thanks.

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin]

Hi Junio,

On Sat, 30 Nov 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Johannes Sixt <j6t@kdbg.org> writes:
> >
> >> Just like on POSIX the value of errno is indeterminate after a
> >> successful system call, the value of GetLastError() indeterminate after
> >> a successful Windows API call. Therefore, the err_win_to_posix() would
> >> not be able to point at a bogus caller reliably. For this reason, let's
> >> consider the function as a simple error code translator, and then
> >> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.
> >
> > OK, that makes sense.
>
> Actually, I do not think it makes that much sense, especially in the
> context of this patch that claims to map success to 0 (assuming that
> no E_ANYTHING has the value of zero---I am not sure POSIX gives such
> a guarantee) "for good measure".

But the intention is not to help correct callers! The purpose of mapping
`ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error
when none happened. On Windows, which is the only platform where the
discussed function is compiled and used, `strerror(0)` has the
well-established behavior of returning "No error".

Now imagine that a user encounters a bug where a code path does indeed
incorrectly report an error where no error occurred (or where Git is
looking for the error too late or something like that). You probably agree
with me that we should really avoid reporting "Function not implemented"
in such a case and rather say "No error", which will help figure out the
bug much quicker.

And that is what this patch is all about.

Ciao,
Dscho

> Even if Windows API makes the GetLastError() unusable after a
> success call (unlike POSIX, where errno is left alone), the API
> calls themselves would be signaling their own success or failure,
> right?  So I would have imagined that any kosher caller would be
> doing this:
>
> 	if (SomeWinAPI() != SUCCESS) {
> 		errno = err_win_to_posix(GetLastError());
> 		... possibly other reactions to the error ...
> 	}
>
> If using a value grabbed from GetLastError() after a successfull
> Windows API call is a wrong thing to do as you taught us in your
> message, then an unconditional
>
> 	SomeWinAPI();
> 	errno = err_win_to_posix(GetLastError());
>
> would be wrong anyway, and touching errno unconditionally, when the
> previous call may have succeeded without checking, makes the pattern
> doubly wrong, no?
>
> Having said that, I do not expect myself to be looking into and
> fixing anything in compat/*win*.[ch], so I do not care too deeply
> either way, but I thought that it would help keep the sanity of
> developers involved if we touched errno only upon a failure from an
> underlying system.
>
> Thanks.
>

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> But the intention is not to help correct callers! The purpose of mapping
> `ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error
> when none happened. On Windows, which is the only platform where the
> discussed function is compiled and used, `strerror(0)` has the
> well-established behavior of returning "No error".

But is the caller that ends up saying "No error" you mention below
by calling strerror() exists *only* on Windows port somewhere in
compat/ directory?  If the call to strerror() is made from a
codepath that is common to all platforms, then that caller *is*
wrong to do so after seeing *no* errors.

So, no, sorry, but I do not understand the above line of reasoning.

> Now imagine that a user encounters a bug where a code path does indeed
> incorrectly report an error where no error occurred (or where Git is
> looking for the error too late or something like that). You probably agree
> with me that we should really avoid reporting "Function not implemented"
> in such a case and rather say "No error", which will help figure out the
> bug much quicker.

Not really.  A BUG() that catches a call to this function that
should not be called when there is no error would help identify the
problematic caller a lot faster by being louder---those users who
get hit will raise their hands more readily than those who see "No
error" and wonders "why does Git give that error message when there
is no error?".  IOW, "No error" sounds like sweeping the issue under
the rug.

The argument could be "I do not want to be pressured to hunt for the
problematic caller, and taking the approach to be less loud than the
BUG() approach would allow the end users keep operating as if
nothing bad happened, so that I can hunt for the caller iff/when I
feel like it, and with no urgent need I can just ignore the hunt
altogether.  Not hitting with BUG() is a service to the end users
who shouldn't be made a guinea pig---that is why we want to sweep
the problematic caller under the rug.  It may delay the discovery
and fixing of problematic callers, which is a downside, though."

I would understand such an argument, even if I might not think that
such an argument makes the right tradeoff between not hitting the
users with BUG() and not being effective in finding the problematic
caller.

Thanks.

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin]

Hi Junio,

On Sun, 1 Dec 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > But the intention is not to help correct callers! The purpose of mapping
> > `ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error
> > when none happened. On Windows, which is the only platform where the
> > discussed function is compiled and used, `strerror(0)` has the
> > well-established behavior of returning "No error".
>
> But is the caller that ends up saying "No error" you mention below
> by calling strerror() exists *only* on Windows port somewhere in
> compat/ directory?  If the call to strerror() is made from a
> codepath that is common to all platforms, then that caller *is*
> wrong to do so after seeing *no* errors.

Yes, that would be a difference between platforms, but _at least_ the
Windows side would help diagnose the bug better. I'd prefer such a half
solution to having no solution at all.

> So, no, sorry, but I do not understand the above line of reasoning.
>
> > Now imagine that a user encounters a bug where a code path does indeed
> > incorrectly report an error where no error occurred (or where Git is
> > looking for the error too late or something like that). You probably agree
> > with me that we should really avoid reporting "Function not implemented"
> > in such a case and rather say "No error", which will help figure out the
> > bug much quicker.
>
> Not really.  A BUG() that catches a call to this function that
> should not be called when there is no error would help identify the
> problematic caller a lot faster by being louder---those users who
> get hit will raise their hands more readily than those who see "No
> error" and wonders "why does Git give that error message when there
> is no error?".  IOW, "No error" sounds like sweeping the issue under
> the rug.

This is a very good point. It _is_ a bug to hit that code path when
`GetLastError()` returned `ERROR_SUCCESS`, and having such a `BUG()` call
would have prevented the bug that I fix in 1/2 from slipping through.

Therefore, I changed 2/2 to raise a BUG() and am currently waiting for the
PR build to finish before sending another iteration of this seris.

Ciao,
Dscho

> The argument could be "I do not want to be pressured to hunt for the
> problematic caller, and taking the approach to be less loud than the
> BUG() approach would allow the end users keep operating as if
> nothing bad happened, so that I can hunt for the caller iff/when I
> feel like it, and with no urgent need I can just ignore the hunt
> altogether.  Not hitting with BUG() is a service to the end users
> who shouldn't be made a guinea pig---that is why we want to sweep
> the problematic caller under the rug.  It may delay the discovery
> and fixing of problematic callers, which is a downside, though."
>
> I would understand such an argument, even if I might not think that
> such an argument makes the right tradeoff between not hitting the
> users with BUG() and not being effective in finding the problematic
> caller.
>
> Thanks.
>
>

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Sixt]

Am 30.11.19 um 11:36 schrieb Johannes Schindelin via GitGitGadget:
> Johannes "Hannes" Sixt pointed out that this patch series (which already
> made it to next) introduces a problem: when falling back to spawning the
> process without restricting the file handles, errno is not set accurately.
> 
> Sadly, the regression test failure observed by Hannes did not show up over
> here, nor in the PR builds (or, for that matter, the literally hundreds of
> CI builds that patch series underwent as part of Git for Windows' master 
> branch). The cause was that errno is set to the expected ENOENT by another 
> part of the code, too, that happens right before the calls to 
> CreateProcessW(): the test whether a given path refers to a script that
> needs to be executed via an interpreter (such as sh.exe) obviously needs to
> open the file, and that obviously fails, setting errno = ENOENT!
> 
> I have currently no idea what function call could be responsible for
> re-setting errno to the reported ERANGE... But at least over here, when I
> partially apply this patch, the part that sets errno = 0;, t0061.2 fails for
> me, too.
> 
> Changes since v1:
> 
>  * A copy/edit fail in the commit message was fixed.
>  * We now assign errno only when the call to CreateProcessW() failed.
>  * For good measure, we teach the err_win_to_posix() function to translate 
>    ERROR_SUCCESS into the errno value 0.

This series fixes the observed failure. The changes look good. Thank you
for the quick turn-around.

Tested-by: Johannes Sixt <j6t@kdbg.org>

> 
> Johannes Schindelin (2):
>   mingw: do set `errno` correctly when trying to restrict handle
>     inheritance
>   mingw: translate ERROR_SUCCESS to errno = 0
> 
>  compat/mingw.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> 
> base-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/480
> 
> Range-diff vs v1:
> 
>  1:  685360f735 ! 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
>      @@ -28,7 +28,10 @@
>           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.
>      +    Let's fix that by making sure that `errno` is set correctly. It even
>      +    appears that `errno` was set in the _wrong_ case previously:
>      +    `CreateProcessW()` returns non-zero upon success, but `errno` was set
>      +    only in the non-zero case.
>       
>           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,
>      @@ -37,9 +40,8 @@
>       
>           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.
>      +    tries to `open()` the file. 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
>      @@ -66,9 +68,10 @@
>        		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());
>      +-		if (ret && buf.len) {
>      ++		if (!ret)
>      + 			errno = err_win_to_posix(GetLastError());
>      ++		if (ret && buf.len) {
>        			warning("failed to restrict file handles (%ld)\n\n%s",
>        				err, buf.buf);
>        		}
>  -:  ---------- > 2:  c3dea00fb1 mingw: translate ERROR_SUCCESS to errno = 0
> 

-- Hannes

Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> This series fixes the observed failure. The changes look good. Thank you
> for the quick turn-around.
>
> Tested-by: Johannes Sixt <j6t@kdbg.org>

Thanks, both.

[PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin via GitGitGadget]

Johannes "Hannes" Sixt pointed out that this patch series (which already
made it to next) introduces a problem: when falling back to spawning the
process without restricting the file handles, errno is not set accurately.

Sadly, the regression test failure observed by Hannes did not show up over
here, nor in the PR builds (or, for that matter, the literally hundreds of
CI builds that patch series underwent as part of Git for Windows' master 
branch). The cause was that errno is set to the expected ENOENT by another 
part of the code, too, that happens right before the calls to 
CreateProcessW(): the test whether a given path refers to a script that
needs to be executed via an interpreter (such as sh.exe) obviously needs to
open the file, and that obviously fails, setting errno = ENOENT!

I have currently no idea what function call could be responsible for
re-setting errno to the reported ERANGE... But at least over here, when I
partially apply this patch, the part that sets errno = 0;, t0061.2 fails for
me, too.

Change since v2:

 * We no longer map ERROR_SUCCESS to 0, but instead raise a bug message when
   the code is asked to handle the "No error" error.

Changes since v1:

 * A copy/edit fail in the commit message was fixed.
 * We now assign errno only when the call to CreateProcessW() failed.
 * For good measure, we teach the err_win_to_posix() function to translate 
   ERROR_SUCCESS into the errno value 0.

Johannes Schindelin (2):
  mingw: do set `errno` correctly when trying to restrict handle
    inheritance
  mingw: translate ERROR_SUCCESS to errno = 0

 compat/mingw.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


base-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/480

Range-diff vs v2:

 1:  280b6d08a4 = 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
 2:  c3dea00fb1 ! 2:  e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0
     @@ -3,12 +3,15 @@
          mingw: translate ERROR_SUCCESS to errno = 0
      
          Johannes Sixt pointed out that the `err_win_to_posix()` function
     -    mishandles `ERROR_SUCCESS`. This commit fixes that.
     +    mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.
      
     -    Technically, we try to only assign `errno` to the corresponding value of
     -    `GetLastError()` (which translation is performed by that function) when
     -    a Win32 API call failed, so this change is purely defensive and is not
     -    expected to fix an existing bug in our code base.
     +    The only purpose of this function is to map Win32 API errors to `errno`
     +    ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea
     +    of `errno` is that it will only be set in case of an error, and left
     +    alone in case of success.
     +
     +    Therefore, as pointed out by Junio Hamano, it is a bug to call this
     +    function when there was not even any error to map.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -19,7 +22,7 @@
       	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
       	case ERROR_SHARING_VIOLATION: error = EACCES; break;
       	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
     -+	case ERROR_SUCCESS: error = 0; break;
     ++	case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!");
       	case ERROR_SWAPERROR: error = ENOENT; break;
       	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
       	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;

-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 even
appears that `errno` was set in the _wrong_ case previously:
`CreateProcessW()` returns non-zero upon success, but `errno` was set
only in the non-zero case.

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. 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 <j6t@kdbg.org>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2b6eca2f56..432adc1aed 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,9 @@ 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);
-		if (ret && buf.len) {
+		if (!ret)
 			errno = err_win_to_posix(GetLastError());
+		if (ret && buf.len) {
 			warning("failed to restrict file handles (%ld)\n\n%s",
 				err, buf.buf);
 		}
-- 
gitgitgadget

[PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Johannes Sixt pointed out that the `err_win_to_posix()` function
mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.

The only purpose of this function is to map Win32 API errors to `errno`
ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea
of `errno` is that it will only be set in case of an error, and left
alone in case of success.

Therefore, as pointed out by Junio Hamano, it is a bug to call this
function when there was not even any error to map.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 432adc1aed..827065d96d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr)
 	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
 	case ERROR_SHARING_VIOLATION: error = EACCES; break;
 	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
+	case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!");
 	case ERROR_SWAPERROR: error = ENOENT; break;
 	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
 	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
-- 
gitgitgadget

Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Sixt]

Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:
> Range-diff vs v2:
> 
>  1:  280b6d08a4 = 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
>  2:  c3dea00fb1 ! 2:  e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0
>      @@ -3,12 +3,15 @@
>           mingw: translate ERROR_SUCCESS to errno = 0

The subject line would have to be adjusted as well.

	mingw: forbid translating ERROR_SUCCESS to an errno value

or something.

>       
>           Johannes Sixt pointed out that the `err_win_to_posix()` function
>      -    mishandles `ERROR_SUCCESS`. This commit fixes that.
>      +    mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.
>       
>      -    Technically, we try to only assign `errno` to the corresponding value of
>      -    `GetLastError()` (which translation is performed by that function) when
>      -    a Win32 API call failed, so this change is purely defensive and is not
>      -    expected to fix an existing bug in our code base.
>      +    The only purpose of this function is to map Win32 API errors to `errno`
>      +    ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea
>      +    of `errno` is that it will only be set in case of an error, and left
>      +    alone in case of success.
>      +
>      +    Therefore, as pointed out by Junio Hamano, it is a bug to call this
>      +    function when there was not even any error to map.
>       
>           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>       
>      @@ -19,7 +22,7 @@
>        	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
>        	case ERROR_SHARING_VIOLATION: error = EACCES; break;
>        	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
>      -+	case ERROR_SUCCESS: error = 0; break;
>      ++	case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!");
>        	case ERROR_SWAPERROR: error = ENOENT; break;
>        	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
>        	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
> 

-- Hannes

Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:
>> Range-diff vs v2:
>> 
>>  1:  280b6d08a4 = 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
>>  2:  c3dea00fb1 ! 2:  e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0
>>      @@ -3,12 +3,15 @@
>>           mingw: translate ERROR_SUCCESS to errno = 0
>
> The subject line would have to be adjusted as well.
>
> 	mingw: forbid translating ERROR_SUCCESS to an errno value
>
> or something.

True.  Thanks for being careful.

Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles

[Johannes Schindelin]

Hi Hannes,

On Mon, 2 Dec 2019, Johannes Sixt wrote:

> Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:
> > Range-diff vs v2:
> >
> >  1:  280b6d08a4 = 1:  280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance
> >  2:  c3dea00fb1 ! 2:  e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0
> >      @@ -3,12 +3,15 @@
> >           mingw: translate ERROR_SUCCESS to errno = 0
>
> The subject line would have to be adjusted as well.
>
> 	mingw: forbid translating ERROR_SUCCESS to an errno value
>
> or something.

Oy. Thank you for catching this! (It is always amazing to me how much I
miss when I have stared at the same commits for a while.)

For good measure, I force-pushed the branch of the PR to match what Junio
has in `pu`, but if there are no other changes I need to make, I will
refrain from submitting another iteration.

Thanks,
Dscho

>
> >
> >           Johannes Sixt pointed out that the `err_win_to_posix()` function
> >      -    mishandles `ERROR_SUCCESS`. This commit fixes that.
> >      +    mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.
> >
> >      -    Technically, we try to only assign `errno` to the corresponding value of
> >      -    `GetLastError()` (which translation is performed by that function) when
> >      -    a Win32 API call failed, so this change is purely defensive and is not
> >      -    expected to fix an existing bug in our code base.
> >      +    The only purpose of this function is to map Win32 API errors to `errno`
> >      +    ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea
> >      +    of `errno` is that it will only be set in case of an error, and left
> >      +    alone in case of success.
> >      +
> >      +    Therefore, as pointed out by Junio Hamano, it is a bug to call this
> >      +    function when there was not even any error to map.
> >
> >           Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> >      @@ -19,7 +22,7 @@
> >        	case ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;
> >        	case ERROR_SHARING_VIOLATION: error = EACCES; break;
> >        	case ERROR_STACK_OVERFLOW: error = ENOMEM; break;
> >      -+	case ERROR_SUCCESS: error = 0; break;
> >      ++	case ERROR_SUCCESS: BUG("err_win_to_posix() called without an error!");
> >        	case ERROR_SWAPERROR: error = ENOENT; break;
> >        	case ERROR_TOO_MANY_MODULES: error = EMFILE; break;
> >        	case ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;
> >
>
> -- Hannes
>