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. > > 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&data=02%7C01%7Cclement.chigot%40atos.net%7C4646e90041f04b4fa46c08d6c84ea22b%7C33440fc6b7c7412cbb730e70b0198d5a%7C0%7C1%7C636916637385863489&sdata=nO1e1MV99iHsCyhDLTgBNIRCj0AZ%2BpwqtGrQY%2FJAB3g%3D&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.
