From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Johannes Sixt <j6t@kdbg.org>,
Johannes Schindelin <johannes.schindelin@gmx.de>,
Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
Date: Sat, 30 Nov 2019 10:36:38 +0000 [thread overview]
Message-ID: <pull.480.v2.git.1575110200.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.480.git.1575063876.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2019-11-30 10:36 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Johannes Schindelin via GitGitGadget [this message]
2019-11-30 10:36 ` [PATCH v2 1/2] " 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.480.v2.git.1575110200.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=johannes.schindelin@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).