{"thread":{"id":"57353","subject":"[PATCH] perl Git.pm: don't ignore signalled failure in _cmd_close()","startedAt":"2022-02-01T20:53:01Z","lastAt":"2022-02-01T21:58:44Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"447459","messageId":"patch-1.1-86353c3b366-20220201T205218Z-avarab@gmail.com","threadId":"57353","inReplyTo":null,"subject":"[PATCH] perl Git.pm: don't ignore signalled failure in _cmd_close()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-01T20:52:52Z","receivedAt":"2022-02-01T20:53:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix misbehavior in Git.pm that dates back to the very first version of\nthe library in git.git added in b1edc53d062 (Introduce Git.pm (v4),\n2006-06-24). When we fail to execute a command we shouldn't ignore all\nsignals, those can happen e.g. if abort() is called, or if the command\nsegfaults.\n\nBecause of this we'd consider e.g. a command that died due to LSAN\nexiting with abort() successful, as is the case with the tests listed\nas running successfully with SANITIZE=leak in 9081a421a6d (checkout:\nfix \"branch info\" memory leaks, 2021-11-16). We did run them\nsuccessfully, but only because we ignored these errors.\n\nThis was then made worse by the use of \"abort_on_error=1\" for LSAN\nadded in 85b81b35ff9 (test-lib: set LSAN_OPTIONS to abort by default,\n2017-09-05). Doing that makes sense, but without providing that option\nwe'd have a \"$? >> 8\" of \"23\" on failure, with abort_on_error=1 we'll\nget \"0\".\n\nAll of our tests pass even without the SIGPIPE exception being added\nhere, but as the code appears to have been trying to ignore it let's\nkeep ignoring it for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n perl/Git.pm                                 | 21 +++++++++++++++++++--\n t/t9102-git-svn-deep-rmdir.sh               |  1 -\n t/t9123-git-svn-rebuild-with-rewriteroot.sh |  1 -\n t/t9128-git-svn-cmd-branch.sh               |  1 -\n t/t9167-git-svn-cmd-branch-subproject.sh    |  1 -\n 5 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 090a7df63fc..080cdc2a21d 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1686,6 +1686,16 @@ sub _setup_git_cmd_env {\n # by searching for it at proper places.\n sub _execv_git_cmd { exec('git', @_); }\n \n+sub _is_sig {\n+\tmy ($v, $n) = @_;\n+\n+\t# We are avoiding a \"use POSIX qw(SIGPIPE SIGABRT)\" in the hot\n+\t# Git.pm codepath.\n+\trequire POSIX;\n+\tno strict 'refs';\n+\t$v == *{\"POSIX::$n\"}->();\n+}\n+\n # Close pipe to a subprocess.\n sub _cmd_close {\n \tmy $ctx = shift @_;\n@@ -1698,9 +1708,16 @@ sub _cmd_close {\n \t\t} elsif ($? >> 8) {\n \t\t\t# The caller should pepper this.\n \t\t\tthrow Git::Error::Command($ctx, $? >> 8);\n+\t\t} elsif ($? & 127 && _is_sig($? & 127, \"SIGPIPE\")) {\n+\t\t\t# we might e.g. closed a live stream; the command\n+\t\t\t# dying of SIGPIPE would drive us here.\n+\t\t} elsif ($? & 127 && _is_sig($? & 127, \"SIGABRT\")) {\n+\t\t\tdie sprintf('BUG?: got SIGABRT ($? = %d, $? & 127 = %d) when closing pipe',\n+\t\t\t\t    $?, $? & 127);\n+\t\t} elsif ($? & 127) {\n+\t\t\tdie sprintf('got signal ($? = %d, $? & 127 = %d) when closing pipe',\n+\t\t\t\t    $?, $? & 127);\n \t\t}\n-\t\t# else we might e.g. closed a live stream; the command\n-\t\t# dying of SIGPIPE would drive us here.\n \t}\n }\n \ndiff --git a/t/t9102-git-svn-deep-rmdir.sh b/t/t9102-git-svn-deep-rmdir.sh\nindex 7b2049caa0c..946ef85eb98 100755\n--- a/t/t9102-git-svn-deep-rmdir.sh\n+++ b/t/t9102-git-svn-deep-rmdir.sh\n@@ -1,7 +1,6 @@\n #!/bin/sh\n test_description='git svn rmdir'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize repo' '\ndiff --git a/t/t9123-git-svn-rebuild-with-rewriteroot.sh b/t/t9123-git-svn-rebuild-with-rewriteroot.sh\nindex 3320b1f39cf..ead404589eb 100755\n--- a/t/t9123-git-svn-rebuild-with-rewriteroot.sh\n+++ b/t/t9123-git-svn-rebuild-with-rewriteroot.sh\n@@ -5,7 +5,6 @@\n \n test_description='git svn respects rewriteRoot during rebuild'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n mkdir import\ndiff --git a/t/t9128-git-svn-cmd-branch.sh b/t/t9128-git-svn-cmd-branch.sh\nindex 9871f5abc93..783e3ba0c53 100755\n--- a/t/t9128-git-svn-cmd-branch.sh\n+++ b/t/t9128-git-svn-cmd-branch.sh\n@@ -5,7 +5,6 @@\n \n test_description='git svn partial-rebuild tests'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize svnrepo' '\ndiff --git a/t/t9167-git-svn-cmd-branch-subproject.sh b/t/t9167-git-svn-cmd-branch-subproject.sh\nindex d9fd111c105..d8128430a8d 100755\n--- a/t/t9167-git-svn-cmd-branch-subproject.sh\n+++ b/t/t9167-git-svn-cmd-branch-subproject.sh\n@@ -5,7 +5,6 @@\n \n test_description='git svn branch for subproject clones'\n \n-TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-git-svn.sh\n \n test_expect_success 'initialize svnrepo' '\n-- \n2.35.0.913.g12b4baa2536\n\n"},{"id":"447470","messageId":"xmqqwnieb65t.fsf@gitster.g","threadId":"57353","inReplyTo":"patch-1.1-86353c3b366-20220201T205218Z-avarab@gmail.com","subject":"Re: [PATCH] perl Git.pm: don't ignore signalled failure in _cmd_close()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-01T21:58:38Z","receivedAt":"2022-02-01T21:58:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix misbehavior in Git.pm that dates back to the very first version of\n> the library in git.git added in b1edc53d062 (Introduce Git.pm (v4),\n> 2006-06-24). When we fail to execute a command we shouldn't ignore all\n> signals, those can happen e.g. if abort() is called, or if the command\n> segfaults.\n\nGood find.\n\n> All of our tests pass even without the SIGPIPE exception being added\n> here, but as the code appears to have been trying to ignore it let's\n> keep ignoring it for now.\n>\n\nSounds OK-ish to me.\n\n> +sub _is_sig {\n> +\tmy ($v, $n) = @_;\n> +\n> +\t# We are avoiding a \"use POSIX qw(SIGPIPE SIGABRT)\" in the hot\n> +\t# Git.pm codepath.\n> +\trequire POSIX;\n> +\tno strict 'refs';\n> +\t$v == *{\"POSIX::$n\"}->();\n> +}\n\n\n\n"}]}