"CHIGOT, CLEMENT" <clement.chigot@xxxxxxxx> writes:
> 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.
> 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?
> + 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.
> +}
> 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.
Thanks.
