Re: [REGRESSION] git-wrapper to run-commands codepath regression

Junio C Hamano [off-list ref] writes:

There appears to be a regression in the codepath between git wrapper and
run_commands API.

	$ T=/var/tmp/test-commands
	$ mkdir $T
	$ cat >$T/git-hello <<\-EOF
	#!/bin/sh
	echo hello
	EOF
	$ chmod +x $T/git-hello
	$ oPATH=$PATH
	$ PATH=$T:$PATH
	$ export PATH
	$ git hello
	hello

So far, I added a "hello" subcommand to "git", and it runs correctly.

Now, when I make the script non-executable, this is what I get from
'maint':

	$ chmod a-x $T/git-hello
	$ git hello
	fatal: cannot exec 'git-hello': Permission denied

But with 'master', we get a disturbing output:

	$ git hello
        fatal: $

Note that we can observe the same regression if you instead make $T
unreadable with:

	$ chmod 755 $T/git-hello ;# make it executable again
	$ chmod a-rwx $T ;# but that directory cannot be read
        $ git hello

So that is the "regression" part.

This bisects down to ebec842 (run-command: prettify -D_FORTIFY_SOURCE
workaround, 2011-03-16).

And we should really have been more careful.  Look at what the patch does:

    Sometimes when there is an output error, especially right before exit,
    there really is nothing to be done.  The obvious solution, adopted in
    v1.7.0.3~20^2 (run-command.c: fix build warnings on Ubuntu,
    2010-01-30), is to save the return value to a dummy variable:
    
    	ssize_t dummy;
    	dummy = write(...);
    
    But that (1) is ugly and (2) triggers -Wunused-but-set-variable
    warnings with gcc-4.6 -Wall, so we are not much better off than when
    we started.

    Instead, use an "if" statement with an empty body to make the intent
    clear.
    
    	if (write(...))
    		; /* yes, yes, there was an error. */
    
No, a non-zero return is not an error from the write(2) system call.
I cannot believe both of us didn't spot it.  What were we smoking?

I'm reverting it for now, but am open to a submission of a proper fix
after 1.7.5.

Junio C Hamano wrote:

    	if (write(...))
    		; /* yes, yes, there was an error. */

No, a non-zero return is not an error from the write(2) system call.
I cannot believe both of us didn't spot it.  What were we smoking?

Yagh.

	if (write(child_err, "fatal: ", 7) ||
	    write(child_err, msg, len) ||
	    write(child_err, "\n", 1))
		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */

There are two unusual conditions in which this could fail:

 - it doesn't write anything at all, in which case the return value
   is -1.
 - a partial write, for example if writing to an almost-full pipe.

I suppose in a calmer time, a better fix will look like

	if (write_in_full(child_err, "fatal: ", 7) != 7 ||
	    write_in_full(child_err, msg, len) != len ||
	    write_in_full(child_err, "\n", 1) != 1)
		/* yes, yes, ...

and gcc will have told us something potentially useful.

Thanks for catching it.

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 <redacted>
Analysis-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
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

Am 4/19/2011 9:05, schrieb Jonathan Nieder:

Unfortunately, on Windows, the bash spawnd by git converts LF to CRLF...

 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 &&

... therefore, we fail here. Can we have this squashed in, because 'cat'
leaves LFs alone?

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index be602fd..979b478 100755
--- a/t/t0061-run-command.sh
+++ b/t/t0061-run-command.sh

@@ -9,7 +9,7 @@ test_description='Test run command'
 
 cat >hello-script <<-EOF
 	#!$SHELL_PATH
-	echo hello
+	cat hello-script
 EOF
 >empty
 

@@ -18,12 +18,11 @@ test_expect_success 'start_command reports ENOENT' '
 '
 
 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 hello-script actual &&
 	test_cmp empty err
 '
 

+test_expect_success POSIXPERM,SANITY 'run_command reports EACCES' '

Thanks for this detail (POSIXPERM). It's required. I did not check whether
SANITY is really needed; I trust you did.

-- Hannes

Johannes Sixt wrote:

Unfortunately, on Windows, the bash spawnd by git converts LF to CRLF...

[...]

... therefore, we fail here. Can we have this squashed in, because 'cat'
leaves LFs alone?

Thanks for catching this...

[...]

I did not check whether
SANITY is really needed; I trust you did.

... and this.  No, SANITY is not needed.

Here's a reroll, on top of v1.7.5-rc3~2 (Revert "run-command: prettify
-D_FORTIFY_SOURCE workaround", 2011-04-18).  It even applies on maint
this way (not that anyone would need that :)).

Jonathan Nieder (2):
  tests: check error message from run_command
  run-command: handle short writes and EINTR in die_child

 run-command.c          |   15 +++++++++------
 t/t0061-run-command.sh |   23 +++++++++++++++++++++++
 test-run-command.c     |    2 ++
 3 files changed, 34 insertions(+), 6 deletions(-)

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 <redacted>
Improved-by: Johannes Sixt [off-list ref]
---
 t/t0061-run-command.sh |   23 +++++++++++++++++++++++
 test-run-command.c     |    2 ++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
index 10b26e4..8d4938f 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 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

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 <redacted>
---
Changes from v1:

 - rewrite the commit message from the pov of a person who does not
   care about this patch's origin as a brown paper bag

 - drop the "if"s.  If the fussy compiler/library combination was
   right in some strange sense after all, then it does not make much
   sense to take the opportunity to make another token effort to
   appease it as a preventative step.

 run-command.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index f91e446..70e8a24 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

-- 
1.7.5.rc2