git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
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
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

  parent reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-29 21:44 [PATCH 0/1] " 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 publically 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

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git