From c0f19bf3b9c42036722396ee26d2c173d6abf761 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 20 Apr 2011 05:35:08 -0500 Subject: [PATCH 1/2] tests: check error message from run_command In git versions starting at v1.7.5-rc0~29^2 until v1.7.5-rc3~2 (Revert "run-command: prettify -D_FORTIFY_SOURCE workaround", 2011-04-18) fixed it, the run_command facility would write a truncated error message when the command is present but cannot be executed for some other reason. For example, if I add a 'hello' command to git: $ echo 'echo hello' >git-hello $ chmod +x git-hello $ PATH=.:$PATH git hello hello and make it non-executable, this is what I normally get: $ chmod -x git-hello $ git hello fatal: cannot exec 'git-hello': Permission denied But with the problematic versions, we get disturbing output: $ PATH=.:$PATH git hello fatal: $ Add some tests to make sure it doesn't happen again. The hello-script used in these tests uses cat instead of echo because on Windows the bash spawned by git converts LF to CRLF in text written by echo while the bash running tests does not, causing the test to fail if "echo" is used. Thanks to Hannes for noticing. Signed-off-by: Jonathan Nieder Improved-by: Johannes Sixt Signed-off-by: Junio C Hamano --- t/t0061-run-command.sh | 23 +++++++++++++++++++++++ test-run-command.c | 2 ++ 2 files changed, 25 insertions(+) diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 10b26e4d8e..8d4938f019 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -7,8 +7,31 @@ test_description='Test run command' . ./test-lib.sh +cat >hello-script <<-EOF + #!$SHELL_PATH + cat hello-script +EOF +>empty + test_expect_success 'start_command reports ENOENT' ' test-run-command start-command-ENOENT ./does-not-exist ' +test_expect_success 'run_command can run a command' ' + cat hello-script >hello.sh && + chmod +x hello.sh && + test-run-command run-command ./hello.sh >actual 2>err && + + test_cmp hello-script actual && + test_cmp empty err +' + +test_expect_success POSIXPERM 'run_command reports EACCES' ' + cat hello-script >hello.sh && + chmod -x hello.sh && + test_must_fail test-run-command run-command ./hello.sh 2>err && + + grep "fatal: cannot exec.*hello.sh" err +' + test_done diff --git a/test-run-command.c b/test-run-command.c index 0612bfa7cd..37918e15f5 100644 --- a/test-run-command.c +++ b/test-run-command.c @@ -29,6 +29,8 @@ int main(int argc, char **argv) fprintf(stderr, "FAIL %s\n", argv[1]); return 1; } + if (!strcmp(argv[1], "run-command")) + exit(run_command(&proc)); fprintf(stderr, "check usage\n"); return 1; From a111eb7808bfdb90286e54b9ccdaea4f3bec3102 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Wed, 20 Apr 2011 05:40:05 -0500 Subject: [PATCH 2/2] run-command: handle short writes and EINTR in die_child If start_command fails after forking and before exec finishes, there is not much use in noticing an I/O error on top of that. finish_command will notice that the child exited with nonzero status anyway. So as noted in v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu, 2010-01-30) and v1.7.5-rc0~29^2 (2011-03-16), it is safe to ignore errors from write in this codepath. Even so, the result from write contains useful information: it tells us if the write was cancelled by a signal (EINTR) or was only partially completed (e.g., when writing to an almost-full pipe). Let's use write_in_full to loop until the desired number of bytes have been written (still ignoring errors if that fails). As a happy side effect, the assignment to a dummy variable to appease gcc -D_FORTIFY_SOURCE is no longer needed. xwrite and write_in_full check the return value from write(2). Noticed with gcc -Wunused-but-set-variable. Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- run-command.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/run-command.c b/run-command.c index f91e446c86..70e8a249d0 100644 --- a/run-command.c +++ b/run-command.c @@ -67,21 +67,24 @@ static int child_notifier = -1; static void notify_parent(void) { - ssize_t unused; - unused = write(child_notifier, "", 1); + /* + * execvp failed. If possible, we'd like to let start_command + * know, so failures like ENOENT can be handled right away; but + * otherwise, finish_command will still report the error. + */ + xwrite(child_notifier, "", 1); } static NORETURN void die_child(const char *err, va_list params) { char msg[4096]; - ssize_t unused; int len = vsnprintf(msg, sizeof(msg), err, params); if (len > sizeof(msg)) len = sizeof(msg); - unused = write(child_err, "fatal: ", 7); - unused = write(child_err, msg, len); - unused = write(child_err, "\n", 1); + write_in_full(child_err, "fatal: ", 7); + write_in_full(child_err, msg, len); + write_in_full(child_err, "\n", 1); exit(128); } #endif