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
Johannes Schindelin via GitGitGadget
