# [PATCH] Fix git init --shared=all on FreeBSD 4.11 10 messages from 2008-03-03 to 2008-03-04. Participants: Alex Riesen, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/12468 ## Alex Riesen, 2008-03-03 23:44 Subject: [PATCH] Fix git init --shared=all on FreeBSD 4.11 Message-ID: <20080303234406.GA28158@steel.home> URL: https://gitlist.dev/e/20080303234406.GA28158%40steel.home ``` At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to a non-root user. Signed-off-by: Alex Riesen --- 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 ``` ## Junio C Hamano, 2008-03-04 00:31 Subject: Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11 Message-ID: <7v1w6rfhyn.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1w6rfhyn.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080303234406.GA28158@steel.home> ``` Alex Riesen 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. ``` ## Alex Riesen, 2008-03-04 07:25 Subject: Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11 Message-ID: <20080304072519.GA3070@steel.home> URL: https://gitlist.dev/e/20080304072519.GA3070%40steel.home In-Reply-To: <7v1w6rfhyn.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano, Tue, Mar 04, 2008 01:31:12 +0100: > Alex Riesen 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). ``` ## Junio C Hamano, 2008-03-04 08:08 Subject: Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11 Message-ID: <7vir02ap3b.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vir02ap3b.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080304072519.GA3070@steel.home> ``` Alex Riesen 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, 2008-03-04 20:20 Subject: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2 Message-ID: <20080304202047.GC3295@steel.home> URL: https://gitlist.dev/e/20080304202047.GC3295%40steel.home In-Reply-To: <7vir02ap3b.fsf@gitster.siamese.dyndns.org> ``` 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 --- Junio C Hamano, Tue, Mar 04, 2008 09:08:24 +0100: > Alex Riesen 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 ``` ## Junio C Hamano, 2008-03-04 20:44 Subject: Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2 Message-ID: <7vr6eq43te.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6eq43te.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080304202047.GC3295@steel.home> ``` Alex Riesen 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. ``` ## Johannes Schindelin, 2008-03-04 20:50 Subject: Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2 Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0803042048330.22527%40racer.site In-Reply-To: <20080304202047.GC3295@steel.home> ``` 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, 2008-03-04 21:46 Subject: Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2 Message-ID: <20080304214622.GD3295@steel.home> URL: https://gitlist.dev/e/20080304214622.GD3295%40steel.home In-Reply-To: <7vr6eq43te.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100: > Alex Riesen 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, 2008-03-04 21:46 Subject: Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2 Message-ID: <20080304214656.GE3295@steel.home> URL: https://gitlist.dev/e/20080304214656.GE3295%40steel.home In-Reply-To: ``` 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. ``` ## Alex Riesen, 2008-03-04 23:15 Subject: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD Message-ID: <20080304231539.GA8085@limbo.localdomain> URL: https://gitlist.dev/e/20080304231539.GA8085%40limbo.localdomain In-Reply-To: <7vr6eq43te.fsf@gitster.siamese.dyndns.org> ``` 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 --- 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 ```