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

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.

The following is a tangent that was brought up at $work.

Some people might argue that we should skip $T/git-hello in the last case
and try to find git-hello in a later directory listed in $PATH, but I do
not personally think that is a right thing to do.  It would make the
problem harder to diagnose, and more importantly, the fact that the user
listed $T earlier in the $PATH is a strong indication that the user wants
the scripts in $T override the scripts with the same name in directories
that appear later in the $PATH, and we should report when that is not
happening, either

 (1) when $T/git-st was found but was not executable; or

 (2) when we cannot read $T and we cannot even tell $T/git-st exists or
     not.

So I think it is Ok to be silent only when we see ENOENT like the current
code does.

I am somewhat sympathetic to the case (2) above, but not sympathetic
enough to suggest changing the current behaviour.  In fact, I would say
if we treat EACCES the same way as we treat ENOENT, it would be a bug.

When your $HOME is mounted over NFS on two different machines, it is
perfectly fine to have a directory that exists on one machine but not on
other machines in $PATH, and it is reasonable to expect such a directory
to be skipped silently without complaints.

That situation, with a small stretch of imagination, can be extended to a
case where a directory early in your $PATH that you are using on one
machine for your private git-script correctly on one machine is owned by
somebody else, used for other purposes, and most importantly you have no
control on it on another machine, and you could argue that these two cases
are similar.

It is _not_ quite similar, though.  Such an "early path component is a
random place I do not control" arrangement is a total security risk, and
we shouldn't be bending backwards to support it.  Instead, we should be
actively discouraging it.  That is why I said I am not sympathetic enough
above.

On Mon, Apr 18, 2011 at 01:54:54PM -0700, Junio C Hamano wrote:

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: $

The good news is that the bug is trivial. It bisects Jonathan's ebec842
(run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which
introduces:

-       unused = write(child_err, "fatal: ", 7);
-       unused = write(child_err, msg, len);
-       unused = write(child_err, "\n", 1);
+       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. */

Stare at that for a minute and see if you can guess what's wrong. :)

-Peff

On Mon, Apr 18, 2011 at 05:11:02PM -0400, Jeff King wrote:

The good news is that the bug is trivial. It bisects Jonathan's ebec842
(run-command: prettify -D_FORTIFY_SOURCE workaround, 2011-03-16), which
introduces:

-       unused = write(child_err, "fatal: ", 7);
-       unused = write(child_err, msg, len);
-       unused = write(child_err, "\n", 1);
+       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. */

Stare at that for a minute and see if you can guess what's wrong. :)

And here's the fix.

-- >8 --
Subject: [PATCH] run-command: fix broken error messages from child

After we fork, we try to exec the child; if exec fails, we
write an error message and exit. We ignore the return value
of write, since there's nothing we can do about it.

Commit ebec842 turned this into a conditional to make
-D_FORTIFY_SOURCE happy with the ignored return value, but
botched the change so that we never write more than
"fatal:".

Write will return the number of bytes written, so the
conditional as written will always appear as an error. Of
course we don't actually do anything for the error, but
the short-circuit logic means we never execute the
subsequent write()s, giving us a truncated error message.

Signed-off-by: Jeff King <redacted>
---
 run-command.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 8619c76..508a4c6 100644
--- a/run-command.c
+++ b/run-command.c

@@ -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(child_err, "fatal: ", 7) < 0 ||
+	    write(child_err, msg, len) < 0 ||
+	    write(child_err, "\n", 1) < 0)
 		; /* yes, gcc -D_FORTIFY_SOURCE, we know there was an error. */
 	exit(128);
 }

-- 
1.7.5.rc2.3.g728b2