Re: [PATCH] git-compat-util: work around for access(X_OK) under root
- Date: Wed, 24 Apr 2019 07:54:01 +0000
- From: "CHIGOT, CLEMENT" <clement.chigot@xxxxxxxx>
- Subject: Re: [PATCH] git-compat-util: work around for access(X_OK) under root
From: Junio C Hamano <jch2355@xxxxxxxxx> on behalf of Junio C Hamano <gitster@xxxxxxxxx>
> "CHIGOT, CLEMENT" <clement.chigot@xxxxxxxx> writes:
> > From: Junio C Hamano <jch2355@xxxxxxxxx> on behalf of Junio C Hamano <gitster@xxxxxxxxx>
> >> > 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.
> > 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
> 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.
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.