Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES

On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano [off-list ref] wrote:

 - That "have_..._ions()" is too long and ugly.

I half expected that one and I agree. I vaguely remember typing it,
deleting it and typing it again when I started on that one.

 - The only thing you care about this callsite is if you have enough
  permission to execute the "cmd".

In fact, you should not unconditionally require read permissions here.

   $ chmod a-r $(type --path git) && /bin/ls -l $(type --path git)
   --wx--x--x 109 junio junio 5126580 Dec 13 09:47 /home/junio/git-active/bin/git
   $ /home/junio/git-active/bin/git --version
   git version 1.7.8.249.gb1b73

You may need read permission when the file is a script (i.e. not binary
executable).

[...]

When checking if you can run "foo/bar/baz", directories "foo/" and "foo/bar/"
do not have to be readable.  They only have to have executable bit to allow
descending into them, and typically this is called "searchable" (see man chmod).

   $ mkdir -p /var/tmp/a/b && cp $(type --path git) /var/tmp/a/b/git
   $ chmod 111 /var/tmp/a /var/tmp/a/b
   $ /var/tmp/a/b/git --version
   git version 1.7.8.249.gb1b73

I'd suggest having two helper functions, instead of the single one with
overlong "have...ions" name.

 - can_search_directory() checks with access(X_OK);

 - can_execute_file() checks with access(X_OK|R_OK), even though R_OK is
  not always needed.

On the whole I like the suggestion. We should probably take it a bit
further. Since the x and r bits basically have nothing to do with each
other, and we need +rx only on scripts, I could just rely on fopen()
for the +r check. I will still add the can_execute_file() and
can_search_dir() helpers to support readability, as access(path, X_OK)
means different things in the different contexts. I would then
probably go for is_searchable() and is_executable() as function names.
is_executable then means "is file and has executable flag set",
is_searchable means "is directory and has executable flag set".
Basically files won't be searchable and directories won't be
executable. If execvp fails on a command that is executable, but not
readable, it is definitely a script and we can generate an error in
that case. 1/2 would then probably use access(path, R_OK), while 2/2
would start using fopen.

Since fopen() uses the effective uid/gid, it then makes sense to use
eaccess(3) instead of access(2) if available. It would be stupid to
have bugs arise just because of a mismatch between the [ug]ids used by
the two access checks. I'm aware of the fact that eaccess isn't a
standard function, so a #define HAVE... fallback to at least access()
would probably be required.

Use the former here where you check the directory that contains the
command, and use the latter up above where you check the command that is
supposed to be executable, and also down below after you checked sb.buf is
a path to a file that may be the command that is supposed to be
executable.

Then patch 2/2 can extend can_execute() to enhance its support for scripts
by reading the hash-bang line and validating it, etc.

I'd rather keep the hash-bang check outside of that function and use
can_execute/is_executable for checking the interpreter as well, if
only for keeping the possibility of easily promoting them into an API.

I'd rather move check_interpreter into where it's called now, but pull
out the logic to find the interpreter. This will keep the error text
generation in diagnose_execvp_eacces. I think the code will make more
sense this way. There's tons of more errors that can be caused by a
faulty interpreter, and it'll be easier to cover more cases this way
in the future.

Thanks for the insightful reviews so far.

Let me know what you think,
Frans