Re: [PATCH 2/2] run-command: Add interpreter permissions check
On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 69 quoted lines
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> If a script is started and the interpreter of that script given in the
>> shebang cannot be started due to permissions, we can get a rather
>> obscure situation. All permission checks pass for the script itself,
>> but we still get EACCES from execvp.
>>
>> Try to find out if the above is the case and warn the user about it.
>>
>> Signed-off-by: Frans Klaver <fransklaver@gmail.com>
>> ---
>> run-command.c | 66 +++++++++++++++++++++++++++++++++++++++++++----
>> t/t0061-run-command.sh | 22 ++++++++++++++++
>> 2 files changed, 82 insertions(+), 6 deletions(-)
>>
>> diff --git a/run-command.c b/run-command.c
>> index 5e38c5a..b8cf8d4 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -194,6 +194,63 @@ static int have_read_execute_permissions(const char *path)
>> return 0;
>> }
>>
>> +static void check_interpreter(const char *cmd)
>> +{
>> + FILE *f;
>> + struct strbuf sb = STRBUF_INIT;
>> + /* bash reads an 80 character line when determining the interpreter.
>> + * BSD apparently only allows 32 characters, as it is the size of
>> + * your average binary executable header.
>> + */
>> + char firstline[80];
>> + char *interpreter = NULL;
>> + size_t s, i;
>> +
>> + f = fopen(cmd, "r");
>> + if (!f) {
>> + error("cannot open file '%s': %s\n", cmd, strerror(errno));
>> + return;
>> + }
>> +
>> + s = fread(firstline, 1, sizeof(firstline), f);
>> + if (s < 2) {
>> + trace_printf("cannot determine file type");
>> + fclose(f);
>> + return;
>> + }
>> +
>> + if (firstline[0] != '#' || firstline[1] != '!') {
>> + trace_printf("file '%s' is not a script or"
>> + " is a script without '#!'", cmd);
>> + fclose(f);
>> + return;
>> + }
>
> Nice touches to silently pass scripts that do not begin with she-bang.
>
>> +
>> + /* see if the given path has the executable bit set */
>> + for (i = 2; i < s; i++) {
>> + if (!interpreter && firstline[i] != ' ' && firstline[i] != '\t')
>> + interpreter = firstline + i;
>> +
>> + if (interpreter && (firstline[i] == ' ' ||
>> + firstline[i] == '\n')) {
>
> Curious.
>
> "#!<TAB>/bin/bash<TAB><LF>" would cause you to check "/bin/bash<TAB>"?Apparently so. Thanks for catching.
Show 6 quoted lines
>> + strbuf_add(&sb, interpreter,
>> + (firstline + i) - interpreter);
>> + break;
>> + }
>
> Wouldn't strcspn() work better instead of this loop?
Show 41 quoted lines
>> + }
>> + if (!sb.len) {
>> + error("could not determine interpreter");
>> + strbuf_release(&sb);
>> + return;
>> + }
>> +
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("bad interpreter: no read/execute permissions on '%s'\n",
>> + sb.buf);
>> +
>> + strbuf_release(&sb);
>> +}
>> +
>> static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> {
>> /* man 2 execve states that EACCES is returned for:
>> @@ -209,8 +266,8 @@ static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> char *next;
>>
>> if (strchr(cmd, '/')) {
>> - if (!have_read_execute_permissions(cmd))
>> - error("no read/execute permissions on '%s'\n", cmd);
>> + if (have_read_execute_permissions(cmd))
>> + check_interpreter(cmd);
>
> I would have expected the overall logic to be more like this:
>
> if we cannot read and execute it then
> that in itself is an error (i.e. the error message from [1/2])
> else if we can read it then
> let's see if there is an error in the interpreter.
>
> It is unnatural to see "if we can read and execute, then see if there is
> anything wrong with the interpreter" and _nothing else_ here. If you made
> the "have_read_execute_permissions()" to issue the error message you used
> to give in your [1/2] patch here, that is OK from the point of view of the
> overall code structure, but then the function is no longer "do we have
> permissions" boolean check and needs to be renamed. And if you didn't,
> then I have to wonder why we do not need the error message you added in
> your [1/2].Hm, yea makes sense. I'll rethink this a bit.
Again, thanks for the review.
Frans Klaver <fransklaver@gmail.com>