threads / patch / 12468

patchFix git init --shared=all on FreeBSD 4.11

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

## tl;dr

10 messages between Mar 3, 2008 and Mar 4, 2008. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Alex Riesen· Mar 3, 2008, 23:44 UTC · lore

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(-)
Show changes to path.c +1 −0
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
Junio C Hamano· Mar 4, 2008, 00:31 UTC · re: Alex Riesen · lore

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

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.
Show 10 quoted lines
> 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.

Alex Riesen· Mar 4, 2008, 07:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Tue, Mar 04, 2008 01:31:12 +0100:
Show 7 quoted lines
> 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.
Show 26 quoted lines
> > 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).

Junio C Hamano· Mar 4, 2008, 08:08 UTC · re: Alex Riesen · lore

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

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.
Alex Riesen· Mar 4, 2008, 20:20 UTC · re: Junio C Hamano · lore

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

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:
Show 10 quoted lines
> 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(-)
Show changes to path.c +2 −0
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
Junio C Hamano· Mar 4, 2008, 20:44 UTC · re: Alex Riesen · lore

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

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.
 ...
Show 12 quoted lines
> 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.
Alex Riesen· Mar 4, 2008, 21:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100:
Show 22 quoted lines
> 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.
Alex Riesen· Mar 4, 2008, 23:15 UTC · re: Junio C Hamano · lore

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

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.
Show 7 quoted lines
>     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(-)
Show changes to 3 files +11 −1

Makefile, git-compat-util.h, path.c

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
Johannes Schindelin· Mar 4, 2008, 20:50 UTC · re: Alex Riesen · lore

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

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

Alex Riesen· Mar 4, 2008, 21:46 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin, Tue, Mar 04, 2008 21:50:34 +0100:
Show 10 quoted lines
> 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.
Show 5 quoted lines
> >  	if (S_ISDIR(mode))
> >  		mode |= S_ISGID;
> 
> What problem does FreeBSD < 4 have with that?
> 
No idea.

← back to recent threads