[PATCH] git-compat-util: work around for access(X_OK) under root

[PATCH] git-compat-util: work around for access(X_OK) under root

[CHIGOT, CLEMENT]

On some OSes like AIX, access with X_OK is always true if launched under
root.
Add NEED_ACCESS_ROOT_HANDLER in order to use an access helper function.
It checks with stat if any executable flags is set when the current user
is root.

Signed-off-by: Clément Chigot <clement.chigot@atos.net>
---
 Makefile          |  8 ++++++++
 compat/access.c   | 29 +++++++++++++++++++++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  8 ++++++++
 4 files changed, 46 insertions(+)
 create mode 100644 compat/access.c

diff --git a/Makefile b/Makefile
index 9f1b6e8926..513d835d01 100644
--- a/Makefile
+++ b/Makefile
@@ -439,6 +439,9 @@ all::
 #
 # Define FILENO_IS_A_MACRO if fileno() is a macro, not a real function.
 #
+# Define NEED_ACCESS_ROOT_HANDLER if access() with X_OK returns always true
+# when launched as root.
+#
 # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
 # default environment variables to be passed when a pager is spawned, e.g.
 #
@@ -1833,6 +1836,11 @@ ifdef FILENO_IS_A_MACRO
 	COMPAT_OBJS += compat/fileno.o
 endif
 
+ifdef NEED_ACCESS_ROOT_HANDLER
+	COMPAT_CFLAGS += -DNEED_ACCESS_ROOT_HANDLER
+	COMPAT_OBJS += compat/access.o
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif
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"
+
+/* 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). */
+int git_access (const char *path, int mode)
+{
+	struct stat st;
+	uid_t euid = geteuid();
+	uid_t uid = getuid();
+
+	if (stat(path, &st) < 0)
+		return -1;
+
+	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);
+}
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
 
+#ifdef NEED_ACCESS_ROOT_HANDLER
+#ifdef access
+#undef access
+#endif
+#define access git_access
+extern int git_access(const char *path, int mode);
+#endif
+
 /*
  * Our code often opens a path to an optional file, to work on its
  * contents when we can successfully open it.  We can ignore a failure
-- 
2.17.1



Clément Chigot
ATOS Bull SAS
1 rue de Provence - 38432 Échirolles - France

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[Duy Nguyen]

On Tue, Apr 23, 2019 at 3:41 PM CHIGOT, CLEMENT <clement.chigot@atos.net> wrote:
>
> On some OSes like AIX, access with X_OK is always true if launched under
> root.
> Add NEED_ACCESS_ROOT_HANDLER in order to use an access helper function.
> It checks with stat if any executable flags is set when the current user
> is root.
>
> Signed-off-by: Clément Chigot <clement.chigot@atos.net>
> ---
>  Makefile          |  8 ++++++++
>  compat/access.c   | 29 +++++++++++++++++++++++++++++
>  config.mak.uname  |  1 +
>  git-compat-util.h |  8 ++++++++
>  4 files changed, 46 insertions(+)
>  create mode 100644 compat/access.c
>
> diff --git a/Makefile b/Makefile
> index 9f1b6e8926..513d835d01 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -439,6 +439,9 @@ all::
>  #
>  # Define FILENO_IS_A_MACRO if fileno() is a macro, not a real function.
>  #
> +# Define NEED_ACCESS_ROOT_HANDLER if access() with X_OK returns always true
> +# when launched as root.
> +#
>  # Define PAGER_ENV to a SP separated VAR=VAL pairs to define
>  # default environment variables to be passed when a pager is spawned, e.g.
>  #
> @@ -1833,6 +1836,11 @@ ifdef FILENO_IS_A_MACRO
>         COMPAT_OBJS += compat/fileno.o
>  endif
>
> +ifdef NEED_ACCESS_ROOT_HANDLER
> +       COMPAT_CFLAGS += -DNEED_ACCESS_ROOT_HANDLER
> +       COMPAT_OBJS += compat/access.o
> +endif
> +
>  ifeq ($(TCLTK_PATH),)
>  NO_TCLTK = NoThanks
>  endif
> 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"
> +
> +/* 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). */
> +int git_access (const char *path, int mode)
> +{
> +       struct stat st;
> +       uid_t euid = geteuid();
> +       uid_t uid = getuid();
> +
> +       if (stat(path, &st) < 0)
> +               return -1;
> +
> +       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);

Do you need an #undef to cancel out the "#define access git_access"
line in git-compat-util.h (or "access" is still not redefined here,
how)?

#undef would screw things up though if the original access is also a
macro. But I guess that's much less likely since this compat code is
only enabled in selected platforms.

