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>
> "CHIGOT, CLEMENT" <clement.chigot@atos.net> writes:
> > 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.
> >> ...
> >
> > 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.
> 
> Ah, OK, so the issue is not that AIX allows the root to execute even
> files that have no executable bit, but X_OK check on it returns
> useless answer when we want to know if an attempted execution of the
> file by the user would succeed.
> 
> That was exactly the kind of information expected in your log
> message to explain why this change is a good thing to have.

Ok I'll add something clearer. 

> 
> >> 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.
> 
> I wasn't questioning if it is "enough".  If the root user "su"es to
> a normal user, does the issue that exec(path) and access(path, X_OK)
> are incoherent still happen?  If not, checking for !uid is actively
> wrong, not just unnecessary.

Yes, it doesn't happen. So, only EUID should be check as you said. 

> 
> >> > +     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.
> 
> Yup, and have the function fall-back to the system supplied access()
> after doing geteuid() and finding that the user is not the root user
> without doing anything else---and use the remaining lines in the
> function for the special case.  That would make the function's logic
> easier to read, too.
> 
> >> 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.
> 
> Good.
> 
> > 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. 
> 
> As the patch we are discussing in this exchange has not been
> accepted nor merged to the 'next' branch yet, you'd be sending a new
> version as a whole (i.e. not as an incremental patch on top of the
> version we have reviewed here) with "[PATCH v2]" on its subject
> header.
> 
> Emily Shaffer has been writing and revising a tutorial on the
> procedure recently, which may be of interest to you, and I am
> interested in using your fresh eyes to see if its expectation
> for the readers is set appropriately.
> 
>   https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpublic-inbox.org%2Fgit%2F20190423193410.101803-1-emilyshaffer%40google.com%2F&amp;data=02%7C01%7Cclement.chigot%40atos.net%7C4646e90041f04b4fa46c08d6c84ea22b%7C33440fc6b7c7412cbb730e70b0198d5a%7C0%7C1%7C636916637385863489&amp;sdata=nO1e1MV99iHsCyhDLTgBNIRCj0AZ%2BpwqtGrQY%2FJAB3g%3D&amp;reserved=0

Indeed, that's a really good tutorial. As I didn't made a new feature, I can't
judge the part referring to this. But, how to submit with both way seems pretty
clear to me. Maybe a sum up can be added because it's quite long and verbose. I
think a part with just the commands to run in order can be useful. Something
like this: https://golang.org/doc/contribute.html#tmp_12. 

> 
> Thanks.