[v2] git-compat-util: work around for access(X_OK) under root
diff mbox series

Message ID AM6PR02MB4950ED66291B16F35BE61A26EA3C0@AM6PR02MB4950.eurprd02.prod.outlook.com
State New
Headers show
Series
  • [v2] git-compat-util: work around for access(X_OK) under root
Related show

Commit Message

CHIGOT, CLEMENT April 24, 2019, noon UTC
On AIX, access(X_OK) may success when run under root even if the
execution isn't possible. This comes from the POSIX specifications
which say:
"... for a process with appropriate privileges, an implementation
 may indicate success for X_OK even if execute permission is not
 granted to any user."

This behavior can lead hook programs to have their execution refused:
 "git commit -m content
  fatal: cannot exec '.git/hooks/pre-commit': Permission denied"

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>
Message-ID: <AM6PR02MB4950BB0152893633FF95DC99EA230@AM6PR02MB4950.eurprd02.prod.outlook.com>
---
 Makefile          |  8 ++++++++
 compat/access.c   | 30 ++++++++++++++++++++++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  7 +++++++
 4 files changed, 46 insertions(+)
 create mode 100644 compat/access.c

Comments

Junio C Hamano April 24, 2019, 1:39 p.m. UTC | #1
"CHIGOT, CLEMENT" <clement.chigot@atos.net> writes:

> On AIX, access(X_OK) may success when run under root even if the

s/success/succeed/; 

Also perhaps s/under/as/.

> execution isn't possible. This comes from the POSIX specifications

s/comes from/behaviour is allowed by/;  I agree with you that AIX
behaviour is suboptimal and I do not think we want to give an
impression that POSIX encourages such an illogical behaviour.  It
merely is permitted.

> which say:
> "... for a process with appropriate privileges, an implementation
>  may indicate success for X_OK even if execute permission is not
>  granted to any user."
>
> This behavior can lead hook programs to have their execution refused:

OK.

>  "git commit -m content
>   fatal: cannot exec '.git/hooks/pre-commit': Permission denied"

I am not sure what the double-quotes around these two lines are
about.  Perhaps drop them, and if you want to clarify which part is
your word and which part is quoted material, have blank lines to
delineate instead, perhap like:

        ... POSIX says:

            ... for a process with appropriate ...
            ... to any user.

        This behaviour can fail the execution of hooks, like so:

            $ git commit
            fatal: cannot exec '.git/hooks/pre-commit': Permission denied

        Add NEED_ACCESS_ROOT_HANDLER in order to...

> is root.
>
> Signed-off-by: Clément Chigot <clement.chigot@atos.net>
> Message-ID: <AM6PR02MB4950BB0152893633FF95DC99EA230@AM6PR02MB4950.eurprd02.prod.outlook.com>

Drop "Message-Id:" from the footer.

> ---
> ...
> diff --git a/compat/access.c b/compat/access.c
> new file mode 100644
> index 0000000000..fcfaefb0c0
> --- /dev/null
> +++ b/compat/access.c
> @@ -0,0 +1,30 @@
> +#define COMPAT_CODE_ACCESS

I am torn between just using COMPAT_CODE like the other one does, or
introducing the new symbol like this.  If we were to do the latter,
perhaps we should give the original one a more specific name as well
(e.g. COMPAT_CODE_FILENO or something like that).

> +#include "../git-compat-util.h"
> +
> +/* Do the same thing access(2) does, but use the effective uid,
> + * 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;
> +
> +	/* do not interfere a normal user */
> +	if (geteuid())
> +		return access(path, mode);

OK.

> +	if (stat(path, &st) < 0)
> +		return -1;
> +
> +	/* 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. */

	/*
	 * Our multi-line comment looks like this,
	 * with opening slash-asterisk and closing
	 * asterisk-slash on their own lines.
	 */

> +	if (st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))
> +		return 0;
> +
> +	errno = EACCES;
> +	return -1;
> +}

OK.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 31b47932bd..d0cb380522 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1242,6 +1242,13 @@ int git_fileno(FILE *stream);
>  # endif
>  #endif
>  
> +#ifdef NEED_ACCESS_ROOT_HANDLER
> +int git_access(const char *path, int mode);
> +# ifndef COMPAT_CODE_ACCESS

Notice that the fileno thing we are trying to mimick protects us
from a system header that defines the macro we are about to define,
which is a good practice to prevent compilers from complaining
against redefinition.  We should imitate it like this here:

   #  ifdef access
   #  undef access
   #  endif

> +#  define access(path, mode) git_access(path, mode)
> +# endif
> +#endif
> +

