From: Frans Klaver <fransklaver@gmail.com>
Date: Wed, 07 Dec 2011 08:37:48 GMT
Subject: Re: [PATCH 2/2] run-command: Add interpreter permissions check
Message-ID: <CAH6sp9MqwKppcrtP7YM8FZAs=odmUicTvsxiYyH0ENmJrPxqEA@mail.gmail.com>
In-Reply-To: <7vk469e2rn.fsf@alter.siamese.dyndns.org>

On Tue, Dec 6, 2011 at 11:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.


>> +                     strbuf_add(&sb, interpreter,
>> +                                     (firstline + i) - interpreter);
>> +                     break;
>> +             }
>
> Wouldn't strcspn() work better instead of this loop?

Probably. Will revise.


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