{"thread":{"id":"52372","subject":"[PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles","startedAt":"2019-11-29T21:44:41Z","lastAt":"2019-12-03T12:05:08Z","messageCount":27,"participants":["Johannes Schindelin via GitGitGadget","Johannes Sixt","Junio C Hamano","Andreas Schwab","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"387320","messageId":"pull.480.git.1575063876.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":null,"subject":"[PATCH 0/1] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-29T21:44:35Z","receivedAt":"2019-11-29T21:44:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Johannes \"Hannes\" Sixt pointed out that this patch series (which already\nmade it to next) introduces a problem: when falling back to spawning the\nprocess without restricting the file handles, errno is not set accurately.\n\nSadly, the regression test failure observed by Hannes did not show up over\nhere, nor in the PR builds (or, for that matter, the literally hundreds of\nCI builds that patch series underwent as part of Git for Windows' master \nbranch). The cause was that errno is set to the expected ENOENT by another \npart of the code, too, that happens right before the calls to \nCreateProcessW(): the test whether a given path refers to a script that\nneeds to be executed via an interpreter (such as sh.exe) obviously needs to\nopen the file, and that obviously fails, setting errno = ENOENT!\n\nI have currently no idea what function call could be responsible for\nre-setting errno to the reported ERANGE... But at least over here, when I\npartially apply this patch, the part that sets errno = 0;, t0061.2 fails for\nme, too.\n\nJohannes Schindelin (1):\n mingw: do set `errno` correctly when trying to restrict handle\n inheritance\n\n compat/mingw.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\n\nbase-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/480\n-- \ngitgitgadget\n"},{"id":"387321","messageId":"685360f735e35e837bc9ef684cbde33564c81666.1575063876.git.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.git.1575063876.gitgitgadget@gmail.com","subject":"[PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-29T21:44:36Z","receivedAt":"2019-11-29T21:44:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 9a780a384de (mingw: spawned processes need to inherit only standard\nhandles, 2019-11-22), we taught the Windows-specific part to restrict\nwhich file handles are passed on to the spawned processes.\n\nSince this logic seemed to be a bit fragile across Windows versions (we\n_still_ support Windows Vista in Git for Windows, for example), a\nfall-back was added to try spawning the process again, this time without\nrestricting which file handles are to be inherited by the spawned\nprocess.\n\nIn the common case (i.e. when the process could not be spawned for\nreasons _other_ than the file handle inheritance), the fall-back attempt\nwould still fail, of course.\n\nCrucially, one thing we missed in that code path was to set `errno`\nappropriately.\n\nThis should have been caught by t0061.2 which expected `errno` to be\n`ENOENT` after trying to start a process for a non-existing executable,\nbut `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:\nwhile looking for the config settings for trace2, Git tries to access\n`xdg_config` and `user_config` via `access_or_die()`, and as neither of\nthose config files exists when running the test case (because in Git's\ntest suite, `HOME` points to the test directory), the `errno` has the\nexpected value, but for the wrong reasons.\n\nLet's fix that by making sure that `errno` is set correctly.\n\nIt would be nice if we could somehow fix t0061 to make sure that this\ndoes not regress again. One approach that seemed like it should work,\nbut did not, was to set `errno` to 0 in the test helper that is used by\nt0061.2.\n\nHowever, when `mingw_spawnvpe()` wants to see whether the file in\nquestion is a script, it calls `parse_interpreter()`, which in turn\ntries to `open()` the file.0/compat/mingw.c#L1134. Obviously,\nthis call fails, and sets `errno` to `ENOENT`, deep inside the call\nchain started from that test helper.\n\nInstead, we force re-set `errno` at the beginning of the function\n`mingw_spawnve_fd()`, which _should_ be safe given that callers of that\nfunction will want to look at `errno` if -1 was returned. And if that\n`errno` is 0 (\"No error\"), regression tests like t0061.2 will kick in.\n\nReported-by: Johannes Sixt \nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2b6eca2f56..bb4eb4211a 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \tconst char *(*quote_arg)(const char *arg) =\n \t\tis_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;\n \n+\t/* Make sure to override previous errors, if any */\n+\terrno = 0;\n+\n \tif (restrict_handle_inheritance < 0)\n \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n \t/*\n@@ -1580,8 +1583,8 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n \t\t\t\t &si.StartupInfo, &pi);\n+\t\terrno = err_win_to_posix(GetLastError());\n \t\tif (ret && buf.len) {\n-\t\t\terrno = err_win_to_posix(GetLastError());\n \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n \t\t\t\terr, buf.buf);\n \t\t}\n-- \ngitgitgadget\n"},{"id":"387324","messageId":"52f2fe39-6a71-a327-8aea-ba757b7eec18@kdbg.org","threadId":"52372","inReplyTo":"685360f735e35e837bc9ef684cbde33564c81666.1575063876.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-11-29T23:02:48Z","receivedAt":"2019-11-29T23:02:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin \n> \n> In 9a780a384de (mingw: spawned processes need to inherit only standard\n> handles, 2019-11-22), we taught the Windows-specific part to restrict\n> which file handles are passed on to the spawned processes.\n> \n> Since this logic seemed to be a bit fragile across Windows versions (we\n> _still_ support Windows Vista in Git for Windows, for example), a\n> fall-back was added to try spawning the process again, this time without\n> restricting which file handles are to be inherited by the spawned\n> process.\n> \n> In the common case (i.e. when the process could not be spawned for\n> reasons _other_ than the file handle inheritance), the fall-back attempt\n> would still fail, of course.\n> \n> Crucially, one thing we missed in that code path was to set `errno`\n> appropriately.\n> \n> This should have been caught by t0061.2 which expected `errno` to be\n> `ENOENT` after trying to start a process for a non-existing executable,\n> but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:\n> while looking for the config settings for trace2, Git tries to access\n> `xdg_config` and `user_config` via `access_or_die()`, and as neither of\n> those config files exists when running the test case (because in Git's\n> test suite, `HOME` points to the test directory), the `errno` has the\n> expected value, but for the wrong reasons.\n> \n> Let's fix that by making sure that `errno` is set correctly.\n> \n> It would be nice if we could somehow fix t0061 to make sure that this\n> does not regress again. One approach that seemed like it should work,\n> but did not, was to set `errno` to 0 in the test helper that is used by\n> t0061.2.\n> \n> However, when `mingw_spawnvpe()` wants to see whether the file in\n> question is a script, it calls `parse_interpreter()`, which in turn\n> tries to `open()` the file.0/compat/mingw.c#L1134. Obviously,\n\nCopy-and-paste garbage?\n\n> this call fails, and sets `errno` to `ENOENT`, deep inside the call\n> chain started from that test helper.\n> \n> Instead, we force re-set `errno` at the beginning of the function\n> `mingw_spawnve_fd()`, which _should_ be safe given that callers of that\n> function will want to look at `errno` if -1 was returned. And if that\n> `errno` is 0 (\"No error\"), regression tests like t0061.2 will kick in.\n> \n> Reported-by: Johannes Sixt \n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/mingw.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 2b6eca2f56..bb4eb4211a 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n> \tconst char *(*quote_arg)(const char *arg) =\n> \t\tis_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;\n> \n> +\t/* Make sure to override previous errors, if any */\n> +\terrno = 0;\n> +\n> \tif (restrict_handle_inheritance < 0)\n> \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n> \t/*\n> @@ -1580,8 +1583,8 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n> \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n> \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n> \t\t\t\t &si.StartupInfo, &pi);\n> +\t\terrno = err_win_to_posix(GetLastError());\n\nI think this should be protected by 'if (!ret)' because\nerr_win_to_posix() does not handle ERROR_SUCCESS and turns it into\nENOSYS. It's not that bad because in the case of success we do not\nguarantee any value of errno anyway.\n\n> \t\tif (ret && buf.len) {\n> -\t\t\terrno = err_win_to_posix(GetLastError());\n> \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n> \t\t\t\terr, buf.buf);\n> \t\t}\n> \n\nThat said, this fixes the failure.\n\n-- Hannes\n"},{"id":"387329","messageId":"280b6d08a4cdae7af168371670f41585aa4778d4.1575110200.git.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-30T10:36:39Z","receivedAt":"2019-11-30T10:36:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 9a780a384de (mingw: spawned processes need to inherit only standard\nhandles, 2019-11-22), we taught the Windows-specific part to restrict\nwhich file handles are passed on to the spawned processes.\n\nSince this logic seemed to be a bit fragile across Windows versions (we\n_still_ support Windows Vista in Git for Windows, for example), a\nfall-back was added to try spawning the process again, this time without\nrestricting which file handles are to be inherited by the spawned\nprocess.\n\nIn the common case (i.e. when the process could not be spawned for\nreasons _other_ than the file handle inheritance), the fall-back attempt\nwould still fail, of course.\n\nCrucially, one thing we missed in that code path was to set `errno`\nappropriately.\n\nThis should have been caught by t0061.2 which expected `errno` to be\n`ENOENT` after trying to start a process for a non-existing executable,\nbut `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:\nwhile looking for the config settings for trace2, Git tries to access\n`xdg_config` and `user_config` via `access_or_die()`, and as neither of\nthose config files exists when running the test case (because in Git's\ntest suite, `HOME` points to the test directory), the `errno` has the\nexpected value, but for the wrong reasons.\n\nLet's fix that by making sure that `errno` is set correctly. It even\nappears that `errno` was set in the _wrong_ case previously:\n`CreateProcessW()` returns non-zero upon success, but `errno` was set\nonly in the non-zero case.\n\nIt would be nice if we could somehow fix t0061 to make sure that this\ndoes not regress again. One approach that seemed like it should work,\nbut did not, was to set `errno` to 0 in the test helper that is used by\nt0061.2.\n\nHowever, when `mingw_spawnvpe()` wants to see whether the file in\nquestion is a script, it calls `parse_interpreter()`, which in turn\ntries to `open()` the file. Obviously, this call fails, and sets `errno`\nto `ENOENT`, deep inside the call chain started from that test helper.\n\nInstead, we force re-set `errno` at the beginning of the function\n`mingw_spawnve_fd()`, which _should_ be safe given that callers of that\nfunction will want to look at `errno` if -1 was returned. And if that\n`errno` is 0 (\"No error\"), regression tests like t0061.2 will kick in.\n\nReported-by: Johannes Sixt \nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2b6eca2f56..432adc1aed 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \tconst char *(*quote_arg)(const char *arg) =\n \t\tis_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;\n \n+\t/* Make sure to override previous errors, if any */\n+\terrno = 0;\n+\n \tif (restrict_handle_inheritance < 0)\n \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n \t/*\n@@ -1580,8 +1583,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n \t\t\t\t &si.StartupInfo, &pi);\n-\t\tif (ret && buf.len) {\n+\t\tif (!ret)\n \t\t\terrno = err_win_to_posix(GetLastError());\n+\t\tif (ret && buf.len) {\n \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n \t\t\t\terr, buf.buf);\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"387328","messageId":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.git.1575063876.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-30T10:36:38Z","receivedAt":"2019-11-30T10:36:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Johannes \"Hannes\" Sixt pointed out that this patch series (which already\nmade it to next) introduces a problem: when falling back to spawning the\nprocess without restricting the file handles, errno is not set accurately.\n\nSadly, the regression test failure observed by Hannes did not show up over\nhere, nor in the PR builds (or, for that matter, the literally hundreds of\nCI builds that patch series underwent as part of Git for Windows' master \nbranch). The cause was that errno is set to the expected ENOENT by another \npart of the code, too, that happens right before the calls to \nCreateProcessW(): the test whether a given path refers to a script that\nneeds to be executed via an interpreter (such as sh.exe) obviously needs to\nopen the file, and that obviously fails, setting errno = ENOENT!\n\nI have currently no idea what function call could be responsible for\nre-setting errno to the reported ERANGE... But at least over here, when I\npartially apply this patch, the part that sets errno = 0;, t0061.2 fails for\nme, too.\n\nChanges since v1:\n\n * A copy/edit fail in the commit message was fixed.\n * We now assign errno only when the call to CreateProcessW() failed.\n * For good measure, we teach the err_win_to_posix() function to translate \n ERROR_SUCCESS into the errno value 0.\n\nJohannes Schindelin (2):\n mingw: do set `errno` correctly when trying to restrict handle\n inheritance\n mingw: translate ERROR_SUCCESS to errno = 0\n\n compat/mingw.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\n\nbase-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/480\n\nRange-diff vs v1:\n\n 1: 685360f735 ! 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n @@ -28,7 +28,10 @@\n test suite, `HOME` points to the test directory), the `errno` has the\n expected value, but for the wrong reasons.\n \n - Let's fix that by making sure that `errno` is set correctly.\n + Let's fix that by making sure that `errno` is set correctly. It even\n + appears that `errno` was set in the _wrong_ case previously:\n + `CreateProcessW()` returns non-zero upon success, but `errno` was set\n + only in the non-zero case.\n \n It would be nice if we could somehow fix t0061 to make sure that this\n does not regress again. One approach that seemed like it should work,\n @@ -37,9 +40,8 @@\n \n However, when `mingw_spawnvpe()` wants to see whether the file in\n question is a script, it calls `parse_interpreter()`, which in turn\n - tries to `open()` the file.0/compat/mingw.c#L1134. Obviously,\n - this call fails, and sets `errno` to `ENOENT`, deep inside the call\n - chain started from that test helper.\n + tries to `open()` the file. Obviously, this call fails, and sets `errno`\n + to `ENOENT`, deep inside the call chain started from that test helper.\n \n Instead, we force re-set `errno` at the beginning of the function\n `mingw_spawnve_fd()`, which _should_ be safe given that callers of that\n @@ -66,9 +68,10 @@\n \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n \t\t\t\t &si.StartupInfo, &pi);\n -+\t\terrno = err_win_to_posix(GetLastError());\n - \t\tif (ret && buf.len) {\n --\t\t\terrno = err_win_to_posix(GetLastError());\n +-\t\tif (ret && buf.len) {\n ++\t\tif (!ret)\n + \t\t\terrno = err_win_to_posix(GetLastError());\n ++\t\tif (ret && buf.len) {\n \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n \t\t\t\terr, buf.buf);\n \t\t}\n -: ---------- > 2: c3dea00fb1 mingw: translate ERROR_SUCCESS to errno = 0\n\n-- \ngitgitgadget\n"},{"id":"387330","messageId":"c3dea00fb10f1ffd393488f8fcc9081abd5a1647.1575110200.git.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] mingw: translate ERROR_SUCCESS to errno = 0","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-30T10:36:40Z","receivedAt":"2019-11-30T10:36:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nJohannes Sixt pointed out that the `err_win_to_posix()` function\nmishandles `ERROR_SUCCESS`. This commit fixes that.\n\nTechnically, we try to only assign `errno` to the corresponding value of\n`GetLastError()` (which translation is performed by that function) when\na Win32 API call failed, so this change is purely defensive and is not\nexpected to fix an existing bug in our code base.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 432adc1aed..8dc45808a3 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr)\n \tcase ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;\n \tcase ERROR_SHARING_VIOLATION: error = EACCES; break;\n \tcase ERROR_STACK_OVERFLOW: error = ENOMEM; break;\n+\tcase ERROR_SUCCESS: error = 0; break;\n \tcase ERROR_SWAPERROR: error = ENOENT; break;\n \tcase ERROR_TOO_MANY_MODULES: error = EMFILE; break;\n \tcase ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;\n-- \ngitgitgadget\n"},{"id":"387343","messageId":"xmqq5zj12qc2.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T18:04:29Z","receivedAt":"2019-11-30T18:04:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> * We now assign errno only when the call to CreateProcessW() failed.\n\nMeaning the global variable 'errno' is left as it was (instead of\ngetting cleared) when a system call succeeds? That I think is the\ncorrect behaviour people who use the variable expect.\n\n> * For good measure, we teach the err_win_to_posix() function to translate \n> ERROR_SUCCESS into the errno value 0.\n\nSo, I am not sure if this is a good idea---who are the callers of\nthis function and why do they call it? I would imagine that a\ncaller who makes a system call, upon seeing a failure from the\nsystem call, calls this helper with the Windows error code it\nreceived from the system call so that errno can be updated with a\nPOSIXy value. If my imagination is correct, such a caller should\nnot be assigning anything to errno if the underlying system call\nsucceeds, i.e. returns ERROR_SUCCESS. So a better solution might\nbe for the function to BUG() when fed ERROR_SUCCESS to point fingers\nat the caller, no?\n\nIf my imagination is not correct, then ignore the whole paragraph\nabove ;-).\n\nThanks.\n"},{"id":"387344","messageId":"6beaf25f-1369-3e77-fd54-fdb5a1e63707@kdbg.org","threadId":"52372","inReplyTo":"xmqq5zj12qc2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-11-30T19:13:56Z","receivedAt":"2019-11-30T19:14:03Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.11.19 um 19:04 schrieb Junio C Hamano:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n>> * We now assign errno only when the call to CreateProcessW() failed.\n> \n> Meaning the global variable 'errno' is left as it was (instead of\n> getting cleared) when a system call succeeds? That I think is the\n> correct behaviour people who use the variable expect.\n\nI hope you mean people who read the code. You cannot possibly mean\ndevelopers who expect that the run-command API keeps errno unchanged if\nthe calls were successful. I'm pretty sure they do not provide such a\nguarantee.\n\n> \n>> * For good measure, we teach the err_win_to_posix() function to translate \n>> ERROR_SUCCESS into the errno value 0.\n> \n> So, I am not sure if this is a good idea---who are the callers of\n> this function and why do they call it? I would imagine that a\n> caller who makes a system call, upon seeing a failure from the\n> system call, calls this helper with the Windows error code it\n> received from the system call so that errno can be updated with a\n> POSIXy value. If my imagination is correct, such a caller should\n> not be assigning anything to errno if the underlying system call\n> succeeds, i.e. returns ERROR_SUCCESS. So a better solution might\n> be for the function to BUG() when fed ERROR_SUCCESS to point fingers\n> at the caller, no?\n\nJust like on POSIX the value of errno is indeterminate after a\nsuccessful system call, the value of GetLastError() indeterminate after\na successful Windows API call. Therefore, the err_win_to_posix() would\nnot be able to point at a bogus caller reliably. For this reason, let's\nconsider the function as a simple error code translator, and then\ntranslating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.\n\n-- Hannes\n"},{"id":"387345","messageId":"7a88a80d-76db-3ca5-c5e1-134b82b5bcbf@kdbg.org","threadId":"52372","inReplyTo":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-11-30T19:20:11Z","receivedAt":"2019-11-30T19:20:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.11.19 um 11:36 schrieb Johannes Schindelin via GitGitGadget:\n> Johannes \"Hannes\" Sixt pointed out that this patch series (which already\n> made it to next) introduces a problem: when falling back to spawning the\n> process without restricting the file handles, errno is not set accurately.\n> \n> Sadly, the regression test failure observed by Hannes did not show up over\n> here, nor in the PR builds (or, for that matter, the literally hundreds of\n> CI builds that patch series underwent as part of Git for Windows' master \n> branch). The cause was that errno is set to the expected ENOENT by another \n> part of the code, too, that happens right before the calls to \n> CreateProcessW(): the test whether a given path refers to a script that\n> needs to be executed via an interpreter (such as sh.exe) obviously needs to\n> open the file, and that obviously fails, setting errno = ENOENT!\n> \n> I have currently no idea what function call could be responsible for\n> re-setting errno to the reported ERANGE... But at least over here, when I\n> partially apply this patch, the part that sets errno = 0;, t0061.2 fails for\n> me, too.\n> \n> Changes since v1:\n> \n> * A copy/edit fail in the commit message was fixed.\n> * We now assign errno only when the call to CreateProcessW() failed.\n> * For good measure, we teach the err_win_to_posix() function to translate \n> ERROR_SUCCESS into the errno value 0.\n\nThis series fixes the observed failure. The changes look good. Thank you\nfor the quick turn-around.\n\nTested-by: Johannes Sixt \n\n> \n> Johannes Schindelin (2):\n> mingw: do set `errno` correctly when trying to restrict handle\n> inheritance\n> mingw: translate ERROR_SUCCESS to errno = 0\n> \n> compat/mingw.c | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> \n> base-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/480\n> \n> Range-diff vs v1:\n> \n> 1: 685360f735 ! 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n> @@ -28,7 +28,10 @@\n> test suite, `HOME` points to the test directory), the `errno` has the\n> expected value, but for the wrong reasons.\n> \n> - Let's fix that by making sure that `errno` is set correctly.\n> + Let's fix that by making sure that `errno` is set correctly. It even\n> + appears that `errno` was set in the _wrong_ case previously:\n> + `CreateProcessW()` returns non-zero upon success, but `errno` was set\n> + only in the non-zero case.\n> \n> It would be nice if we could somehow fix t0061 to make sure that this\n> does not regress again. One approach that seemed like it should work,\n> @@ -37,9 +40,8 @@\n> \n> However, when `mingw_spawnvpe()` wants to see whether the file in\n> question is a script, it calls `parse_interpreter()`, which in turn\n> - tries to `open()` the file.0/compat/mingw.c#L1134. Obviously,\n> - this call fails, and sets `errno` to `ENOENT`, deep inside the call\n> - chain started from that test helper.\n> + tries to `open()` the file. Obviously, this call fails, and sets `errno`\n> + to `ENOENT`, deep inside the call chain started from that test helper.\n> \n> Instead, we force re-set `errno` at the beginning of the function\n> `mingw_spawnve_fd()`, which _should_ be safe given that callers of that\n> @@ -66,9 +68,10 @@\n> \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n> \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n> \t\t\t\t &si.StartupInfo, &pi);\n> -+\t\terrno = err_win_to_posix(GetLastError());\n> - \t\tif (ret && buf.len) {\n> --\t\t\terrno = err_win_to_posix(GetLastError());\n> +-\t\tif (ret && buf.len) {\n> ++\t\tif (!ret)\n> + \t\t\terrno = err_win_to_posix(GetLastError());\n> ++\t\tif (ret && buf.len) {\n> \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n> \t\t\t\terr, buf.buf);\n> \t\t}\n> -: ---------- > 2: c3dea00fb1 mingw: translate ERROR_SUCCESS to errno = 0\n> \n\n-- Hannes\n"},{"id":"387346","messageId":"8736e5t98b.fsf@igel.home","threadId":"52372","inReplyTo":"6beaf25f-1369-3e77-fd54-fdb5a1e63707@kdbg.org","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2019-11-30T20:11:48Z","receivedAt":"2019-11-30T20:11:56Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 30 2019, Johannes Sixt wrote:\n\n> Am 30.11.19 um 19:04 schrieb Junio C Hamano:\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>> \n>>> * We now assign errno only when the call to CreateProcessW() failed.\n>> \n>> Meaning the global variable 'errno' is left as it was (instead of\n>> getting cleared) when a system call succeeds? That I think is the\n>> correct behaviour people who use the variable expect.\n>\n> I hope you mean people who read the code. You cannot possibly mean\n> developers who expect that the run-command API keeps errno unchanged if\n> the calls were successful. I'm pretty sure they do not provide such a\n> guarantee.\n\nPOSIX guarantees that no library function sets errno to zero.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"387347","messageId":"xmqq1rtp2k01.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"6beaf25f-1369-3e77-fd54-fdb5a1e63707@kdbg.org","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T20:21:18Z","receivedAt":"2019-11-30T20:23:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Just like on POSIX the value of errno is indeterminate after a\n> successful system call, the value of GetLastError() indeterminate after\n> a successful Windows API call. Therefore, the err_win_to_posix() would\n> not be able to point at a bogus caller reliably. For this reason, let's\n> consider the function as a simple error code translator, and then\n> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.\n\nOK, that makes sense.\n\nThanks.\n"},{"id":"387348","messageId":"xmqqwobh15c7.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"8736e5t98b.fsf@igel.home","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T20:23:20Z","receivedAt":"2019-11-30T20:23:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> On Nov 30 2019, Johannes Sixt wrote:\n>\n>> Am 30.11.19 um 19:04 schrieb Junio C Hamano:\n>>> \"Johannes Schindelin via GitGitGadget\" \n>>> writes:\n>>> \n>>>> * We now assign errno only when the call to CreateProcessW() failed.\n>>> \n>>> Meaning the global variable 'errno' is left as it was (instead of\n>>> getting cleared) when a system call succeeds? That I think is the\n>>> correct behaviour people who use the variable expect.\n>>\n>> I hope you mean people who read the code. You cannot possibly mean\n>> developers who expect that the run-command API keeps errno unchanged if\n>> the calls were successful. I'm pretty sure they do not provide such a\n>> guarantee.\n>\n> POSIX guarantees that no library function sets errno to zero.\n\nIt is true, but the rest of the Git code works on top of an\nabstraction a bit higher than C/POSIX library. The run-command API\nJ6t cites is an example.\n\nIOW, we cannot take advantage of that POSIX guarantee in the\ncodepaths that use our internal API. So...\n\n"},{"id":"387349","messageId":"87y2vxrt6t.fsf@igel.home","threadId":"52372","inReplyTo":"xmqqwobh15c7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2019-11-30T20:43:38Z","receivedAt":"2019-11-30T20:47:35Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 30 2019, Junio C Hamano wrote:\n\n> IOW, we cannot take advantage of that POSIX guarantee in the\n> codepaths that use our internal API. So...\n\nBut it's still a good practice to follow.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"387351","messageId":"nycvar.QRO.7.76.6.1911302219350.31080@tvgsbejvaqbjf.bet","threadId":"52372","inReplyTo":"87y2vxrt6t.fsf@igel.home","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-30T21:22:36Z","receivedAt":"2019-11-30T21:23:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Andreas,\n\nOn Sat, 30 Nov 2019, Andreas Schwab wrote:\n\n> On Nov 30 2019, Junio C Hamano wrote:\n>\n> > IOW, we cannot take advantage of that POSIX guarantee in the\n> > codepaths that use our internal API. So...\n>\n> But it's still a good practice to follow.\n\nAn even better practice is to focus on what's important.\n\nYou could, for example, review the original patches again that needed this\nfixup, to make sure that nothing similar lurks in them. That would seem to\nme certainly more meaningful than harping on POSIX semantics in a context\nwhere they do not actually matter.\n\nCiao,\nJohannes\n"},{"id":"387353","messageId":"nycvar.QRO.7.76.6.1911302303510.31080@tvgsbejvaqbjf.bet","threadId":"52372","inReplyTo":"52f2fe39-6a71-a327-8aea-ba757b7eec18@kdbg.org","subject":"Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-30T22:06:56Z","receivedAt":"2019-11-30T22:07:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sat, 30 Nov 2019, Johannes Sixt wrote:\n\n> Am 29.11.19 um 22:44 schrieb Johannes Schindelin via GitGitGadget:\n> > From: Johannes Schindelin \n> >\n> > In 9a780a384de (mingw: spawned processes need to inherit only standard\n> > handles, 2019-11-22), we taught the Windows-specific part to restrict\n> > which file handles are passed on to the spawned processes.\n> >\n> > Since this logic seemed to be a bit fragile across Windows versions (we\n> > _still_ support Windows Vista in Git for Windows, for example), a\n> > fall-back was added to try spawning the process again, this time without\n> > restricting which file handles are to be inherited by the spawned\n> > process.\n> >\n> > In the common case (i.e. when the process could not be spawned for\n> > reasons _other_ than the file handle inheritance), the fall-back attempt\n> > would still fail, of course.\n> >\n> > Crucially, one thing we missed in that code path was to set `errno`\n> > appropriately.\n> >\n> > This should have been caught by t0061.2 which expected `errno` to be\n> > `ENOENT` after trying to start a process for a non-existing executable,\n> > but `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:\n> > while looking for the config settings for trace2, Git tries to access\n> > `xdg_config` and `user_config` via `access_or_die()`, and as neither of\n> > those config files exists when running the test case (because in Git's\n> > test suite, `HOME` points to the test directory), the `errno` has the\n> > expected value, but for the wrong reasons.\n> >\n> > Let's fix that by making sure that `errno` is set correctly.\n> >\n> > It would be nice if we could somehow fix t0061 to make sure that this\n> > does not regress again. One approach that seemed like it should work,\n> > but did not, was to set `errno` to 0 in the test helper that is used by\n> > t0061.2.\n> >\n> > However, when `mingw_spawnvpe()` wants to see whether the file in\n> > question is a script, it calls `parse_interpreter()`, which in turn\n> > tries to `open()` the file.0/compat/mingw.c#L1134. Obviously,\n>\n> Copy-and-paste garbage?\n\nYes :-(\n\n> > this call fails, and sets `errno` to `ENOENT`, deep inside the call\n> > chain started from that test helper.\n> >\n> > Instead, we force re-set `errno` at the beginning of the function\n> > `mingw_spawnve_fd()`, which _should_ be safe given that callers of that\n> > function will want to look at `errno` if -1 was returned. And if that\n> > `errno` is 0 (\"No error\"), regression tests like t0061.2 will kick in.\n> >\n> > Reported-by: Johannes Sixt \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > compat/mingw.c | 5 ++++-\n> > 1 file changed, 4 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index 2b6eca2f56..bb4eb4211a 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n> > \tconst char *(*quote_arg)(const char *arg) =\n> > \t\tis_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;\n> >\n> > +\t/* Make sure to override previous errors, if any */\n> > +\terrno = 0;\n> > +\n> > \tif (restrict_handle_inheritance < 0)\n> > \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n> > \t/*\n> > @@ -1580,8 +1583,8 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n> > \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n> > \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n> > \t\t\t\t &si.StartupInfo, &pi);\n> > +\t\terrno = err_win_to_posix(GetLastError());\n>\n> I think this should be protected by 'if (!ret)' because\n> err_win_to_posix() does not handle ERROR_SUCCESS and turns it into\n> ENOSYS. It's not that bad because in the case of success we do not\n> guarantee any value of errno anyway.\n\nAh, that's the reason!\n\nAre you building with `NO_GETTEXT` perchance? I ask because gettext\noverrides `vsnprintf()` with their own version, which is obviously quite\ndifferent from the version MSVCRT provides... and the former version is\nwhat I use, and what all those CI/PR builds use.\n\n> > \t\tif (ret && buf.len) {\n> > -\t\t\terrno = err_win_to_posix(GetLastError());\n> > \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n> > \t\t\t\terr, buf.buf);\n> > \t\t}\n> >\n>\n> That said, this fixes the failure.\n\nThanks,\nDscho\n"},{"id":"387354","messageId":"xmqqk17h10fb.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"7a88a80d-76db-3ca5-c5e1-134b82b5bcbf@kdbg.org","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T22:09:28Z","receivedAt":"2019-11-30T22:09:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> This series fixes the observed failure. The changes look good. Thank you\n> for the quick turn-around.\n>\n> Tested-by: Johannes Sixt \n\nThanks, both.\n"},{"id":"387356","messageId":"4c8dc4c9-6d37-6879-d535-2ed4f17f2b81@kdbg.org","threadId":"52372","inReplyTo":"nycvar.QRO.7.76.6.1911302303510.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-11-30T22:16:28Z","receivedAt":"2019-11-30T22:16:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.11.19 um 23:06 schrieb Johannes Schindelin:\n> Are you building with `NO_GETTEXT` perchance? I ask because gettext\n> overrides `vsnprintf()` with their own version, which is obviously quite\n> different from the version MSVCRT provides... and the former version is\n> what I use, and what all those CI/PR builds use.\n\nIndeed, I build with NO_GETTEXT.\n\n-- Hannes\n"},{"id":"387367","messageId":"xmqqfti41rz1.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"xmqq1rtp2k01.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-01T06:26:42Z","receivedAt":"2019-12-01T06:27:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Sixt writes:\n>\n>> Just like on POSIX the value of errno is indeterminate after a\n>> successful system call, the value of GetLastError() indeterminate after\n>> a successful Windows API call. Therefore, the err_win_to_posix() would\n>> not be able to point at a bogus caller reliably. For this reason, let's\n>> consider the function as a simple error code translator, and then\n>> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.\n>\n> OK, that makes sense.\n\nActually, I do not think it makes that much sense, especially in the\ncontext of this patch that claims to map success to 0 (assuming that\nno E_ANYTHING has the value of zero---I am not sure POSIX gives such\na guarantee) \"for good measure\".\n\nEven if Windows API makes the GetLastError() unusable after a\nsuccess call (unlike POSIX, where errno is left alone), the API\ncalls themselves would be signaling their own success or failure,\nright? So I would have imagined that any kosher caller would be\ndoing this:\n\n\tif (SomeWinAPI() != SUCCESS) {\n\t\terrno = err_win_to_posix(GetLastError());\n\t\t... possibly other reactions to the error ...\n\t}\n\nIf using a value grabbed from GetLastError() after a successfull\nWindows API call is a wrong thing to do as you taught us in your\nmessage, then an unconditional\n\n\tSomeWinAPI();\n\terrno = err_win_to_posix(GetLastError());\n\nwould be wrong anyway, and touching errno unconditionally, when the\nprevious call may have succeeded without checking, makes the pattern\ndoubly wrong, no?\n\nHaving said that, I do not expect myself to be looking into and\nfixing anything in compat/*win*.[ch], so I do not care too deeply\neither way, but I thought that it would help keep the sanity of\ndevelopers involved if we touched errno only upon a failure from an\nunderlying system.\n\nThanks.\n"},{"id":"387371","messageId":"nycvar.QRO.7.76.6.1912011042460.31080@tvgsbejvaqbjf.bet","threadId":"52372","inReplyTo":"xmqqfti41rz1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-01T09:53:25Z","receivedAt":"2019-12-01T09:53:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 30 Nov 2019, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Johannes Sixt writes:\n> >\n> >> Just like on POSIX the value of errno is indeterminate after a\n> >> successful system call, the value of GetLastError() indeterminate after\n> >> a successful Windows API call. Therefore, the err_win_to_posix() would\n> >> not be able to point at a bogus caller reliably. For this reason, let's\n> >> consider the function as a simple error code translator, and then\n> >> translating ERROR_SUCCESS to 0 (or is there ESUCCESS?) makes total sense.\n> >\n> > OK, that makes sense.\n>\n> Actually, I do not think it makes that much sense, especially in the\n> context of this patch that claims to map success to 0 (assuming that\n> no E_ANYTHING has the value of zero---I am not sure POSIX gives such\n> a guarantee) \"for good measure\".\n\nBut the intention is not to help correct callers! The purpose of mapping\n`ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error\nwhen none happened. On Windows, which is the only platform where the\ndiscussed function is compiled and used, `strerror(0)` has the\nwell-established behavior of returning \"No error\".\n\nNow imagine that a user encounters a bug where a code path does indeed\nincorrectly report an error where no error occurred (or where Git is\nlooking for the error too late or something like that). You probably agree\nwith me that we should really avoid reporting \"Function not implemented\"\nin such a case and rather say \"No error\", which will help figure out the\nbug much quicker.\n\nAnd that is what this patch is all about.\n\nCiao,\nDscho\n\n> Even if Windows API makes the GetLastError() unusable after a\n> success call (unlike POSIX, where errno is left alone), the API\n> calls themselves would be signaling their own success or failure,\n> right? So I would have imagined that any kosher caller would be\n> doing this:\n>\n> \tif (SomeWinAPI() != SUCCESS) {\n> \t\terrno = err_win_to_posix(GetLastError());\n> \t\t... possibly other reactions to the error ...\n> \t}\n>\n> If using a value grabbed from GetLastError() after a successfull\n> Windows API call is a wrong thing to do as you taught us in your\n> message, then an unconditional\n>\n> \tSomeWinAPI();\n> \terrno = err_win_to_posix(GetLastError());\n>\n> would be wrong anyway, and touching errno unconditionally, when the\n> previous call may have succeeded without checking, makes the pattern\n> doubly wrong, no?\n>\n> Having said that, I do not expect myself to be looking into and\n> fixing anything in compat/*win*.[ch], so I do not care too deeply\n> either way, but I thought that it would help keep the sanity of\n> developers involved if we touched errno only upon a failure from an\n> underlying system.\n>\n> Thanks.\n>\n"},{"id":"387384","messageId":"xmqqk17fz2e7.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"nycvar.QRO.7.76.6.1912011042460.31080@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-02T06:07:28Z","receivedAt":"2019-12-02T06:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> But the intention is not to help correct callers! The purpose of mapping\n> `ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error\n> when none happened. On Windows, which is the only platform where the\n> discussed function is compiled and used, `strerror(0)` has the\n> well-established behavior of returning \"No error\".\n\nBut is the caller that ends up saying \"No error\" you mention below\nby calling strerror() exists *only* on Windows port somewhere in\ncompat/ directory? If the call to strerror() is made from a\ncodepath that is common to all platforms, then that caller *is*\nwrong to do so after seeing *no* errors.\n\nSo, no, sorry, but I do not understand the above line of reasoning.\n\n> Now imagine that a user encounters a bug where a code path does indeed\n> incorrectly report an error where no error occurred (or where Git is\n> looking for the error too late or something like that). You probably agree\n> with me that we should really avoid reporting \"Function not implemented\"\n> in such a case and rather say \"No error\", which will help figure out the\n> bug much quicker.\n\nNot really. A BUG() that catches a call to this function that\nshould not be called when there is no error would help identify the\nproblematic caller a lot faster by being louder---those users who\nget hit will raise their hands more readily than those who see \"No\nerror\" and wonders \"why does Git give that error message when there\nis no error?\". IOW, \"No error\" sounds like sweeping the issue under\nthe rug.\n\nThe argument could be \"I do not want to be pressured to hunt for the\nproblematic caller, and taking the approach to be less loud than the\nBUG() approach would allow the end users keep operating as if\nnothing bad happened, so that I can hunt for the caller iff/when I\nfeel like it, and with no urgent need I can just ignore the hunt\naltogether. Not hitting with BUG() is a service to the end users\nwho shouldn't be made a guinea pig---that is why we want to sweep\nthe problematic caller under the rug. It may delay the discovery\nand fixing of problematic callers, which is a downside, though.\"\n\nI would understand such an argument, even if I might not think that\nsuch an argument makes the right tradeoff between not hitting the\nusers with BUG() and not being effective in finding the problematic\ncaller.\n\nThanks.\n"},{"id":"387391","messageId":"nycvar.QRO.7.76.6.1912021001510.31080@tvgsbejvaqbjf.bet","threadId":"52372","inReplyTo":"xmqqk17fz2e7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-02T09:05:08Z","receivedAt":"2019-12-02T09:05:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 1 Dec 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > But the intention is not to help correct callers! The purpose of mapping\n> > `ERROR_SUCCESS` to 0 will help _only_ callers that try to report an error\n> > when none happened. On Windows, which is the only platform where the\n> > discussed function is compiled and used, `strerror(0)` has the\n> > well-established behavior of returning \"No error\".\n>\n> But is the caller that ends up saying \"No error\" you mention below\n> by calling strerror() exists *only* on Windows port somewhere in\n> compat/ directory? If the call to strerror() is made from a\n> codepath that is common to all platforms, then that caller *is*\n> wrong to do so after seeing *no* errors.\n\nYes, that would be a difference between platforms, but _at least_ the\nWindows side would help diagnose the bug better. I'd prefer such a half\nsolution to having no solution at all.\n\n> So, no, sorry, but I do not understand the above line of reasoning.\n>\n> > Now imagine that a user encounters a bug where a code path does indeed\n> > incorrectly report an error where no error occurred (or where Git is\n> > looking for the error too late or something like that). You probably agree\n> > with me that we should really avoid reporting \"Function not implemented\"\n> > in such a case and rather say \"No error\", which will help figure out the\n> > bug much quicker.\n>\n> Not really. A BUG() that catches a call to this function that\n> should not be called when there is no error would help identify the\n> problematic caller a lot faster by being louder---those users who\n> get hit will raise their hands more readily than those who see \"No\n> error\" and wonders \"why does Git give that error message when there\n> is no error?\". IOW, \"No error\" sounds like sweeping the issue under\n> the rug.\n\nThis is a very good point. It _is_ a bug to hit that code path when\n`GetLastError()` returned `ERROR_SUCCESS`, and having such a `BUG()` call\nwould have prevented the bug that I fix in 1/2 from slipping through.\n\nTherefore, I changed 2/2 to raise a BUG() and am currently waiting for the\nPR build to finish before sending another iteration of this seris.\n\nCiao,\nDscho\n\n> The argument could be \"I do not want to be pressured to hunt for the\n> problematic caller, and taking the approach to be less loud than the\n> BUG() approach would allow the end users keep operating as if\n> nothing bad happened, so that I can hunt for the caller iff/when I\n> feel like it, and with no urgent need I can just ignore the hunt\n> altogether. Not hitting with BUG() is a service to the end users\n> who shouldn't be made a guinea pig---that is why we want to sweep\n> the problematic caller under the rug. It may delay the discovery\n> and fixing of problematic callers, which is a downside, though.\"\n>\n> I would understand such an argument, even if I might not think that\n> such an argument makes the right tradeoff between not hitting the\n> users with BUG() and not being effective in finding the problematic\n> caller.\n>\n> Thanks.\n>\n>\n"},{"id":"387393","messageId":"e04e1269b308bb000ea2dad2e5f2d04ef6f17502.1575286409.git.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.v3.git.1575286409.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] mingw: translate ERROR_SUCCESS to errno = 0","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-02T11:33:29Z","receivedAt":"2019-12-02T11:33:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nJohannes Sixt pointed out that the `err_win_to_posix()` function\nmishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.\n\nThe only purpose of this function is to map Win32 API errors to `errno`\nones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea\nof `errno` is that it will only be set in case of an error, and left\nalone in case of success.\n\nTherefore, as pointed out by Junio Hamano, it is a bug to call this\nfunction when there was not even any error to map.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 432adc1aed..827065d96d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -114,6 +114,7 @@ int err_win_to_posix(DWORD winerr)\n \tcase ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;\n \tcase ERROR_SHARING_VIOLATION: error = EACCES; break;\n \tcase ERROR_STACK_OVERFLOW: error = ENOMEM; break;\n+\tcase ERROR_SUCCESS: BUG(\"err_win_to_posix() called without an error!\");\n \tcase ERROR_SWAPERROR: error = ENOENT; break;\n \tcase ERROR_TOO_MANY_MODULES: error = EMFILE; break;\n \tcase ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;\n-- \ngitgitgadget\n"},{"id":"387394","messageId":"pull.480.v3.git.1575286409.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.v2.git.1575110200.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-02T11:33:27Z","receivedAt":"2019-12-02T11:33:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Johannes \"Hannes\" Sixt pointed out that this patch series (which already\nmade it to next) introduces a problem: when falling back to spawning the\nprocess without restricting the file handles, errno is not set accurately.\n\nSadly, the regression test failure observed by Hannes did not show up over\nhere, nor in the PR builds (or, for that matter, the literally hundreds of\nCI builds that patch series underwent as part of Git for Windows' master \nbranch). The cause was that errno is set to the expected ENOENT by another \npart of the code, too, that happens right before the calls to \nCreateProcessW(): the test whether a given path refers to a script that\nneeds to be executed via an interpreter (such as sh.exe) obviously needs to\nopen the file, and that obviously fails, setting errno = ENOENT!\n\nI have currently no idea what function call could be responsible for\nre-setting errno to the reported ERANGE... But at least over here, when I\npartially apply this patch, the part that sets errno = 0;, t0061.2 fails for\nme, too.\n\nChange since v2:\n\n * We no longer map ERROR_SUCCESS to 0, but instead raise a bug message when\n the code is asked to handle the \"No error\" error.\n\nChanges since v1:\n\n * A copy/edit fail in the commit message was fixed.\n * We now assign errno only when the call to CreateProcessW() failed.\n * For good measure, we teach the err_win_to_posix() function to translate \n ERROR_SUCCESS into the errno value 0.\n\nJohannes Schindelin (2):\n mingw: do set `errno` correctly when trying to restrict handle\n inheritance\n mingw: translate ERROR_SUCCESS to errno = 0\n\n compat/mingw.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\n\nbase-commit: ac33519ddfa818f420b4ef5a09b4a7b3ac8adeb8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-480%2Fdscho%2Fmingw-inherit-only-std-handles-set-errno-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-480/dscho/mingw-inherit-only-std-handles-set-errno-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/480\n\nRange-diff vs v2:\n\n 1: 280b6d08a4 = 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n 2: c3dea00fb1 ! 2: e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0\n @@ -3,12 +3,15 @@\n mingw: translate ERROR_SUCCESS to errno = 0\n \n Johannes Sixt pointed out that the `err_win_to_posix()` function\n - mishandles `ERROR_SUCCESS`. This commit fixes that.\n + mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.\n \n - Technically, we try to only assign `errno` to the corresponding value of\n - `GetLastError()` (which translation is performed by that function) when\n - a Win32 API call failed, so this change is purely defensive and is not\n - expected to fix an existing bug in our code base.\n + The only purpose of this function is to map Win32 API errors to `errno`\n + ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea\n + of `errno` is that it will only be set in case of an error, and left\n + alone in case of success.\n +\n + Therefore, as pointed out by Junio Hamano, it is a bug to call this\n + function when there was not even any error to map.\n \n Signed-off-by: Johannes Schindelin \n \n @@ -19,7 +22,7 @@\n \tcase ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;\n \tcase ERROR_SHARING_VIOLATION: error = EACCES; break;\n \tcase ERROR_STACK_OVERFLOW: error = ENOMEM; break;\n -+\tcase ERROR_SUCCESS: error = 0; break;\n ++\tcase ERROR_SUCCESS: BUG(\"err_win_to_posix() called without an error!\");\n \tcase ERROR_SWAPERROR: error = ENOENT; break;\n \tcase ERROR_TOO_MANY_MODULES: error = EMFILE; break;\n \tcase ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;\n\n-- \ngitgitgadget\n"},{"id":"387395","messageId":"280b6d08a4cdae7af168371670f41585aa4778d4.1575286409.git.gitgitgadget@gmail.com","threadId":"52372","inReplyTo":"pull.480.v3.git.1575286409.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] mingw: do set `errno` correctly when trying to restrict handle inheritance","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-02T11:33:28Z","receivedAt":"2019-12-02T11:33:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 9a780a384de (mingw: spawned processes need to inherit only standard\nhandles, 2019-11-22), we taught the Windows-specific part to restrict\nwhich file handles are passed on to the spawned processes.\n\nSince this logic seemed to be a bit fragile across Windows versions (we\n_still_ support Windows Vista in Git for Windows, for example), a\nfall-back was added to try spawning the process again, this time without\nrestricting which file handles are to be inherited by the spawned\nprocess.\n\nIn the common case (i.e. when the process could not be spawned for\nreasons _other_ than the file handle inheritance), the fall-back attempt\nwould still fail, of course.\n\nCrucially, one thing we missed in that code path was to set `errno`\nappropriately.\n\nThis should have been caught by t0061.2 which expected `errno` to be\n`ENOENT` after trying to start a process for a non-existing executable,\nbut `errno` was set to `ENOENT` prior to the `CreateProcessW()` call:\nwhile looking for the config settings for trace2, Git tries to access\n`xdg_config` and `user_config` via `access_or_die()`, and as neither of\nthose config files exists when running the test case (because in Git's\ntest suite, `HOME` points to the test directory), the `errno` has the\nexpected value, but for the wrong reasons.\n\nLet's fix that by making sure that `errno` is set correctly. It even\nappears that `errno` was set in the _wrong_ case previously:\n`CreateProcessW()` returns non-zero upon success, but `errno` was set\nonly in the non-zero case.\n\nIt would be nice if we could somehow fix t0061 to make sure that this\ndoes not regress again. One approach that seemed like it should work,\nbut did not, was to set `errno` to 0 in the test helper that is used by\nt0061.2.\n\nHowever, when `mingw_spawnvpe()` wants to see whether the file in\nquestion is a script, it calls `parse_interpreter()`, which in turn\ntries to `open()` the file. Obviously, this call fails, and sets `errno`\nto `ENOENT`, deep inside the call chain started from that test helper.\n\nInstead, we force re-set `errno` at the beginning of the function\n`mingw_spawnve_fd()`, which _should_ be safe given that callers of that\nfunction will want to look at `errno` if -1 was returned. And if that\n`errno` is 0 (\"No error\"), regression tests like t0061.2 will kick in.\n\nReported-by: Johannes Sixt \nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2b6eca2f56..432adc1aed 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1423,6 +1423,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \tconst char *(*quote_arg)(const char *arg) =\n \t\tis_msys2_sh(*argv) ? quote_arg_msys2 : quote_arg_msvc;\n \n+\t/* Make sure to override previous errors, if any */\n+\terrno = 0;\n+\n \tif (restrict_handle_inheritance < 0)\n \t\trestrict_handle_inheritance = core_restrict_inherited_handles;\n \t/*\n@@ -1580,8 +1583,9 @@ static pid_t mingw_spawnve_fd(const char *cmd, const char **argv, char **deltaen\n \t\tret = CreateProcessW(*wcmd ? wcmd : NULL, wargs, NULL, NULL,\n \t\t\t\t TRUE, flags, wenvblk, dir ? wdir : NULL,\n \t\t\t\t &si.StartupInfo, &pi);\n-\t\tif (ret && buf.len) {\n+\t\tif (!ret)\n \t\t\terrno = err_win_to_posix(GetLastError());\n+\t\tif (ret && buf.len) {\n \t\t\twarning(\"failed to restrict file handles (%ld)\\n\\n%s\",\n \t\t\t\terr, buf.buf);\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"387406","messageId":"a4ac4530-9b5c-3eb6-ada2-067c6ef73868@kdbg.org","threadId":"52372","inReplyTo":"pull.480.v3.git.1575286409.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-12-02T17:35:10Z","receivedAt":"2019-12-02T17:35:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:\n> Range-diff vs v2:\n> \n> 1: 280b6d08a4 = 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n> 2: c3dea00fb1 ! 2: e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0\n> @@ -3,12 +3,15 @@\n> mingw: translate ERROR_SUCCESS to errno = 0\n\nThe subject line would have to be adjusted as well.\n\n\tmingw: forbid translating ERROR_SUCCESS to an errno value\n\nor something.\n\n> \n> Johannes Sixt pointed out that the `err_win_to_posix()` function\n> - mishandles `ERROR_SUCCESS`. This commit fixes that.\n> + mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.\n> \n> - Technically, we try to only assign `errno` to the corresponding value of\n> - `GetLastError()` (which translation is performed by that function) when\n> - a Win32 API call failed, so this change is purely defensive and is not\n> - expected to fix an existing bug in our code base.\n> + The only purpose of this function is to map Win32 API errors to `errno`\n> + ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea\n> + of `errno` is that it will only be set in case of an error, and left\n> + alone in case of success.\n> +\n> + Therefore, as pointed out by Junio Hamano, it is a bug to call this\n> + function when there was not even any error to map.\n> \n> Signed-off-by: Johannes Schindelin \n> \n> @@ -19,7 +22,7 @@\n> \tcase ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;\n> \tcase ERROR_SHARING_VIOLATION: error = EACCES; break;\n> \tcase ERROR_STACK_OVERFLOW: error = ENOMEM; break;\n> -+\tcase ERROR_SUCCESS: error = 0; break;\n> ++\tcase ERROR_SUCCESS: BUG(\"err_win_to_posix() called without an error!\");\n> \tcase ERROR_SWAPERROR: error = ENOENT; break;\n> \tcase ERROR_TOO_MANY_MODULES: error = EMFILE; break;\n> \tcase ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;\n> \n\n-- Hannes\n"},{"id":"387410","messageId":"xmqqzhgawnui.fsf@gitster-ct.c.googlers.com","threadId":"52372","inReplyTo":"a4ac4530-9b5c-3eb6-ada2-067c6ef73868@kdbg.org","subject":"Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-02T19:04:37Z","receivedAt":"2019-12-02T19:04:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:\n>> Range-diff vs v2:\n>> \n>> 1: 280b6d08a4 = 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n>> 2: c3dea00fb1 ! 2: e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0\n>> @@ -3,12 +3,15 @@\n>> mingw: translate ERROR_SUCCESS to errno = 0\n>\n> The subject line would have to be adjusted as well.\n>\n> \tmingw: forbid translating ERROR_SUCCESS to an errno value\n>\n> or something.\n\nTrue. Thanks for being careful.\n"},{"id":"387438","messageId":"nycvar.QRO.7.76.6.1912031303250.31080@tvgsbejvaqbjf.bet","threadId":"52372","inReplyTo":"a4ac4530-9b5c-3eb6-ada2-067c6ef73868@kdbg.org","subject":"Re: [PATCH v3 0/2] Brown-bag fix on top of js/mingw-inherit-only-std-handles","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-03T12:04:45Z","receivedAt":"2019-12-03T12:05:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 2 Dec 2019, Johannes Sixt wrote:\n\n> Am 02.12.19 um 12:33 schrieb Johannes Schindelin via GitGitGadget:\n> > Range-diff vs v2:\n> >\n> > 1: 280b6d08a4 = 1: 280b6d08a4 mingw: do set `errno` correctly when trying to restrict handle inheritance\n> > 2: c3dea00fb1 ! 2: e04e1269b3 mingw: translate ERROR_SUCCESS to errno = 0\n> > @@ -3,12 +3,15 @@\n> > mingw: translate ERROR_SUCCESS to errno = 0\n>\n> The subject line would have to be adjusted as well.\n>\n> \tmingw: forbid translating ERROR_SUCCESS to an errno value\n>\n> or something.\n\nOy. Thank you for catching this! (It is always amazing to me how much I\nmiss when I have stared at the same commits for a while.)\n\nFor good measure, I force-pushed the branch of the PR to match what Junio\nhas in `pu`, but if there are no other changes I need to make, I will\nrefrain from submitting another iteration.\n\nThanks,\nDscho\n\n>\n> >\n> > Johannes Sixt pointed out that the `err_win_to_posix()` function\n> > - mishandles `ERROR_SUCCESS`. This commit fixes that.\n> > + mishandles `ERROR_SUCCESS`: it maps it to `ENOSYS`.\n> >\n> > - Technically, we try to only assign `errno` to the corresponding value of\n> > - `GetLastError()` (which translation is performed by that function) when\n> > - a Win32 API call failed, so this change is purely defensive and is not\n> > - expected to fix an existing bug in our code base.\n> > + The only purpose of this function is to map Win32 API errors to `errno`\n> > + ones, and there is actually no equivalent to `ERROR_SUCCESS`: the idea\n> > + of `errno` is that it will only be set in case of an error, and left\n> > + alone in case of success.\n> > +\n> > + Therefore, as pointed out by Junio Hamano, it is a bug to call this\n> > + function when there was not even any error to map.\n> >\n> > Signed-off-by: Johannes Schindelin \n> >\n> > @@ -19,7 +22,7 @@\n> > \tcase ERROR_SHARING_BUFFER_EXCEEDED: error = ENFILE; break;\n> > \tcase ERROR_SHARING_VIOLATION: error = EACCES; break;\n> > \tcase ERROR_STACK_OVERFLOW: error = ENOMEM; break;\n> > -+\tcase ERROR_SUCCESS: error = 0; break;\n> > ++\tcase ERROR_SUCCESS: BUG(\"err_win_to_posix() called without an error!\");\n> > \tcase ERROR_SWAPERROR: error = ENOENT; break;\n> > \tcase ERROR_TOO_MANY_MODULES: error = EMFILE; break;\n> > \tcase ERROR_TOO_MANY_OPEN_FILES: error = EMFILE; break;\n> >\n>\n> -- Hannes\n>\n"}]}