From 96bfb2d8ce221d31b3da08a49a455e316dbef7fb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 22 Nov 2021 16:28:13 -0500 Subject: run-command: unify signal and regular logic for wait_or_whine() Since 507d7804c0 (pager: don't use unsafe functions in signal handlers, 2015-09-04), we have a separate code path in wait_or_whine() for the case that we're in a signal handler. But that code path misses some of the cases handled by the main logic. This was improved in be8fc53e36 (pager: properly log pager exit code when signalled, 2021-02-02), but that covered only case: actually returning the correct error code. But there are some other cases: - if waitpid() returns failure, we wouldn't notice and would look at uninitialized garbage in the status variable; it's not clear if it's possible to trigger this or not - if the process exited by signal, then we would still report "-1" rather than the correct signal code This latter case even had a test added in be8fc53e36, but it doesn't work reliably. It sets the pager command to: >pager-used; test-tool sigchain The latter command will die by signal, but because there are multiple commands, there will be a shell in between. And it's the shell whose waitpid() call will see the signal death, and it will then exit with code 143, which is what Git will see. To make matters even more confusing, some shells (such as bash) will realize that there's nothing for the shell to do after test-tool finishes, and will turn it into an exec. So the test was only checking what it thought when /bin/sh points to a shell like bash (we're relying on the shell used internally by Git to spawn sub-commands here, so even running the test under bash would not be enough). This patch adjusts the tests to explicitly call "exec" in the pager command, which produces a consistent outcome regardless of shell. Note that without the code change in this patch it _should_ fail reliably, but doesn't. That test, like its siblings, tries to trigger SIGPIPE in the git-log process writing to the pager, but only do so racily. That will be fixed in a follow-on patch. For the code change here, we have two options: - we can teach the in_signal code to handle WIFSIGNALED() - we can stop returning early when in_signal is set, and instead annotate individual calls that we need to skip in this case The former is a simpler patch, but means we're essentially duplicating all of the logic. So instead I went with the latter. The result is a bigger patch, and we do run the risk of new code being added but forgetting to handle in_signal. But in the long run it seems more maintainable. I've skipped any non-trivial calls for the in_signal case, like calling error(). We'll also skip the call to clear_child_for_cleanup(), as we were before. This is arguably the wrong thing to do, since we wouldn't want to try to clean it up again. But: - we can't call it as-is, because it calls free(), which we must avoid in a signal handler (we'd have to pass in_signal so it can skip the free() call) - we'll only go through the list of children to clean once, since our cleanup_children_on_signal() handler pops itself after running (and then re-raises, so eventually we'd just exit). So this cleanup only matters if a process is on the cleanup list _and_ it has a separate handler to clean itself up. Which is questionable in the first place (and AFAIK we do not do). - double-cleanup isn't actually that bad anyway. waitpid() will just return an error, which we won't even report because of in_signal. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- run-command.c | 19 +++++++++---------- t/t7006-pager.sh | 2 +- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/run-command.c b/run-command.c index 509841bf27..350593928d 100644 --- a/run-command.c +++ b/run-command.c @@ -551,20 +551,17 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR) ; /* nothing */ - if (in_signal) { - if (WIFEXITED(status)) - code = WEXITSTATUS(status); - return code; - } if (waiting < 0) { failed_errno = errno; - error_errno("waitpid for %s failed", argv0); + if (!in_signal) + error_errno("waitpid for %s failed", argv0); } else if (waiting != pid) { - error("waitpid is confused (%s)", argv0); + if (!in_signal) + error("waitpid is confused (%s)", argv0); } else if (WIFSIGNALED(status)) { code = WTERMSIG(status); - if (code != SIGINT && code != SIGQUIT && code != SIGPIPE) + if (!in_signal && code != SIGINT && code != SIGQUIT && code != SIGPIPE) error("%s died of signal %d", argv0, code); /* * This return value is chosen so that code & 0xff @@ -575,10 +572,12 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal) } else if (WIFEXITED(status)) { code = WEXITSTATUS(status); } else { - error("waitpid is confused (%s)", argv0); + if (!in_signal) + error("waitpid is confused (%s)", argv0); } - clear_child_for_cleanup(pid); + if (!in_signal) + clear_child_for_cleanup(pid); errno = failed_errno; return code; diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 0e7cf75435..032bde278e 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -767,7 +767,7 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' ' test_when_finished "rm pager-used trace.normal" && - test_config core.pager ">pager-used; test-tool sigchain" && + test_config core.pager ">pager-used; exec test-tool sigchain" && GIT_TRACE2="$(pwd)/trace.normal" && export GIT_TRACE2 && test_when_finished "unset GIT_TRACE2" && -- cgit v1.2.3 From f7991f01f2d1bc800a47adcf66a1b29a1f7cf697 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 21 Nov 2021 17:54:39 -0500 Subject: t7006: clean up SIGPIPE handling in trace2 tests Comit c24b7f6736 (pager: test for exit code with and without SIGPIPE, 2021-02-02) introduced some tests that don't reliably generate SIGPIPE where we expect it (i.e., when our pager doesn't read all of the output from git-log). There are two problems that somewhat cancel each other out. First is that the output of git-log isn't very large (only around 800 bytes). So even if the pager doesn't read all of our output, it's racy whether or not we'll actually get a SIGPIPE (we won't if we write all of the output into the pipe buffer before the pager exits). But we wrap git-log with test_terminal, which is supposed to propagate the exit status of git-log. However, it doesn't always do so; test_terminal will copy to stdout any lines that it got from our fake pager, and it pipes to an empty command. So most of the time we are seeing a SIGPIPE from test_terminal itself (though this is likewise racy). Let's try to make this more robust in two ways: 1. We'll put a commit with a huge message at the tip of history. Since this is over a megabyte, it should fill the OS pipe buffer completely, causing git-log to keep trying to write even after the pager has exited. 2. We'll redirect the output of test_terminal to /dev/null. That means it can never get SIGPIPE itself, and will always be giving us the exit code from git-log. These two changes reveal that one of the tests was looking for the wrong behavior. If we try to start a pager that does not exist (according to execve()), then the error propagates from start_command() back to the pager code as an error, and we avoid redirecting git-log's stdout to the broken pager entirely. Instead, it goes straight to the original stdout (test_terminal's pty in this case), and we do not see a SIGPIPE at all. So the test "git attempts to page to nonexisting pager command, gets SIGPIPE" is checking the wrong outcome; it should be looking for a successful exit (and was only confused by test_terminal's SIGPIPE). There's a related test, "git discards nonexisting pager without SIGPIPE", which sets the pager to a shell command which will read all input and _then_ run a non-existing command. But that doesn't trigger the same execve() behavior. We really do run the shell there and redirect git-log's stdout to it. And the fact that the shell then exits 127 is not interesting. It is not different at that point than the earlier test to check for "exit 1". So we can drop that test entirely. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7006-pager.sh | 42 ++++++++++++++---------------------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 032bde278e..c980e8dca5 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -661,6 +661,13 @@ test_expect_success 'setup trace2' ' export GIT_TRACE2_BRIEF ' +test_expect_success 'setup large log output' ' + perl -e " + print \"this is a long commit message\" x 50000 + " >commit-msg && + git commit --allow-empty -F commit-msg +' + test_expect_success TTY 'git returns SIGPIPE on early pager exit' ' test_when_finished "rm pager-used trace.normal" && test_config core.pager ">pager-used; head -n 1; exit 0" && @@ -670,7 +677,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && test_match_signal 13 "$OUT" else test_terminal git log @@ -691,7 +698,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && test_match_signal 13 "$OUT" else test_terminal git log @@ -712,7 +719,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && test "$OUT" -eq 0 else test_terminal git log @@ -724,28 +731,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' ' test_path_is_file pager-used ' -test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' ' - test_when_finished "rm pager-used trace.normal" && - test_config core.pager "wc >pager-used; does-not-exist" && - GIT_TRACE2="$(pwd)/trace.normal" && - export GIT_TRACE2 && - test_when_finished "unset GIT_TRACE2" && - - if test_have_prereq !MINGW - then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && - test "$OUT" -eq 0 - else - test_terminal git log - fi && - - grep child_exit trace.normal >child-exits && - test_line_count = 1 child-exits && - grep " code:127 " child-exits && - test_path_is_file pager-used -' - -test_expect_success TTY 'git attempts to page to nonexisting pager command, gets SIGPIPE' ' +test_expect_success TTY 'git skips paging nonexisting command' ' test_when_finished "rm trace.normal" && test_config core.pager "does-not-exist" && GIT_TRACE2="$(pwd)/trace.normal" && @@ -754,8 +740,8 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && - test_match_signal 13 "$OUT" + OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && + test "$OUT" -eq 0 else test_terminal git log fi && @@ -774,7 +760,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) | :) 3>&1 ) && + OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && test_match_signal 13 "$OUT" else test_terminal git log -- cgit v1.2.3 From 5263e22cba8dce3a579bec41ac576f74bf4258df Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sun, 21 Nov 2021 18:10:56 -0500 Subject: t7006: simplify exit-code checks for sigpipe tests Some tests in t7006 check for a SIGPIPE result by recording $? and comparing it with test_match_signal. Before the previous commit, the command was on the left-hand side of a pipe, and so we had to do some subshell trickery to extract it. But now that this is no longer the case, we can do things much more simply: just run the command directly, using braces to avoid wrecking the &&-chain, and then record $?. We could almost use test_expect_code here, but it doesn't know about test_match_signal. Likewise, for tests which expect success (i.e., not SIGPIPE), we can just put them in the &&-chain as usual. That even lets us get rid of the !MINGW check, since the expectation is the same on both sides. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t7006-pager.sh | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index c980e8dca5..a87ef37803 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -677,7 +677,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && + { test_terminal git log >/dev/null; OUT=$?; } && test_match_signal 13 "$OUT" else test_terminal git log @@ -698,7 +698,7 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && + { test_terminal git log >/dev/null; OUT=$?; } && test_match_signal 13 "$OUT" else test_terminal git log @@ -717,13 +717,7 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' ' export GIT_TRACE2 && test_when_finished "unset GIT_TRACE2" && - if test_have_prereq !MINGW - then - OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && - test "$OUT" -eq 0 - else - test_terminal git log - fi && + test_terminal git log && grep child_exit trace.normal >child-exits && test_line_count = 1 child-exits && @@ -738,13 +732,7 @@ test_expect_success TTY 'git skips paging nonexisting command' ' export GIT_TRACE2 && test_when_finished "unset GIT_TRACE2" && - if test_have_prereq !MINGW - then - OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && - test "$OUT" -eq 0 - else - test_terminal git log - fi && + test_terminal git log && grep child_exit trace.normal >child-exits && test_line_count = 1 child-exits && @@ -760,7 +748,7 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' ' if test_have_prereq !MINGW then - OUT=$( ((test_terminal git log; echo $? 1>&3) >/dev/null) 3>&1 ) && + { test_terminal git log >/dev/null; OUT=$?; } && test_match_signal 13 "$OUT" else test_terminal git log -- cgit v1.2.3