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