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

From: Frans Klaver <fransklaver@gmail.com>
Date: Dec 14, 2011, 14:31 UTC
Message-ID: <CAH6sp9Mf=EjkVN9mDN59ZCxCU0sCFLa8E=7YxM1J8LCCMr=xYQ@mail.gmail.com>
In-Reply-To: <7vliqguwhq.fsf@alter.siamese.dyndns.org>

On Tue, Dec 13, 2011 at 8:01 PM, Junio C Hamano <gitster@pobox.com> 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.

Show 13 quoted lines

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

[...]

Show 16 quoted lines

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

Show 9 quoted lines

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