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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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.

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

[Jeff King]

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 <peff@peff.net>
---
 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And here's the fix.

I was tempted to suggest the change in your patch.

With ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,
2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE
for run-command.c (but I do get "ignoring return value of 'fwrite' from
many places).  Perhaps the kinds of checks done by versions of gcc you,
Jonathan and I use are different.

I'd rather revert it for now; I don't want to see patch ping-pong at this
late in the pre-release cycle.

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

[Jeff King]

On Mon, Apr 18, 2011 at 02:40:31PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > And here's the fix.
> 
> I was tempted to suggest the change in your patch.
> 
> With ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,
> 2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE
> for run-command.c (but I do get "ignoring return value of 'fwrite' from
> many places).  Perhaps the kinds of checks done by versions of gcc you,
> Jonathan and I use are different.

I don't use _FORTIFY_SOURCE at all, so I have no clue. I just saw that
the code in ebec842 is obviously wrong, and the fix looked equally
obvious.

> I'd rather revert it for now; I don't want to see patch ping-pong at this
> late in the pre-release cycle.

That's your call, but the fix seems dead simple to me. _FORTIFY_SOURCE
likes the conditional, according to Jonathan's patch. We don't remove
the conditional, just the wrong "non-zero is an error" assumption. So I
wouldn't expect any ping-pong on it, but then again, it looked like a
pretty innocuous patch in the first place, and held a pretty nasty and
surprising bug. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ..., but then again, it looked like a
> pretty innocuous patch in the first place, and held a pretty nasty and
> surprising bug. :)

Exactly.  It was quite embarrassing.

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

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> With ebec842 (run-command: prettify -D_FORTIFY_SOURCE workaround,
> 2011-03-16) reverted, I still don't get complaints from -D_FORTIFY_SOURCE
> for run-command.c (but I do get "ignoring return value of 'fwrite' from
> many places).  Perhaps the kinds of checks done by versions of gcc you,
> Jonathan and I use are different.

It's not about fortify warnings, but "set but not used" warnings.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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.
>
> ...
> 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.

And that is the topic of this follow-up.

> 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-hello was found but was not executable; or
>
>  (2) when we cannot read $T and we cannot even tell $T/git-hello exists or
>      not.
>
> So I think it is Ok to be silent only when we see ENOENT like the current
> code does.

Unfortunately, as we are giving "git-hello" to underlying execvp(3),
instead of splitting $PATH, prefixing each element of it in front of
"git-hello" and trying execv() in a loop, when the call fails, we do not
know which component of $PATH caused execvp() to fail, and end up saying
"fatal: cannot exec 'git-hello': Permission denied".  So in that sense, we
are not helping the end user by making it easier to diagnose the problem.

We would need to emulate what execvp() does ourselves (i.e. split $PATH,
prefix each component and try execv(), ignoring ENOENT or EACCES while
trying next component in $PATH), plus note the first path that got EACCES
so that we can report which script (including its leading directories) had
trouble executing.  Perhaps a simple enough task for beginners.

[PATCH] run-command: write full error message in die_child

[Jonathan Nieder]

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

[PATCH] report which $PATH entry had trouble running execvp(3)

[Junio C Hamano]

You can add your own custom subcommand 'frotz' to the system by adding
'git-frotz' in a directory somewhere in your $PATH environment variable.
When you ask "git frotz" from the command line, "git-frotz" is run via
execvp(3).

Three plausible scenarios that the execvp(3) would fail for us are:

 * The first 'git-frotz' found in a directory on $PATH was not a proper
   executable binary, and we got "Exec format error" (ENOEXEC);

 * The only 'git-frotz' found in the directories listed on $PATH were not
   marked with executable bit, and we got "Permission denied" (EACCES); or

 * No 'git-frotz' was found in the directories listed on $PATH, but one of
   the directories were unreadable, and we got EACCES.

The first one is easy to understand and to rectify.  Most likely, the user
made a typo, either on the command line, or when creating the custom
subcommand.  However, the latter two cases are harder to notice, as we do
not report 'git-frotz' in which directory we had trouble with.  We could
do better if we implemented the command search behaviour of execvp(3)
ourselves.

Add an internal function sane_execvp() that emulates execvp(3), skipping
ENOENT and EACCES while remembering a path that resulted in EACCES while
trying later directories on $PATH.  When failing the request at the end,
report the path that we had trouble with, and use it when reporting the
error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Junio C Hamano <gitster@pobox.com> writes:

  >> The following is a tangent that was brought up at $work.
  > ...
  > We would need to emulate what execvp() does ourselves (i.e. split $PATH,
  > prefix each component and try execv(), ignoring ENOENT or EACCES while
  > trying next component in $PATH), plus note the first path that got EACCES
  > so that we can report which script (including its leading directories) had
  > trouble executing.  Perhaps a simple enough task for beginners.

 run-command.c |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index f91e446..4c95f50 100644
