git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Frans Klaver <fransklaver@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 2/2] run-command: Add interpreter permissions check
Date: Wed, 7 Dec 2011 09:37:48 +0100	[thread overview]
Message-ID: <CAH6sp9MqwKppcrtP7YM8FZAs=odmUicTvsxiYyH0ENmJrPxqEA@mail.gmail.com> (raw)
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.

  reply	other threads:[~2011-12-07  8:37 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-11-21 21:53 [PATCH] run-command.c: Accept EACCES as command not found Frans Klaver
2011-11-21 22:13 ` Junio C Hamano
2011-11-21 23:06   ` Frans Klaver
2011-11-21 23:54     ` Junio C Hamano
2011-11-22  9:31       ` Frans Klaver
2011-11-23  8:17         ` Frans Klaver
2011-11-23 12:04           ` Nguyen Thai Ngoc Duy
2011-11-23 13:25             ` Frans Klaver
2011-11-23 22:55           ` Frans Klaver
2011-12-06 21:38             ` [PATCH 0/2] run-command: Add EACCES diagnostics Frans Klaver
2011-12-06 21:38               ` [PATCH 1/2] run-command: Add checks after execvp fails with EACCES Frans Klaver
2011-12-06 22:35                 ` Junio C Hamano
2011-12-07  8:31                   ` Frans Klaver
2011-12-08 21:44                   ` Frans Klaver
2011-12-09 17:23                     ` Junio C Hamano
2011-12-09 21:35                       ` Frans Klaver
2011-12-06 21:38               ` [PATCH 2/2] run-command: Add interpreter permissions check Frans Klaver
2011-12-06 22:47                 ` Junio C Hamano
2011-12-07  8:37                   ` Frans Klaver [this message]
2011-12-13 15:08             ` [PATCH 0/2 v2] run-command: Add eacces diagnostics Frans Klaver
2011-12-13 15:08               ` [PATCH 1/2] run-command: Add checks after execvp fails with EACCES Frans Klaver
2011-12-13 19:01                 ` Junio C Hamano
2011-12-14 14:31                   ` Frans Klaver
2011-12-14 22:06                     ` Frans Klaver
2011-12-13 15:08               ` [PATCH 2/2] run-command: Add interpreter permissions check Frans Klaver

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAH6sp9MqwKppcrtP7YM8FZAs=odmUicTvsxiYyH0ENmJrPxqEA@mail.gmail.com' \
    --to=fransklaver@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).