Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
- From
Frans Klaver <fransklaver@gmail.com>
- Date
- Dec 7, 2011, 08:31 UTC
- Message-ID
- <CAH6sp9NsRDWoMtnBUXOP-OMFwjjUm-OuRLpNvcS4pC1S=C93EQ@mail.gmail.com>
- In-Reply-To
- <7vpqg1e3au.fsf@alter.siamese.dyndns.org>
Thanks for the review. There's a lot of things you mention that I either didn't see (staring blind, you know) or that I didn't know of.
On Tue, Dec 6, 2011 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> Frans Klaver <fransklaver@gmail.com> writes:
>
>> +#ifndef WIN32
>> +static int is_in_group(gid_t gid)
>> ...
>> +static int have_read_execute_permissions(const char *path)
>> +{
>> + struct stat s;
>> + trace_printf("checking '%s'\n", path);
>> +
>> + if (stat(path, &s) < 0) {
>> + ...
>> + /* check world permissions */
>> + if ((s.st_mode&(S_IXOTH|S_IROTH)) == (S_IXOTH|S_IROTH))
>> + return 1;
>
> Hmm, do you need to do this with stat(2)?
>
> Wouldn't access(2) with R_OK|X_OK give you exactly what you want without
> this much trouble?Probably. I'll use access instead in a reroll.
Show 12 quoted lines
> I also think that your permission check is incorrectly implemented. > > $ cd /var/tmp && date >j && chmod 044 j && ls -l j > ----r--r-- 1 junio junio 29 Dec 6 14:32 j > $ cat j > cat: j: Permission denied > $ su pogo > Password: > $ cat j > Tue Dec 6 14:32:23 PST 2011 > > That's a world-readable but unreadable-only-to-me file.
Hmm, this is a case that didn't fit my expectations. Thanks for catching.
Show 25 quoted lines
>> +static void diagnose_execvp_eacces(const char *cmd, const char **argv)
>> +{
>> + /* man 2 execve states that EACCES is returned for:
>
> /*
> * Just a style, but we tend to write multi-line comment like
> * this, without anything else on opening and closing lines of
> * the comment block.
> */
>
>> + * - The file system is mounted noexec
>> + */
>> + struct strbuf sb = STRBUF_INIT;
>> + char *path = getenv("PATH");
>> + char *next;
>> +
>> + if (strchr(cmd, '/')) {
>> + if (!have_read_execute_permissions(cmd))
>> + error("no read/execute permissions on '%s'\n", cmd);
>> + return;
>> + }
>
> Ok, execvp() failed and "cmd" has at least one slash, so we know we did
> not look for it in $PATH. We check only one and return (did you need
> getenv() in that case?).Obviously not. Missed that.
Show 9 quoted lines
>
>> + for (;;) {
>> + next = strchrnul(path, ':');
>> + if (path < next)
>> + strbuf_add(&sb, path, next - path);
>> + else
>> + strbuf_addch(&sb, '.');
>
> Nice touch that you did not forget an empty component on $PATH.Yes, that's a relic from me starting work based on one of your proposed patches[1]. So that one goes to you.
[1] http://article.gmane.org/gmane.comp.version-control.git/171838
Show 6 quoted lines
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n", sb.buf);
>
> Don't you want to continue here upon error, after resetting sb? You just
> saw the directory is unreadble, so you know next file_exists() will fail
> before you try it.Yes. I thought about that. I didn't do that because of the fact that I had to do more than just resetting sb. The path variable has to be updated as well. I had the choice of adding a level of indentation {}, duplicating the code, or just do a check I know before will fail. There's probably something to say for each one of them. I'll probably refactor that a bit more.
Show 15 quoted lines
>> + if (sb.len && sb.buf[sb.len - 1] != '/')
>> + strbuf_addch(&sb, '/');
>> + strbuf_addstr(&sb, cmd);
>> +
>> + if (file_exists(sb.buf)) {
>> + if (!have_read_execute_permissions(sb.buf))
>> + error("no read/execute permissions on '%s'\n",
>> + sb.buf);
>> + else
>> + warn("file '%s' exists and permissions "
>> + "seem OK.\nIf this is a script, see if you "
>> + "have sufficient privileges to run the "
>> + "interpreter", sb.buf);
>
> Does "warn()" do the right thing for multi-line strings like this?I don't know/remember. It seemed like a natural thing to do, but I'll find out.