Re: [PATCH] git-compat-util: work around for access(X_OK) under root

["CHIGOT, CLEMENT" <clement.chigot@atos.net>]

From: Junio C Hamano <jch2355@gmail.com> on behalf of Junio C Hamano <gitster@pobox.com>
> > On some OSes like AIX, access with X_OK is always true if launched under
> > root.
> 
> That may be the case, but you'd need to describe why it is a problem
> here, before talking about the need for a "work around".
> 
> For example, if a directory on $PATH has a file called git-frotz
> that has no executable bit, perhaps "git frotz" would execute that
> file but only when you are running it as the root user, but not as
> any other user.
> 
> But that by itself does not sound like a problem to me.  After all,
> a user with such a set-up on AIX may deliberately wanted to make
> sure that a program like "git-frotz" that is only useful for
> administrative purposes does not get in the way when using "git"
> normally, but becomes available only when the user does "su".  IOW,
> that sounds more like a feature an AIX user might want to take
> advantage of.
> 
> Perhaps the reason why you do not want to use access(X_OK) that
> returns true for root may be different from the above, but without
> knowing what it is, it is far from clear to me why this patch is a
> good idea.  The patch needs to be justified a lot better.
> 
> Everything below may become a moot point, as it is unclear if the
> (untold) motivation behind this change makes sense in the first
> place, but assuming that it is a good change that merely is poorly
> explained, let's read on.
> 

This patch is needed in order to have hooks working on AIX. When run as root,
access on hooks will return true even if a hook can't be executed. Therefore,
as far as I know, git will try to execute it as is and we'll get this kind of
error:
"git commit -m content
 fatal: cannot exec '.git/hooks/pre-commit': Permission denied"

I'm not fully aware about how git is using access and if it's adding some chmod,
if it returns false. 

We already know that there is some problems with access() as it has already
occurs during our port of bash. Note that a part of this code comes from it.

We find this work around and thought it could be interesting to add it in the
source code as some others OS (like Solaris according to bash) has the same kind
of problems.
However, you're right it's far from perfect and I might have
submitted to soon, sorry about that.. 

> > diff --git a/compat/access.c b/compat/access.c
> > new file mode 100644
> > index 0000000000..e4202d4585
> > --- /dev/null
> > +++ b/compat/access.c
> > @@ -0,0 +1,29 @@
> > +#include "../git-compat-util.h"
> 
> This will get interesting.
> 
> > +/* Do the same thing access(2) does, but use the effective uid and gid,
> > +   and don't make the mistake of telling root that any file is
> > +   executable.  This version uses stat(2). */
> 
>         /*
>          * Our multi-line comment looks more like
>          * this.  A slash-asterisk without anything else
>          * on its own line begins it, and it is concluded
>          * with  an asterisk-slash on its own line.
>          * Each line in between begins with an asterisk,
>          * and the asterisks align on a monospace terminal.
>          */
> 
> > +int git_access (const char *path, int mode)
> 
> No SP after function name before the parens that begins the
> parameter list.
> 
> > +{
> > +     struct stat st;
> > +     uid_t euid = geteuid();
> > +     uid_t uid = getuid();
> > +
> > +     if (stat(path, &st) < 0)
> > +             return -1;
> 
> This stat is a wasted syscall if the running user is not root.
> Structure the function more like
> 
> 
>         int git_access(const char *path, int mode)
>         {
>                 struct stat st;
> 
>                 /* do not interfere a normal user */
>                 if (geteuid())
>                         return access(path, mode);
> 
>                 if (stat(path, &st) < 0)
>                         return -1; /* errno apprpriately set by stat() */
>                 ... other stuff needed for the root user ...
>         }
> 
> Does the true UID matter for the purpose of permission/privilege
> checking?  Why do we have to check anything other than the effective
> UID?
>

Actually, I don't know. Bash is doing it but I think EUID is enough. 

> > +     if (!(uid) || !(euid)) {
> > +             /* Root can read or write any file. */
> > +             if (!(mode & X_OK))
> > +                     return 0;
> > +
> > +             /* Root can execute any file that has any one of the execute
> > +                bits set. */
> > +             if (st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))
> > +                     return 0;
> > +             errno = EACCES;
> > +             return -1;
> > +     }
> > +
> > +     return access(path, X_OK);
> 
> I think the last "fallback to the system access()" is wrong, as the
> "special case for root" block seems to except that the function may
> be called to check for Read or Write permission, not just for X_OK.

That's a mistake from me. It should be "mode" instead of "X_OK". It seems that 
most of the time, it's used only with X_OK or F_OK that's why it has worked. I'll 
fix that. 

> 
> > +}
> > diff --git a/config.mak.uname b/config.mak.uname
> > index 86cbe47627..ce13ab8295 100644
> > --- a/config.mak.uname
> > +++ b/config.mak.uname
> > @@ -270,6 +270,7 @@ ifeq ($(uname_S),AIX)
> >        NEEDS_LIBICONV = YesPlease
> >        BASIC_CFLAGS += -D_LARGE_FILES
> >        FILENO_IS_A_MACRO = UnfortunatelyYes
> > +     NEED_ACCESS_ROOT_HANDLER = UnfortunatelyYes
> >        ifeq ($(shell expr "$(uname_V)" : '[1234]'),1)
> >                NO_PTHREADS = YesPlease
> >        else
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 31b47932bd..bb8df9d2e5 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1242,6 +1242,14 @@ int git_fileno(FILE *stream);
> >  # endif
> >  #endif
> 
> As I promised earlier, this will get interesting.
> 
> > +#ifdef NEED_ACCESS_ROOT_HANDLER
> > +#ifdef access
> > +#undef access
> > +#endif
> 
> If a platform that needs git_access() wrapper happens to define
> access(2) as a macro in its system header, you would lose the real
> name of that function with this.
> 
> > +#define access git_access
> > +extern int git_access(const char *path, int mode);
> 
> And in any source file that includes git-compat-util.h, when you
> make a call to access(2), you'll end up calling git_access()
> instead.
> 
> Remember what was in the end (in your original) or the early part of
> git_access() that handled the case where the function is called for
> a non-root user?  Yes, we write "access(path, mode)", expecting to
> make a fallback call to the system-supplied access(2).  With this
> include file, that will never happen---instead, it will recurse in
> itself forever.
> 
> See how FILENO_IS_A_MACRO defined immediately before this part uses
> the "#ifndef COMPAT_CODE" to guard against exactly the same problem.

Alright, I now understand how this work. Actually, I haven't thought about that
problem. But yeah this cannot work at the moment. I'll redo this patch and check
if everything is still working.
I'm sorry about these mistakes. I've made this patch quickly in order to have
something else working. It seems to be too quickly. I'll fix that !  

By the way, do I need to recreate a thread with [PATCH v2] ? Or I'll add the new
version in this one ? I don't know how you're proceeding.  
> 
> 
> Thanks.