patch, 5 partsrun-command: Elaborate execvp error checking

This patch series replaces $gmane/187026. It aims to improve the information provided after execvp fails in run-command.

This series takes a rather different approach than the previous one. In the former the focus was on determining what might cause an EACCES failure. This series focuses on trying to provide hints on what went wrong on any error.

The resulting checking produces behavior rather like bash, with the notable exception that we consider "Not found and PATH access issues" a failure, where bash doesn't.

[PATCH 1/5] t0061: Fix incorrect indentation [PATCH 2/5] t0061: Add tests [PATCH 3/5] run-command: Elaborate execvp error checking [PATCH 4/5] run-command: Warn if PATH entry cannot be searched [PATCH 5/5] run-command: Error out if interpreter not found

run-command.c | 140 +++++++++++++++++++++++++++++++++++++++++++++--- t/t0061-run-command.sh | 57 ++++++++++++++++++- 2 files changed, 186 insertions(+), 11 deletions(-)

The hello.sh script started with <tab>#!, which is not considered a correct hash-bang line. The default interpreter happened to be the one required for this specific test.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 t/t0061-run-command.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Frans Klaver <fransklaver@gmail.com> writes:

Isn't that exactly the reason why we start the here text with "<<-EOF", not the usual "<<EOF"?

Frans Klaver wrote:

Looks like a no-op --- the script already started with #! and no leading tab for me. Does it behave differently on your machine?

On Tue, 24 Jan 2012 23:40:00 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

Hurr? I'm fairly sure the script ended up being indented for me. I'll recheck.

"Frans Klaver" <fransklaver@gmail.com> writes:

It could be that your shell is broken and does not understand the distinction between the "<<-EOF" vs "<<EOF". What system are you on? If the problem is real and widespread, we might want to mention it in INSTALL or Makefile, just like we label Solaris /bin/sh as unusable and advise people to use /usr/xpg[46]/bin variant.

On Wed, 25 Jan 2012 08:00:57 +0100, Junio C Hamano <gitster@pobox.com> wrote:

Gentoo Linux, using bash. Would be hard to believe bash on plain old linux mucks this up, wouldn't it?

I'll check and notify.

On Wed, Jan 25, 2012 at 7:27 AM, Frans Klaver <fransklaver@gmail.com> wrote:

Hm, that must have been my imagination back then. I'm sure I did this for a reason. Ah well, we'll drop this patch. I'll put the tabs into the other test scripts as well, because it does make them that much more readable.

Thanks for catching.

Capture failure behavior when running into
- EACCES caused by search path permissions
- ENOENT caused by interpreter not found

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 t/t0061-run-command.sh |   50 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 49 insertions(+), 1 deletions(-)

Hi,

Frans Klaver wrote:

Thanks for writing tests. Some hints on mechanics below, and one on strategy (see (*) below).

What is the point of repreatedly writing an empty file named "empty"?

I think this would be easier to read and maintain if scripts not shared between multiple tests were written in the body of the relevant tests. For example, that way it is easier to remember to remove a helper script if the relevant test assertion changes to no longer need it.

[...]

[...]

(*) These tests would be easier to understand if squashed with the relevant later patch in the series that changes the error message.

Maybe they could be less repetitive that way, too.

	test_expect_success POSIXPERM 'diagnose command in inaccessible part of $PATH' '
		mkdir -p subdir &&
		cat hello-script >subdir/hello.sh &&
		chmod +x subdir/hello.sh &&
		chmod -x subdir &&
		(
			PATH=$(pwd)/inaccessible:$PATH &&
			test_must_fail test-run-command run-command hello.sh 2>err
		) &&
		test_i18ngrep ...
	'

[...]

Is this the common case? Why would my interpreter be in the designated spot but not marked executable? Is there some other motivating example? (I'm genuinely curious; it's ok if the answer is "no".)

[...]

Maybe:

	test_expect_success POSIXPERM 'diagnose missing interpreter' '
		echo "#!/nonexistent/interpreter" >hello.sh &&
		chmod +x hello.sh &&
		test_must_fail test-run-command run-command hello.sh 2>err &&
		test_i18ngrep ...
	'

Hope that helps, Jonathan

On Tue, 24 Jan 2012 23:56:36 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

There isn't. I copied it along with the hello-script lines and didn't pay it much heed. I'll remove the excessive '>empty's.

Makes sense. I'll reorder.

You mean "Elaborate execvp error checking"?

I wouldn't think so. This particular one is addressing a concern raised by Johannes Sixt in reaction to a patch from Junio.

http://article.gmane.org/gmane.comp.version-control.git/171848

Will check.

The interpretation of errors from execvp was rather terse. For user convenience communication of the nature of the error can be improved.