> +}
> 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
>
> +#ifdef NEED_ACCESS_ROOT_HANDLER
> +#ifdef access
> +#undef access
> +#endif
> +#define access git_access
> +extern int git_access(const char *path, int mode);
> +#endif
> +
>  /*
>   * Our code often opens a path to an optional file, to work on its
>   * contents when we can successfully open it.  We can ignore a failure
> --
> 2.17.1
>
>
>
> Clément Chigot
> ATOS Bull SAS
> 1 rue de Provence - 38432 Échirolles - France



-- 
Duy

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[Junio C Hamano]

"CHIGOT, CLEMENT" <clement.chigot@atos.net> 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.

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[CHIGOT, CLEMENT]

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.
> 
> 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.
> 

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. Therefore,
as far as I know, git will try to execute it as is and we'll get this kind of
error:
"git commit -m content
 fatal: cannot exec '.git/hooks/pre-commit': Permission denied"

I'm not fully aware about how git is using access and if it's adding some chmod,
if it returns false. 

We already know that there is some problems with access() as it has already
occurs during our port of bash. Note that a part of this code comes from it.

We find this work around and thought it could be interesting to add it in the
source code as some others OS (like Solaris according to bash) has the same kind
of problems.
However, you're right it's far from perfect and I might have
submitted to soon, sorry about that.. 

> > 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?
>

Actually, I don't know. Bash is doing it but I think EUID is enough. 

> > +     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.

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. 

> 
> > +}
> > 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.

Alright, I now understand how this work. Actually, I haven't thought about that
problem. But yeah this cannot work at the moment. I'll redo this patch and check
if everything is still working.
I'm sorry about these mistakes. I've made this patch quickly in order to have
something else working. It seems to be too quickly. I'll fix that !  

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.  
> 
> 
> Thanks.

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Tue, Apr 23, 2019 at 11:31:02AM +0000, CHIGOT, CLEMENT wrote:
> From: Junio C Hamano <jch2355@gmail.com> on behalf of Junio C Hamano <gitster@pobox.com>
> 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. Therefore,
> as far as I know, git will try to execute it as is and we'll get this kind of
> error:
> "git commit -m content
>  fatal: cannot exec '.git/hooks/pre-commit': Permission denied"

I think this is the interesting part.

What POSIX says on this is the following:

  If any access permissions are checked, each shall be checked
  individually, as described in XBD File Access Permissions, except that
  where that description refers to execute permission for a process with
  appropriate privileges, an implementation may indicate success for
  X_OK even if execute permission is not granted to any user.

The XBD File Access Permissions text says:

  If a process has appropriate privileges:
    […]
    If execute permission is requested, access shall be granted if
    execute permission is granted to at least one user by the file
    permission bits or by an alternate access control mechanism;
    otherwise, access shall be denied.

I believe that's what's occurring here. Your commit message, however,
should contain some text that explains that AIX takes this liberty
provided by POSIX, and why that causes problems for Git (i.e., what
problems the user will see). Ideally, the reader of the commit message
will know the relevant details about this issue from your commit message
without needing to consult the standard itself.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> What POSIX says on this is the following:
>
>   If any access permissions are checked, each shall be checked
>   individually, as described in XBD File Access Permissions, except that
>   where that description refers to execute permission for a process with
>   appropriate privileges, an implementation may indicate success for
>   X_OK even if execute permission is not granted to any user.

Do I take this "not granted to any user" as "no +x bit is set for
owner, group or other", and "a process with appropriate privileges"
as "running as root"?

The latter half feels iffy, if the system is still allowed to fail
execution by "a process with appropriate privileges", leading to
inconsistent answer from access(2) and behaviour by execv(2).  But
at least that explains what was observed.

> I believe that's what's occurring here. Your commit message, however,
> should contain some text that explains that AIX takes this liberty
> provided by POSIX, and why that causes problems for Git (i.e., what
> problems the user will see). Ideally, the reader of the commit message
> will know the relevant details about this issue from your commit message
> without needing to consult the standard itself.

Very true.

Thanks.

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

On Wed, Apr 24, 2019 at 09:55:04AM +0900, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > What POSIX says on this is the following:
> >
> >   If any access permissions are checked, each shall be checked
> >   individually, as described in XBD File Access Permissions, except that
> >   where that description refers to execute permission for a process with
> >   appropriate privileges, an implementation may indicate success for
> >   X_OK even if execute permission is not granted to any user.
> 
> Do I take this "not granted to any user" as "no +x bit is set for
> owner, group or other", and "a process with appropriate privileges"
> as "running as root"?

Yes. The language of the former is designed to allow ACLs or other
mechanisms, and "appropriate privileges" is the POSIX term for "root
permissions". The latter is also designed to allow implementations
leeway to implement an alternate mechanism.

Sorry for not explaining that up front; I'm so used to POSIX-speak by
now that it doesn't seem strange to me.

> The latter half feels iffy, if the system is still allowed to fail
> execution by "a process with appropriate privileges", leading to
> inconsistent answer from access(2) and behaviour by execv(2).  But
> at least that explains what was observed.

Yeah, I don't love that POSIX makes this exception, but it is what it
is.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[Junio C Hamano]

"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.

>> 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@google.com/

Thanks.

Re: [PATCH] git-compat-util: work around for access(X_OK) under root

[CHIGOT, CLEMENT]

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.