[PATCH] Fix git init --shared=all on FreeBSD 4.11

[PATCH] Fix git init --shared=all on FreeBSD 4.11

[Alex Riesen]

At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to
a non-root user.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

I seem to get really weird systems lately...

 path.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/path.c b/path.c
index af27161..4865e98 100644
--- a/path.c
+++ b/path.c
@@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)
 		return 0;
 	if (lstat(path, &st) < 0)
 		return -1;
+	st.st_mode &= 07777 & ~(S_ISUID|S_ISGID);
 	mode = st.st_mode;
 	if (mode & S_IRUSR)
 		mode |= (shared_repository == PERM_GROUP
-- 
1.5.4.3.469.gf84e2

Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to
> a non-root user.

Sorry, but I do not understand this change.

> diff --git a/path.c b/path.c
> index af27161..4865e98 100644
> --- a/path.c
> +++ b/path.c
> @@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)
>  		return 0;
>  	if (lstat(path, &st) < 0)
>  		return -1;
> +	st.st_mode &= 07777 & ~(S_ISUID|S_ISGID);
>  	mode = st.st_mode;

If the thing is a directory, we say later in the code that we want to see
S_ISGID set, like this:

	...
	if (S_ISDIR(mode))
		mode |= S_ISGID;
	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
		return -2;
	return 0;

and then we compare with st.st_mode so that we do not chmod() what's
already good  Your change means we will always try to chmod all the
directories, and your explanation suggests that such a chmod to do g+s on
directories would also fail (and your patch does not fix it -- we actively
try to make sure directories have g+s set).

	Side note. the wording in your message, "does not allow changing",
	is very unclear.  Do you mean "non-root cannot do u+s,g+s"?  Or do
	you mean "non-root cannot do u+s,g+s, non-root cannot do u-s,g-s
	either"?

For regular files, I do not think we have any reason to set u+s or g+s
ourselves, and we do not try to do so either.  As long as the original
st.st_mode does not have such bits set, the mode we will pass to chmod for
regular would not try to set them.  If you already had u+s,g+s when you
read st.st_mode that's a different story, but then I do not know why you
had such a file to begin with.

I do not mind a change to make sure we do u-s,g-s on regular files, but I
do not think it is necessary, and I am curious why you had files with such
perm bits to begin with.

Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11

[Alex Riesen]

Junio C Hamano, Tue, Mar 04, 2008 01:31:12 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to
> > a non-root user.
> 
> Sorry, but I do not understand this change.
> 

Now that I look at it again, I admint I don't understand what I was
thinking either. I see the problem in t1301-shared-repo.sh: it fails
to chmod(042775, ".git/refs") with EPERM.

Will re-investigate.

> > diff --git a/path.c b/path.c
> > index af27161..4865e98 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)
> >  		return 0;
> >  	if (lstat(path, &st) < 0)
> >  		return -1;
> > +	st.st_mode &= 07777 & ~(S_ISUID|S_ISGID);
> >  	mode = st.st_mode;
> 
> If the thing is a directory, we say later in the code that we want to see
> S_ISGID set, like this:
> 
> 	...
> 	if (S_ISDIR(mode))
> 		mode |= S_ISGID;
> 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
> 		return -2;
> 	return 0;
> 
> and then we compare with st.st_mode so that we do not chmod() what's
> already good  Your change means we will always try to chmod all the
> directories, and your explanation suggests that such a chmod to do g+s on
> directories would also fail (and your patch does not fix it -- we actively
> try to make sure directories have g+s set).

The change is very bogus. Dunno how it happened...

> 	Side note. the wording in your message, "does not allow changing",
> 	is very unclear.  Do you mean "non-root cannot do u+s,g+s"?  Or do
> 	you mean "non-root cannot do u+s,g+s, non-root cannot do u-s,g-s
> 	either"?

chmod(2), as it was, just fails. It seemed like the ordinary users
could not do g+s (which is what the function is actually supposed to
do).

> I do not mind a change to make sure we do u-s,g-s on regular files, but I
> do not think it is necessary, and I am curious why you had files with such
> perm bits to begin with.

It is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just
does not work (now I am just observing, no coding).

Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> It is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just
> does not work (now I am just observing, no coding).

IIRC, g+s on directory to make group ownership inherited was a SysVism;
BSD did not need that as it did the sane thing by default without g+s.

Perhaps we should make it conditional.

[PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2

[Alex Riesen]

It does not allow changing the bit to a non-root user.
This fixes t1301-shared-repo.sh on the platform.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Tue, Mar 04, 2008 09:08:24 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > It is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just
> > does not work (now I am just observing, no coding).
> 
> IIRC, g+s on directory to make group ownership inherited was a SysVism;
> BSD did not need that as it did the sane thing by default without g+s.
> 
> Perhaps we should make it conditional.
> 

Perhaps like this (FreeBSD 4 and older).

Can someone try the change on the later versions?

 path.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/path.c b/path.c
index af27161..6f09ba2 100644
--- a/path.c
+++ b/path.c
@@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)
 			 : (shared_repository == PERM_EVERYBODY
 			    ? (S_IXGRP|S_IXOTH)
 			    : 0));
+#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)
 	if (S_ISDIR(mode))
 		mode |= S_ISGID;
+#endif
 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
 		return -2;
 	return 0;