--- a/run-command.c
+++ b/run-command.c
@@ -135,6 +135,52 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 	return code;
 }
 
+static const char *sane_execvp(const char *arg0, const char **argv)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf failed_path = STRBUF_INIT;
+	char *path = getenv("PATH");
+	char *next;
+
+	if (!path)
+		path = "";
+
+	for (;;) {
+		next = strchrnul(path, ':');
+		if (path < next)
+			strbuf_add(&sb, path, next - path);
+		else
+			strbuf_addch(&sb, '.');
+		if (sb.len && sb.buf[sb.len - 1] != '/')
+			strbuf_addch(&sb, '/');
+		strbuf_addstr(&sb, arg0);
+		execv(sb.buf, (char * const*) argv);
+
+		/*
+		 * execvp() skips EACCES and ENOENT and goes on to try
+		 * the next entry in the $PATH, but sets errno to EACCES
+		 * when it fails at the end.
+		 */
+		if (errno == EACCES && !failed_path.len)
+			strbuf_add(&failed_path, sb.buf, sb.len);
+		if (errno != ENOENT) {
+			strbuf_release(&failed_path);
+			return strbuf_detach(&sb, NULL);
+		}
+		strbuf_release(&sb);
+		if (!*next)
+			break;
+		path = next + 1;
+	}
+	if (failed_path.len) {
+		errno = EACCES;
+		return strbuf_detach(&failed_path, NULL);
+	}
+	strbuf_release(&sb);
+	strbuf_release(&failed_path);
+	return arg0;
+}
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -278,7 +324,7 @@ fail_pipe:
 		} else if (cmd->use_shell) {
 			execv_shell_cmd(cmd->argv);
 		} else {
-			execvp(cmd->argv[0], (char *const*) cmd->argv);
+			cmd->argv[0] = sane_execvp(cmd->argv[0], cmd->argv);
 		}
 		/*
 		 * Do not check for cmd->silent_exec_failure; the parent

Re: [PATCH] report which $PATH entry had trouble running execvp(3)

[Jeff King]

On Tue, Apr 19, 2011 at 09:01:21PM -0700, Junio C Hamano wrote:

> You can add your own custom subcommand 'frotz' to the system by adding
> 'git-frotz' in a directory somewhere in your $PATH environment variable.
> When you ask "git frotz" from the command line, "git-frotz" is run via
> execvp(3).
> [...]
> we do not report 'git-frotz' in which directory we had trouble with.
> We could do better if we implemented the command search behaviour of
> execvp(3) ourselves.

I like the idea of giving the user more information about which
git-frotz was the problem. Usually there is just one, and pointing them
to it saves them time.

But what about the case of

  mkdir one two
  touch one/frotz two/frotz
  PATH=one:two:$PATH

We would report two/frotz, but might it be even better to say "we found
2 frotzes, but neither of them were executable"?

I don't know if it is worth the effort for such a weird corner case.

> Three plausible scenarios that the execvp(3) would fail for us are:
> 
>  * The first 'git-frotz' found in a directory on $PATH was not a proper
>    executable binary, and we got "Exec format error" (ENOEXEC);

What about the magic "unknown things get executed as shell scripts"
behavior that is implemented by libc's execvp? Your patch has a
regression for:

  echo "git log --with-some-options" >local/bin/git-frotz
  chmod +x local/bin/git-frotz
  git frotz

I have always found that behavior slightly insane, but it is
well-established, and your sane_execvp breaks anybody who is depending
on it.

> @@ -278,7 +324,7 @@ fail_pipe:
>  		} else if (cmd->use_shell) {
>  			execv_shell_cmd(cmd->argv);
>  		} else {
> -			execvp(cmd->argv[0], (char *const*) cmd->argv);
> +			cmd->argv[0] = sane_execvp(cmd->argv[0], cmd->argv);
>  		}
>  		/*
>  		 * Do not check for cmd->silent_exec_failure; the parent

This is inside "#ifndef WIN32". Presumably people on Windows want it,
too.  In fact, they already have their own execvp in compat/mingw.c. It
might make sense to bring the implementations together. Or perhaps not.
Theirs is quite different; it does a search of PATH itself, looking for
executables (and magically appending ".exe"), and then exec's the
result. On the other hand, doing that PATH lookup, deciding you have
something, and _then_ exec'ing can be convenient. IIRC, there are a few
warts in the git wrapper that could be improved by doing that, but I
don't recall the specifics anymore (maybe something like handling the
pager between the momemnt when we decide a command exists and when we
exec?).

-Peff

Re: [PATCH] report which $PATH entry had trouble running execvp(3)

[Johannes Sixt]

Am 4/20/2011 6:01, schrieb Junio C Hamano:
> Add an internal function sane_execvp() that emulates execvp(3), skipping
> ENOENT and EACCES while remembering a path that resulted in EACCES while
> trying later directories on $PATH.  When failing the request at the end,
> report the path that we had trouble with, and use it when reporting the
> error.

I don't think this is worth the trouble. In which way is git different
from other tools that execvp other programs?

And how do you help when the script is executable, but the interpreter is not:

$ chmod -x git	# use git itself just for exposition
$ echo '#!'"$(pwd)/git" > git-frotz
$ chmod +x git-frotz
$ git --exec-path=. frotz
fatal: cannot exec 'git-frotz': Permission denied
$ # WTF, git-frotz *is* executable and readable!?!?

IOW, when you get strange behavior, you still have to dig around to know
what went wrong.

-- Hannes

Re: [PATCH] run-command: write full error message in die_child

[Johannes Sixt]

Am 4/19/2011 9:05, schrieb Jonathan Nieder:
> 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
> +

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

[PATCH v2 0/2] Re: run-command: write full error message in die_child

[Jonathan Nieder]

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(-)

[PATCH 1/2] tests: check error message from run_command

[Jonathan Nieder]

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 <jrnieder@gmail.com>
Improved-by: Johannes Sixt <j6t@kdbg.org>
---
 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

[PATCH 2/2] run-command: handle short writes and EINTR in die_child

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
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

Re: [PATCH] report which $PATH entry had trouble running execvp(3)

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> You can add your own custom subcommand 'frotz' to the system by adding
> 'git-frotz' in a directory somewhere in your $PATH environment variable.
> When you ask "git frotz" from the command line, "git-frotz" is run via
> execvp(3).
>
> Three plausible scenarios that the execvp(3) would fail for us are:
[...]
> The first one is easy to understand and to rectify.  Most likely, the user
> made a typo, either on the command line, or when creating the custom
> subcommand.  However, the latter two cases are harder to notice, as we do
> not report 'git-frotz' in which directory we had trouble with.  We could
> do better if we implemented the command search behaviour of execvp(3)
> ourselves.

My first reaction was the same as Hannes's.  I suppose I would be
happier about something like an optional dependency on something
generic like libexplain[1] (though I'm not thrilled about the style of
its error messages).  If we are to implement it ourselves, using
standard execvp and then trying to track down a guess for the cause
after it fails might be okay.

[1] http://libexplain.sourceforge.net/

I was also reminded that anyone writing scripts following the advice
of POSIX (meaning no #!) would find their custom git commands broken.
Luckily that is easily fixed by using execvp with absolute path.

A part of this is tempting: as Jeff mentioned, it would be nice to
avoid commit_pager_choice when checking for a dashed external before
executing an alias to an internal command that doesn't want a pager
(see v1.7.2~16^2, git --paginate: paginate external commands again,
2010-07-14).  Hm.

Re: [PATCH] report which $PATH entry had trouble running execvp(3)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 19, 2011 at 09:01:21PM -0700, Junio C Hamano wrote:
>
>> You can add your own custom subcommand 'frotz' to the system by adding
>> 'git-frotz' in a directory somewhere in your $PATH environment variable.
>> When you ask "git frotz" from the command line, "git-frotz" is run via
>> execvp(3).
>> [...]
>> we do not report 'git-frotz' in which directory we had trouble with.
>> We could do better if we implemented the command search behaviour of
>> execvp(3) ourselves.
>
> I like the idea of giving the user more information about which
> git-frotz was the problem. Usually there is just one, and pointing them
> to it saves them time.
>
> But what about the case of
>
>   mkdir one two
>   touch one/frotz two/frotz
>   PATH=one:two:$PATH
>
> We would report two/frotz, but might it be even better to say "we found
> 2 frotzes, but neither of them were executable"?

No, one/frotz is the first one found along $PATH, and we report that we
cannot exec 'one/frotz'.  We are trying to imitate the semantics of the
usual command search done by execvp() and by the shell.  Three possible
user reactions are (1) Huh? I wanted to see two/frotz be used.  My $PATH
is wrong, and I'll fix it by reordering elements on $PATH; (2) Huh? I
wanted to see two/frotz be used. I have a stale one in one/frotz, and I'll
fix it by removing it; and (3) Yuck, I forgot to chmod +x one/frotz.

As J6t mentioned, the user needs to examine what went wrong anyway.  The
point of the change is to make it easier by giving more information than
what execvp(3) gives us (especially there may be hidden GIT_EXEC_PATH that
the user of a third-party scripted Porcelain may not be aware of, which is
tacked before the usual $PATH).

>> Three plausible scenarios that the execvp(3) would fail for us are:
>> 
>>  * The first 'git-frotz' found in a directory on $PATH was not a proper
>>    executable binary, and we got "Exec format error" (ENOEXEC);
>
> What about the magic "unknown things get executed as shell scripts"
> behavior that is implemented by libc's execvp?
> ...
> I have always found that behavior slightly insane, but it is
> well-established, and your sane_execvp breaks anybody who is depending
> on it.

We can choose to add that on top of the sane_execvp() patch.