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.
next prev parent 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).