Git Mailing List Archive on lore.kernel.org
 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 v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles
Date: Mon, 02 Dec 2019 11:33:27 +0000
Message-ID: <pull.480.v3.git.1575286409.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.480.v2.git.1575110200.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.

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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -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

  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 ` [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles Johannes Schindelin via GitGitGadget
2019-11-30 10:36   ` [PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance 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   ` Johannes Schindelin via GitGitGadget [this message]
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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.480.v3.git.1575286409.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 Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


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