"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. >> 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. >> > + 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://public-inbox.org/git/20190423193410.101803-1-emilyshaffer@xxxxxxxxxx/ Thanks.