This patch introduces a function with more elaborate investigation of the errors encountered. After a failure occurs on a program or script, inspect_failure will run through the PATH entries if necessary, and if the file exists, perform some tests on the file. If nothing is found, ENOENT is returned.

The function inspect_file() will try to find out if the file itself or an interpreter caused the issue. Some scripts don't have the hash-bang and will not be reported as scripts. If the interpreter cannot be found, the existing behavior is kept -- i.e. ENOENT will be reported to the parent process.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |  144 +++++++++++++++++++++++++++++++++++++++++++++---
 t/t0061-run-command.sh |    6 +-
 2 files changed, 140 insertions(+), 10 deletions(-)

Klaver wrote:

Could you give an example?

[...]

Not related to this patch, but I wonder if there should be a separate run-command-unix.c file so these ifdefs would no longer be necessary.

What happens on Windows?

I suspect it might be clearer to use die() inline in the two call sites so the reader does not have to figure out the calling convention.

What does this function do? What happens if first_line doesn't start with "#!"? What should happen when there is a newline instead of a command name? How about commands with quoting characters like " and backslash --- are the semantics portable in these cases?

No need to use a strbuf here: xmemdupz would take care of the allocation and copy more simply.

I wonder if it's possible to rearrange this code to avoid deep nesting. What does the function do, anyway? (If the reader has to ask, it needs a comment or to be renamed.)

I guess the idea is to diagnose after the fact why execvp failed. Might be simplest like this:

	To diagnose execvp failure:
		if filename does not contain a '/':
			if we can't find it on the search path:
				That's the problem, dummy!
			replace filename with full path
		if file does not exist:
			just report strerror(errno)
		if not executable:
			...
		if interpreter does not exist:
			...
		if interpreter not executable:
			...
		otherwise, just report strerror(errno)

with a separate function to find a command on the PATH, complaining when it encounters an unsearchable entry.

Thanks for a fun read.

Jonathan

On Wed, 25 Jan 2012 00:22:39 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

The case that triggered me to work on this. I had an incorrect entry in my PATH and some aliasing tests failed. The generated command output was something like

fatal: script: Access Denied

while I was certain it didn't exist because I was expecting an alias to be executed. Also, access denied on a file that doesn't exist? That didn't make sense (it really doesn't if you don't know the execvp workings). Basically git takes no effort in actually pointing the user to the source of the problem. During the previous two versions I came to realize that this goes for all error codes execvp produces.

Might be useful indeed.

I haven't changed anything on the windows side, so that probably sticks to the old behavior.

Fair enough. I originally expected it to be used more than twice in the code.

This gets the interpreter from a #! line, assuming that it actually has a #!. You might call it naive, which would be fair enough. I didn't see much point in doing the same check twice, but inlining this code in its callsite felt messy.

Right.

Correct guess.

Well, this approach basically does the same thing, but this may have gotten a bit tunnel-visioned. I'll have a look at your approach and see what it gives us.

Thanks for an insightful review.

Frans Klaver wrote:

Sorry for the lack of clarity. I meant that a (precise) "before and after" example could make the commit message a lot easier to understand.

[...]

This was mostly a comment on the change description --- unless I look at the patch, if I try this out on Windows after reading the changelog I would end up utterly confused. For patch 5/5, it also brings up worries about consistency --- if systems are going to be relying on a missing #! interpreter being treated differently from a missing script for the sake of silent_exec_failure, do the same considerations apply on Windows, too?

Perhaps it's more along the lines of "this is not supposed to happen in practice, and when it does, humans will find it easier to debug if we error out hard instead of falling back to the 'if the command does not exist' behavior (e.g., by trying an alias next)". In other words, maybe this is intended as an optional nicety rather than something scripts would ever rely on.

Jonathan

On Wed, 25 Jan 2012 20:22:22 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

Ah I see. I'll add something along those lines.

I'm actually not sure if scripts would be relying on this. There is of course a good chance that people actually will rely on it, regardless of what we think. If there are consistency concerns on different platforms I'd probably have to work on that as well. Mentioning that windows isn't affected by these changes would be a start though.

Exactly. My concern was primarily the human interaction, so getting at least some pointer to the cause of the error. Would that be nice to have on windows as well? It probably would.

Am 24.01.2012 23:32, schrieb Frans Klaver:

Can we end up here if errno == ENOENT? If so, silent_exec_failure must be checked. (inspect_file does not return.)

Isn't it important that this function calls exit(127) if we want silent_exec_failure and errno == ENOENT? But I don't see that this guaranteed by inspect_failure; see above.

-- Hannes

On Wed, 25 Jan 2012 20:03:46 +0100, Johannes Sixt <j6t@kdbg.org> wrote:

Hm, good catch. Yes, we can if the interpreter isn't found. I never intended to actually leave the "interpreter not found" ENOENT case in it's current shape, so it probably slipped through. Will fix inspect_failure here to guarantee silent_exec_failure is heeded. Patch 5/5 would probably remove it again.

Thanks.

execvp will return EACCES when PATH entries are encountered that cannot be searched and the requested command isn't found. Since this error only arises in that particular case, warning the user is more appropriate than producing an error.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
 run-command.c          |    1 +
 t/t0061-run-command.sh |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

If the interpreter wasn't found, execvp returns ENOENT. The existing error checking did not differentiate between file not found and interpreter not found. While the former may be an incentive to start inspecting aliases, the latter is an error because the desired script is actually found.

This patch explicitly makes the interpreter failure a fatal error.

Signed-off-by: Frans Klaver <fransklaver@gmail.com>
---
As far as I'm concerned, this is a bug fix. However, since it really is
a change in git's behavior, we can still consider postponing this patch.

 run-command.c          |    7 +------
 t/t0061-run-command.sh |    4 ++--
 2 files changed, 3 insertions(+), 8 deletions(-)

Frans Klaver wrote:

Thanks. I'd suggest using "test_expect_code" rather than the detailed wording of the message, since that is what scripts might want to rely on.

What happens on Windows?

On Wed, 25 Jan 2012 00:24:21 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

OK, makes sense.

I didn't plan anything to happen on windows. Doesn't POSIXPERM rule that OS out? I guess it could use similar code to this patch series to tackle all this.

Am 25.01.2012 08:12, schrieb Frans Klaver:

It does; these tests are skipped on Windows.

No thanks. IMHO, this is already too much code for too little gain.

-- Hannes

On Wed, 25 Jan 2012 19:55:36 +0100, Johannes Sixt <j6t@kdbg.org> wrote:

That's OK. If it's not only you who feels this way, I'd better spend my time on something that does add value.

Thanks for having a look in any case.

Frans

Johannes Sixt <j6t@kdbg.org> writes:

