[PATCH] run-command: write full error message in die_child
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 19, 2011, 07:05 UTC
- Message-ID
- <20110419070510.GB28291@elie>
- In-Reply-To
- <7v4o5v8dlp.fsf@alter.siamese.dyndns.org>
The run_command facility writes 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 then make it non-executable, this is what I get from 'maint':
$ chmod a-x git-hello $ git hello fatal: cannot exec 'git-hello': Permission denied
But with 'master', we get disturbing output:
$ PATH=.:$PATH git hello fatal: $
That is a regression introduced by v1.7.5-rc0~29^2 (run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which uses the construct "if (write(...) || write(...) || write(...))" to perform some writes in sequence, with the "if" body acknowledging errors from them once. write does not return 0 on success, so only the first write succeeds. Oops.
While fixing the above, let's actually pay attention to the return value and handle partial writes. write_in_full has the desired semantics --- it loops until the desired number of bytes have been written and on error it returns -1 to let us handle the error.
The "if" to appease warn_unused_result is no longer necessary after this patch since xwrite and write_in_full check the return value from write(2), but we leave it in for clarity and for robustness against future static analyzers.
Reported-by: Junio C Hamano <gitster@pobox.com> Analysis-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- Junio C Hamano wrote:
> I'm reverting it for now, but am open to a submission of a proper fix > after 1.7.5.
Knowing myself, I'm likely to forget to submit a fix later. So here's a patch to consider applying after 1.7.5.
Based directly against ebec84277 (run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16). The "grep" in the test case should be test_i18ngrep if applying to a gettextized git.
Sorry for the breakage.
run-command.c | 8 ++++---- t/t0061-run-command.sh | 24 ++++++++++++++++++++++++ test-run-command.c | 2 ++ 3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/run-command.c b/run-command.c index 8619c76..3e2ce2a 100644 --- a/run-command.c +++ b/run-command.c @@ -72,7 +72,7 @@ static void notify_parent(void) * know, so failures like ENOENT can be handled right away; but * otherwise, finish_command will still report the error. */ - if (write(child_notifier, "", 1)) + if (xwrite(child_notifier, "", 1) < 0) ; /* yes, dear gcc -D_FORTIFY_SOURCE, there was an error. */ } @@ -83,9 +83,9 @@ static NORETURN void die_child(const char *err, va_list params) if (len > sizeof(msg)) len = sizeof(msg); - if (write(child_err, "fatal: ", 7) || - write(child_err, msg, len) || - write(child_err, "\n", 1)) + if (write_in_full(child_err, "fatal: ", 7) < 0 || + write_in_full(child_err, msg, len) < 0 || + write_in_full(child_err, "\n", 1) < 0) ; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */ exit(128); } diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index 10b26e4..be602fd 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -7,8 +7,32 @@ test_description='Test run command' . ./test-lib.sh +cat >hello-script <<-EOF + #!$SHELL_PATH + echo hello +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' ' + echo hello >expect && + cat hello-script >hello.sh && + chmod +x hello.sh && + test-run-command run-command ./hello.sh >actual 2>err && + + test_cmp expect actual && + test_cmp empty err +' + +test_expect_success POSIXPERM,SANITY '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 0612bfa..37918e1 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;
-- 1.7.5.rc2