Other than that, looks good to me.
CHIGOT, CLEMENT April 24, 2019, 1:58 p.m. UTC | #2
> From: Junio C Hamano <gitster@pobox.com>
> "CHIGOT, CLEMENT" <clement.chigot@atos.net> writes:
> 
> > On AIX, access(X_OK) may success when run under root even if the
> 
> s/success/succeed/;
> 
> Also perhaps s/under/as/.
> 
> > execution isn't possible. This comes from the POSIX specifications
> 
> s/comes from/behaviour is allowed by/;  I agree with you that AIX
> behaviour is suboptimal and I do not think we want to give an
> impression that POSIX encourages such an illogical behaviour.  It
> merely is permitted.

Yes, that's better

> 
> >  "git commit -m content
> >   fatal: cannot exec '.git/hooks/pre-commit': Permission denied"
> 
> I am not sure what the double-quotes around these two lines are
> about.  Perhaps drop them, and if you want to clarify which part is
> your word and which part is quoted material, have blank lines to
> delineate instead, perhap like:
> 
>         ... POSIX says:
> 
>             ... for a process with appropriate ...
>             ... to any user.
> 
>         This behaviour can fail the execution of hooks, like so:
> 
>             $ git commit
>             fatal: cannot exec '.git/hooks/pre-commit': Permission denied
> 
>         Add NEED_ACCESS_ROOT_HANDLER in order to...

I'm used to do that. But if you think that's clearer, I'll do it. 

> 
> > is root.
> >
> > Signed-off-by: Clément Chigot <clement.chigot@atos.net>
> > Message-ID: <AM6PR02MB4950BB0152893633FF95DC99EA230@AM6PR02MB4950.eurprd02.prod.outlook.com>
> 
> Drop "Message-Id:" from the footer.
> 
> > ---
> > ...
> > diff --git a/compat/access.c b/compat/access.c
> > new file mode 100644
> > index 0000000000..fcfaefb0c0
> > --- /dev/null
> > +++ b/compat/access.c
> > @@ -0,0 +1,30 @@
> > +#define COMPAT_CODE_ACCESS
> 
> I am torn between just using COMPAT_CODE like the other one does, or
> introducing the new symbol like this.  If we were to do the latter,
> perhaps we should give the original one a more specific name as well
> (e.g. COMPAT_CODE_FILENO or something like that).
>

I think using the same define is possible for now. But it might become confusing
if others are needed. Moreover, I'm not sure it will work if, for example,
fileno is needed inside git_access. That's very unlikely to happen, but
it seems better to have one COMPAT_CODE_X defined for each special handler. 
I'll let you decide., anyway. 

> > +     if (stat(path, &st) < 0)
> > +             return -1;
> > +
> > +     /* 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. */
> 
>         /*
>          * Our multi-line comment looks like this,
>          * with opening slash-asterisk and closing
>          * asterisk-slash on their own lines.
>          */

.. I've forgotten this one, sorry. 

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 31b47932bd..d0cb380522 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -1242,6 +1242,13 @@ int git_fileno(FILE *stream);
> >  # endif
> >  #endif


> > 
> > +#ifdef NEED_ACCESS_ROOT_HANDLER
> > +int git_access(const char *path, int mode);
> > +# ifndef COMPAT_CODE_ACCESS
> 
> Notice that the fileno thing we are trying to mimick protects us
> from a system header that defines the macro we are about to define,
> which is a good practice to prevent compilers from complaining
> against redefinition.  We should imitate it like this here:
> 
>    #  ifdef access
>    #  undef access
>    #  endif
> 
> > +#  define access(path, mode) git_access(path, mode)
> > +# endif
> > +#endif
> > +

Ok. 

> 
> Other than that, looks good to me.

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index 9f1b6e8926..6ac7218106 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() under root may success for X_OK
+# even if execution permission isn't granted for any user.
+#
 # 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..fcfaefb0c0
--- /dev/null
+++ b/compat/access.c
@@ -0,0 +1,30 @@ 
+#define COMPAT_CODE_ACCESS
+#include "../git-compat-util.h"
+
+/* Do the same thing access(2) does, but use the effective uid,
+ * 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;
+
+	/* do not interfere a normal user */
+	if (geteuid())
+		return access(path, mode);
+
+	if (stat(path, &st) < 0)
+		return -1;
+
+	/* 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;
+}
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..d0cb380522 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1242,6 +1242,13 @@  int git_fileno(FILE *stream);
 # endif
 #endif
 
+#ifdef NEED_ACCESS_ROOT_HANDLER
+int git_access(const char *path, int mode);
+# ifndef COMPAT_CODE_ACCESS
+#  define access(path, mode) git_access(path, mode)
+# endif
+#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