-- 
1.5.4.3.469.gf84e2

Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> It does not allow changing the bit to a non-root user.
> This fixes t1301-shared-repo.sh on the platform.

 ...

> diff --git a/path.c b/path.c
> index af27161..6f09ba2 100644
> --- a/path.c
> +++ b/path.c
> @@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)
>  			 : (shared_repository == PERM_EVERYBODY
>  			    ? (S_IXGRP|S_IXOTH)
>  			    : 0));
> +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)
>  	if (S_ISDIR(mode))
>  		mode |= S_ISGID;
> +#endif

As usual, please do something like

(1) In git-compat-util.h

    #ifndef MKDIR_HAS_BSD_GROUP_SEMANTICS
    #define FORCE_BSD_GROUP_SEMANTICS S_ISGID
    #else
    #define FORCE_BSD_GROUP_SEMANTICS 0
    #endif

(2) In Makefile (and configure.in perhaps)

    ifeq ($(uname_S),FreeBSD)
    	MKDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
    endif
    ...
    ifdef MKDIR_HAS_BSD_GROUP_SEMANTICS
        COMPAT_CFLAGS += -DMKDIR_HAS_BSD_GROUP_SEMANTICS
    endif

(3) In the above codepath:

	if (S_ISDIR(mode))
        	mode |= FORCE_BSD_GROUP_SEMANTICS;

or something.  Avoid #if directly based on platform names in *.c files.

Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2

[Alex Riesen]

Junio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > It does not allow changing the bit to a non-root user.
> > This fixes t1301-shared-repo.sh on the platform.
> 
>  ...
> 
> > diff --git a/path.c b/path.c
> > index af27161..6f09ba2 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)
> >  			 : (shared_repository == PERM_EVERYBODY
> >  			    ? (S_IXGRP|S_IXOTH)
> >  			    : 0));
> > +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)
> >  	if (S_ISDIR(mode))
> >  		mode |= S_ISGID;
> > +#endif
> 
> As usual, please do something like
> 

Right. Will do.

[PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD

[Alex Riesen]

It does not allow changing the bit to a non-root user.
This fixes t1301-shared-repo.sh on the platform.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Junio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100:
> (2) In Makefile (and configure.in perhaps)

Just the configure.ac is a bit too much for today.

>     ifeq ($(uname_S),FreeBSD)
>     	MKDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
>     endif
>     ...
>     ifdef MKDIR_HAS_BSD_GROUP_SEMANTICS
>         COMPAT_CFLAGS += -DMKDIR_HAS_BSD_GROUP_SEMANTICS
>     endif

I tweaked the names a bit.

 Makefile          |    4 ++++
 git-compat-util.h |    6 ++++++
 path.c            |    2 +-
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index ca5aad9..6e857e6 100644
--- a/Makefile
+++ b/Makefile
@@ -478,6 +478,7 @@ ifeq ($(uname_S),FreeBSD)
 	NO_MEMMEM = YesPlease
 	BASIC_CFLAGS += -I/usr/local/include
 	BASIC_LDFLAGS += -L/usr/local/lib
+	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 endif
 ifeq ($(uname_S),OpenBSD)
 	NO_STRCASESTR = YesPlease
@@ -747,6 +748,9 @@ ifdef THREADED_DELTA_SEARCH
 	EXTLIBS += -lpthread
 	LIB_OBJS += thread-utils.o
 endif
+ifdef DIR_HAS_BSD_GROUP_SEMANTICS
+	COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
+endif
 
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK=NoThanks
diff --git a/git-compat-util.h b/git-compat-util.h
index 2a40703..5912443 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -437,4 +437,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #define qsort git_qsort
 #endif
 
+#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
+# define FORCE_DIR_SET_GID S_ISGID
+#else
+# define FORCE_DIR_SET_GID 0
+#endif
+
 #endif
diff --git a/path.c b/path.c
index af27161..f4ed979 100644
--- a/path.c
+++ b/path.c
@@ -283,7 +283,7 @@ int adjust_shared_perm(const char *path)
 			    ? (S_IXGRP|S_IXOTH)
 			    : 0));
 	if (S_ISDIR(mode))
-		mode |= S_ISGID;
+		mode |= FORCE_DIR_SET_GID;
 	if ((mode & st.st_mode) != mode && chmod(path, mode) < 0)
 		return -2;
 	return 0;
-- 
1.5.4.3.272.gf0c3

Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2

[Johannes Schindelin]

Hi,

On Tue, 4 Mar 2008, Alex Riesen wrote:

> +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)

We are not avoiding to clutter the source code with ugly __MINGW32__ 
conditionals with a huge effort, so that you can clutter it with 
__FreeBSD__ conditionals.


>  	if (S_ISDIR(mode))
>  		mode |= S_ISGID;

What problem does FreeBSD < 4 have with that?

Ciao,
Dscho

Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2

[Alex Riesen]

Johannes Schindelin, Tue, Mar 04, 2008 21:50:34 +0100:
> Hi,
> 
> On Tue, 4 Mar 2008, Alex Riesen wrote:
> 
> > +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)
> 
> We are not avoiding to clutter the source code with ugly __MINGW32__ 
> conditionals with a huge effort, so that you can clutter it with 
> __FreeBSD__ conditionals.
> 

Will fix.

> >  	if (S_ISDIR(mode))
> >  		mode |= S_ISGID;
> 
> What problem does FreeBSD < 4 have with that?
> 

No idea.