git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles
@ 2019-11-29 21:44 Johannes Schindelin via GitGitGadget
  2019-11-29 21:44 ` [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
  0 siblings, 2 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-11-29 21:44 UTC (permalink / raw)
  To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-11-29 21:44 [PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
@ 2019-11-29 21:44 ` Johannes Schindelin via GitGitGadget
  2019-11-29 23:02   ` Johannes Sixt
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
  1 sibling, 1 reply; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-11-29 21:44 UTC (permalink / raw)
  To: git
  Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-11-29 21:44 ` [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
@ 2019-11-29 23:02   ` Johannes Sixt
  2019-11-30 22:06     ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Johannes Sixt @ 2019-11-29 23:02 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-29 21:44 [PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
  2019-11-29 21:44 ` [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
@ 2019-11-30 10:36 ` Johannes Schindelin via GitGitGadget
  2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
                     ` (4 more replies)
  1 sibling, 5 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-11-30 10:36 UTC (permalink / raw)
  To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
@ 2019-11-30 10:36   ` Johannes Schindelin via GitGitGadget
  2019-11-30 10:36   ` [PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-11-30 10:36 UTC (permalink / raw)
  To: git
  Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
  2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
@ 2019-11-30 10:36   ` Johannes Schindelin via GitGitGadget
  2019-11-30 18:04   ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Junio C Hamano
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-11-30 10:36 UTC (permalink / raw)
  To: git
  Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
  2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
  2019-11-30 10:36   ` [PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
@ 2019-11-30 18:04   ` Junio C Hamano
  2019-11-30 19:13     ` Johannes Sixt
  2019-11-30 19:20   ` Johannes Sixt
  2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  4 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2019-11-30 18:04 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Sixt, Johannes Schindelin

"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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 18:04   ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Junio C Hamano
@ 2019-11-30 19:13     ` Johannes Sixt
  2019-11-30 20:11       ` Andreas Schwab
  2019-11-30 20:21       ` Junio C Hamano
  0 siblings, 2 replies; 27+ messages in thread
From: Johannes Sixt @ 2019-11-30 19:13 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
                     ` (2 preceding siblings ...)
  2019-11-30 18:04   ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Junio C Hamano
@ 2019-11-30 19:20   ` Johannes Sixt
  2019-11-30 22:09     ` Junio C Hamano
  2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  4 siblings, 1 reply; 27+ messages in thread
From: Johannes Sixt @ 2019-11-30 19:20 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 19:13     ` Johannes Sixt
@ 2019-11-30 20:11       ` Andreas Schwab
  2019-11-30 20:23         ` Junio C Hamano
  2019-11-30 20:21       ` Junio C Hamano
  1 sibling, 1 reply; 27+ messages in thread
From: Andreas Schwab @ 2019-11-30 20:11 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git,
	Johannes Schindelin

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."

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 19:13     ` Johannes Sixt
  2019-11-30 20:11       ` Andreas Schwab
@ 2019-11-30 20:21       ` Junio C Hamano
  2019-12-01  6:26         ` Junio C Hamano
  1 sibling, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2019-11-30 20:21 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 20:11       ` Andreas Schwab
@ 2019-11-30 20:23         ` Junio C Hamano
  2019-11-30 20:43           ` Andreas Schwab
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2019-11-30 20:23 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git,
	Johannes Schindelin

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...


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 20:23         ` Junio C Hamano
@ 2019-11-30 20:43           ` Andreas Schwab
  2019-11-30 21:22             ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Andreas Schwab @ 2019-11-30 20:43 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git,
	Johannes Schindelin

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."

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 20:43           ` Andreas Schwab
@ 2019-11-30 21:22             ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2019-11-30 21:22 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Junio C Hamano, Johannes Sixt, Johannes Schindelin via GitGitGadget, git

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-11-29 23:02   ` Johannes Sixt
@ 2019-11-30 22:06     ` Johannes Schindelin
  2019-11-30 22:16       ` Johannes Sixt
  0 siblings, 1 reply; 27+ messages in thread
From: Johannes Schindelin @ 2019-11-30 22:06 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 19:20   ` Johannes Sixt
@ 2019-11-30 22:09     ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2019-11-30 22:09 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-11-30 22:06     ` Johannes Schindelin
@ 2019-11-30 22:16       ` Johannes Sixt
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Sixt @ 2019-11-30 22:16 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 20:21       ` Junio C Hamano
@ 2019-12-01  6:26         ` Junio C Hamano
  2019-12-01  9:53           ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2019-12-01  6:26 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-01  6:26         ` Junio C Hamano
@ 2019-12-01  9:53           ` Johannes Schindelin
  2019-12-02  6:07             ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Johannes Schindelin @ 2019-12-01  9:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git

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.
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-01  9:53           ` Johannes Schindelin
@ 2019-12-02  6:07             ` Junio C Hamano
  2019-12-02  9:05               ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2019-12-02  6:07 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-02  6:07             ` Junio C Hamano
@ 2019-12-02  9:05               ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2019-12-02  9:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git

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.
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
                     ` (3 preceding siblings ...)
  2019-11-30 19:20   ` Johannes Sixt
@ 2019-12-02 11:33   ` Johannes Schindelin via GitGitGadget
  2019-12-02 11:33     ` [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
                       ` (2 more replies)
  4 siblings, 3 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-12-02 11:33 UTC (permalink / raw)
  To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance
  2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
@ 2019-12-02 11:33     ` Johannes Schindelin via GitGitGadget
  2019-12-02 11:33     ` [PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
  2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
  2 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-12-02 11:33 UTC (permalink / raw)
  To: git
  Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

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


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0
  2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  2019-12-02 11:33     ` [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
@ 2019-12-02 11:33     ` Johannes Schindelin via GitGitGadget
  2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
  2 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-12-02 11:33 UTC (permalink / raw)
  To: git
  Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
  2019-12-02 11:33     ` [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
  2019-12-02 11:33     ` [PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
@ 2019-12-02 17:35     ` Johannes Sixt
  2019-12-02 19:04       ` Junio C Hamano
  2019-12-03 12:04       ` Johannes Schindelin
  2 siblings, 2 replies; 27+ messages in thread
From: Johannes Sixt @ 2019-12-02 17:35 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
@ 2019-12-02 19:04       ` Junio C Hamano
  2019-12-03 12:04       ` Johannes Schindelin
  1 sibling, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2019-12-02 19:04 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

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.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
  2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
  2019-12-02 19:04       ` Junio C Hamano
@ 2019-12-03 12:04       ` Johannes Schindelin
  1 sibling, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2019-12-03 12:04 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2019-12-03 12:05 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 21:44 [PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
2019-11-29 21:44 ` [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-11-29 23:02   ` Johannes Sixt
2019-11-30 22:06     ` Johannes Schindelin
2019-11-30 22:16       ` Johannes Sixt
2019-11-30 10:36 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-11-30 10:36   ` [PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
2019-11-30 18:04   ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Junio C Hamano
2019-11-30 19:13     ` Johannes Sixt
2019-11-30 20:11       ` Andreas Schwab
2019-11-30 20:23         ` Junio C Hamano
2019-11-30 20:43           ` Andreas Schwab
2019-11-30 21:22             ` Johannes Schindelin
2019-11-30 20:21       ` Junio C Hamano
2019-12-01  6:26         ` Junio C Hamano
2019-12-01  9:53           ` Johannes Schindelin
2019-12-02  6:07             ` Junio C Hamano
2019-12-02  9:05               ` Johannes Schindelin
2019-11-30 19:20   ` Johannes Sixt
2019-11-30 22:09     ` Junio C Hamano
2019-12-02 11:33   ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2019-12-02 11:33     ` [PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance Johannes Schindelin via GitGitGadget
2019-12-02 11:33     ` [PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0 Johannes Schindelin via GitGitGadget
2019-12-02 17:35     ` [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Sixt
2019-12-02 19:04       ` Junio C Hamano
2019-12-03 12:04       ` Johannes Schindelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).