Thanks for bringing a bit of sanity. You have already said it "In which way is git different from other tools that execvp other programs?" earlier (http://thread.gmane.org/gmane.comp.version-control.git/171755/focus=171848)

On Thu, Jan 26, 2012 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Well, one tool that it differs from is bash (although bash uses execve directly I think). Personally I think this whole thing essentially a lack of information from execv*. Also, I do agree that the code required for this is quite more than I would have liked, but it will reduce confusion when things go wrong. It's when things go wrong that people get annoyed. Annoyed people look for greener grass. If that bit of annoyance could be reduced, why not go the extra mile for that little bit of gain?

Being as it is, I'll stop working on this. If this was pretty much going to be /dev/null'ed from the beginning, I'd rather have heard it after my first patches.

In any case, it has been an education so far. Thanks for that. And if there's any issue you think I could start tackling, please don't hesitate to cc me.

Cheers, Frans

(+cc: Jeff because mentioning a pagination side-issue [*]) Frans Klaver wrote:

Almost always when a developer has an itch, it is _possible_ to massage a patch that scratches it into something acceptable to others. And whether it is worth the trouble in terms of time is something that only that developer can decide.

So no, I would not say these patches were not doomed from the beginning. However, I certainly agree that in their current form they are more complicated than the use case justifies.

There is a tension between requirements that leaves me oddly uncomfortable with the series:

a. on one hand, it would be nice to preserve all the current features
   of execvp(), which makes the approach of only doing post-mortem
   analysis after a failed execvp appealing;

b. on the other hand, it would be nice [*] to avoid launching a pager
   only in order to call execvp for a command that does not exist when
   the fallback might be to an alias to a command that does not want a
   pager.  That would require figuring out in advance that execvp
   would fail with ENOENT and missing out on possible system extensions
   that allow execvp to run shell built-in commands not existing on
   the filesystem.

I want to like (b), but the downside seems unacceptable. I honestly don't know if something like (a) would be a good idea if well executed, so I was happy to have the opportunity to try to help massage these patches into a form that would make the answer more obvious.

On Fri, Jan 27, 2012 at 9:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Hannes' reaction and Junio's to his didn't give me the impression they even saw a possibility.

Good. That's something we can work on.

Just for my understanding: before a command is executed, a pager (less/more or so) is started? We want to avoid starting the pager if we won't be able to execute the command?

The downside being: having to figure out what execvp is going to do? That would be tantamount to writing your own execvp.

Given the above information, I'm happy to work on this to see if we can mould it into something usable. Since the impact seems to go beyond figuring out why execvp failed, I'm probably going to need some help.

For now, I'll go through your suggestions and see what it produces. We'll go from there.

Thanks for the heads-up.

Frans Klaver wrote:

See [1] for an example of a recent patch touching the relevant code path.

For example: if I run "git --paginate foo", foo is an alias for bar, and the "[pager] bar" configuration is set to point to "otherpager", then without this safety git launches the default pager in preparation for running git-foo, receives ENOENT from execvp("git-foo"), and then the pager has already been launched and it is too late to launch otherpager instead.

Exactly.

I see.

Well, as I said, I don't know. :) And I don't want to give false hopes --- it's perfectly possible and not even unlikely that this is a dead end and any patch in this direction will turn out not to be a good idea and not applied.

That's part of why I was really grateful to Hannes for the reminder to take a step back for a moment and consider whether it's worth it. Maybe there's another way or a more targetted way to take care of the motivational original confusing scenario that leads to execvp errors. (By the way, can you remind me which one that was?)

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/179635

On Fri, Jan 27, 2012 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

I'll have a look at that.

Something to be looked into then.

Hm don't worry about false hopes. I don't insist on having some of my work actually in if there's no point in including it. Contributing to the research is good enough for me if we can come to a conclusion that we can present to people running into similar issues.

I wonder.

I'll even summarize my thinking and motivation about this.

I was executing the test suite on my PC. Some test for aliases failed -- git said EACCES, while for aliases one would expect ENOENT. For users expecting an alias to be executed, "cannot execute git-frotz: Access Denied" will be rather confusing. "Huh? Access denied? The file doesn't even exist?!". It took me quite some debugging in git to track this down to an inaccessible PATH entry. As someone who didn't know anything of the git internal code it took quite a bit of learning as well just to find out where we'd end up in the first place. It bothered me (and still does) that execve uses EACCES for at least four different errors:

...
    EACCES Search  permission is denied on a component of the path
prefix of filename or the
              name of a script interpreter.  (See also path_resolution(7).)
    EACCES The file or a script interpreter is not a regular file.
    EACCES Execute permission is denied for the file or a script or
ELF interpreter.
    EACCES The file system is mounted noexec.
...

Anyway, reading through that man page later on I found that a lot of errors are only mentioned once, but do contain 'or' in the problem description, like the first one of the EACCES items. ENOENT does that as well:

    ENOENT The file filename or a script or ELF interpreter does  not
exist,  or  a  shared
              library needed for file or interpreter cannot be found.

I then additionally figured that always silently passing ENOENT is a bad thing to do, because it simply can mean more than just "The file you asked for cannot be found". It means "something required cannot be found". My resulting view on this is basically that the execvp error handling git currently has, is lacking a nuance that is necessary for effective debugging. As I said earlier, it's when things go wrong people get annoyed. Even more so if you don't provide them with pointers to what might be wrong.

It also bothers me that to effectively debug program execution errors, you have to know that git uses execvp, and you have to know how it behaves. I would label that "implementation details" and as a user I really don't want to be bothered by that, especially not as a new user. For that reason I would have liked to end up with something like bash does. I would rather see "Hey dummy, can't access /some/path" or "Hey dummy, you ask for an interpreter that I have no acces to" than "Well we got EACCES, so check the following things: Do we have search permission on.... Is it a regular file? mounted noexec?..." or "We got EACCES, check man execve(2) for possible reasons", although I'd agree that even that would be slightly better than "We got EACCES".

So take of your git-guru hat and put on your new-git-user one and let it simmer for a while.

On Fri, 27 Jan 2012 10:41:45 +0100, Jonathan Nieder <jrnieder@gmail.com> wrote:

Took me a while to catch your drift, but if I understand correctly, you're thinking using some of the code to find out if starting the pager is a good idea or not. If I factor out the part that finds a command in PATH, there's the helper that with a fair amount of certainty, will predict whether 'git foo' will fail with ENOENT or not. It would fix a possible problem that is currently there. Obviously the only case we can catch, is the command not actually existing. Although it is just one of the cases ENOENT can be returned for, I think it is the only one git actually cares about when checking for it.

So as it seems, there are a few cases where we can fairly reliably predict whether a command is or isn't going to be found. Unless I'm mistaken, dashed externals are never shell built-ins and so we don't have to be able to check for their existence. Then assuming that silent_exec_failure really only cares about commands actually not existing, we can be fairly naive about it. See if we can find it somewhere in PATH and if we can't bail out. If we can, start the pager and everything execvp then returns will be regarded a fatal error. In this case it would be a choice between spawning the wrong pager, or having a quick browse through the file system.

It may be a sensible reminder. I didn't understand that comment as such. Maybe it's Hannes' style, I don't know.

Been thinking about it and I doubt it. To find out whether EACCES is returned due to a PATH issue, you have to go through all of those PATH entries. So while you're at it, there's a lot more you can check and most of those checks are fairly trivial to do.

I think I've worked through all your review comments. I'll address Hannes' comments, create an RFC series and see where we end up.

Junio, care to be CC'd in that?

Thanks, Frans