Brown-bag fix on top of js/mingw-inherit-only-std-handles

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

From: Johannes Schindelin <[email protected]>

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.

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

index 2b6eca2f56..bb4eb4211a 100644
@@ -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

Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:

Copy-and-paste garbage?

Hi Hannes,

Yes :-(

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.

Thanks,
Dscho

Am 30.11.19 um 23:06 schrieb Johannes Schindelin:

Indeed, I build with NO_GETTEXT.

-- Hannes

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

From: Johannes Schindelin <[email protected]>

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.

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

index 2b6eca2f56..432adc1aed 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 compat/mingw.c | 1 +
 1 file changed, 1 insertion(+)

index 432adc1aed..8dc45808a3 100644
@@ -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

"Johannes Schindelin via GitGitGadget" <[email protected]>

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.

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.

Am 30.11.19 um 19:04 schrieb Junio C Hamano:

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.

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

POSIX guarantees that no library function sets errno to zero.

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

But it's still a good practice to follow.

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Hi Andreas,

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

OK, that makes sense.

Thanks.

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.

Hi Junio,

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

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.

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.

Hi Junio,

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.

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

Am 30.11.19 um 11:36 schrieb Johannes Schindelin via GitGitGadget:

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

-- Hannes

Thanks, both.

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.

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

From: Johannes Schindelin <[email protected]>

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.

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

index 2b6eca2f56..432adc1aed 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 compat/mingw.c | 1 +
 1 file changed, 1 insertion(+)

index 432adc1aed..827065d96d 100644
@@ -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

Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:

The subject line would have to be adjusted as well.

mingw: forbid translating ERROR_SUCCESS to an errno value

or something.

-- Hannes

True. Thanks for being careful.

Hi Hannes,

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