{"thread":{"id":"12468","subject":"[PATCH] Fix git init --shared=all on FreeBSD 4.11","startedAt":"2008-03-03T23:44:06Z","lastAt":"2008-03-04T23:15:39Z","messageCount":10,"participants":["Alex Riesen","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70855","messageId":"20080303234406.GA28158@steel.home","threadId":"12468","inReplyTo":null,"subject":"[PATCH] Fix git init --shared=all on FreeBSD 4.11","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-03T23:44:06Z","receivedAt":"2008-03-03T23:44:06Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to\na non-root user.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nI seem to get really weird systems lately...\n\n path.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex af27161..4865e98 100644\n--- a/path.c\n+++ b/path.c\n@@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)\n \t\treturn 0;\n \tif (lstat(path, &st) < 0)\n \t\treturn -1;\n+\tst.st_mode &= 07777 & ~(S_ISUID|S_ISGID);\n \tmode = st.st_mode;\n \tif (mode & S_IRUSR)\n \t\tmode |= (shared_repository == PERM_GROUP\n-- \n1.5.4.3.469.gf84e2\n\n"},{"id":"70860","messageId":"7v1w6rfhyn.fsf@gitster.siamese.dyndns.org","threadId":"12468","inReplyTo":"20080303234406.GA28158@steel.home","subject":"Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T00:31:12Z","receivedAt":"2008-03-04T00:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to\n> a non-root user.\n\nSorry, but I do not understand this change.\n\n> diff --git a/path.c b/path.c\n> index af27161..4865e98 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)\n>  \t\treturn 0;\n>  \tif (lstat(path, &st) < 0)\n>  \t\treturn -1;\n> +\tst.st_mode &= 07777 & ~(S_ISUID|S_ISGID);\n>  \tmode = st.st_mode;\n\nIf the thing is a directory, we say later in the code that we want to see\nS_ISGID set, like this:\n\n\t...\n\tif (S_ISDIR(mode))\n\t\tmode |= S_ISGID;\n\tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n\t\treturn -2;\n\treturn 0;\n\nand then we compare with st.st_mode so that we do not chmod() what's\nalready good  Your change means we will always try to chmod all the\ndirectories, and your explanation suggests that such a chmod to do g+s on\ndirectories would also fail (and your patch does not fix it -- we actively\ntry to make sure directories have g+s set).\n\n\tSide note. the wording in your message, \"does not allow changing\",\n\tis very unclear.  Do you mean \"non-root cannot do u+s,g+s\"?  Or do\n\tyou mean \"non-root cannot do u+s,g+s, non-root cannot do u-s,g-s\n\teither\"?\n\nFor regular files, I do not think we have any reason to set u+s or g+s\nourselves, and we do not try to do so either.  As long as the original\nst.st_mode does not have such bits set, the mode we will pass to chmod for\nregular would not try to set them.  If you already had u+s,g+s when you\nread st.st_mode that's a different story, but then I do not know why you\nhad such a file to begin with.\n\nI do not mind a change to make sure we do u-s,g-s on regular files, but I\ndo not think it is necessary, and I am curious why you had files with such\nperm bits to begin with.\n"},{"id":"70897","messageId":"20080304072519.GA3070@steel.home","threadId":"12468","inReplyTo":"7v1w6rfhyn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-04T07:25:19Z","receivedAt":"2008-03-04T07:25:19Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Tue, Mar 04, 2008 01:31:12 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > At least FreeBSD 4.11p2 does not allow changing SUID/GUID bits to\n> > a non-root user.\n> \n> Sorry, but I do not understand this change.\n> \n\nNow that I look at it again, I admint I don't understand what I was\nthinking either. I see the problem in t1301-shared-repo.sh: it fails\nto chmod(042775, \".git/refs\") with EPERM.\n\nWill re-investigate.\n\n> > diff --git a/path.c b/path.c\n> > index af27161..4865e98 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -265,6 +265,7 @@ int adjust_shared_perm(const char *path)\n> >  \t\treturn 0;\n> >  \tif (lstat(path, &st) < 0)\n> >  \t\treturn -1;\n> > +\tst.st_mode &= 07777 & ~(S_ISUID|S_ISGID);\n> >  \tmode = st.st_mode;\n> \n> If the thing is a directory, we say later in the code that we want to see\n> S_ISGID set, like this:\n> \n> \t...\n> \tif (S_ISDIR(mode))\n> \t\tmode |= S_ISGID;\n> \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n> \t\treturn -2;\n> \treturn 0;\n> \n> and then we compare with st.st_mode so that we do not chmod() what's\n> already good  Your change means we will always try to chmod all the\n> directories, and your explanation suggests that such a chmod to do g+s on\n> directories would also fail (and your patch does not fix it -- we actively\n> try to make sure directories have g+s set).\n\nThe change is very bogus. Dunno how it happened...\n\n> \tSide note. the wording in your message, \"does not allow changing\",\n> \tis very unclear.  Do you mean \"non-root cannot do u+s,g+s\"?  Or do\n> \tyou mean \"non-root cannot do u+s,g+s, non-root cannot do u-s,g-s\n> \teither\"?\n\nchmod(2), as it was, just fails. It seemed like the ordinary users\ncould not do g+s (which is what the function is actually supposed to\ndo).\n\n> I do not mind a change to make sure we do u-s,g-s on regular files, but I\n> do not think it is necessary, and I am curious why you had files with such\n> perm bits to begin with.\n\nIt is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just\ndoes not work (now I am just observing, no coding).\n\n"},{"id":"70901","messageId":"7vir02ap3b.fsf@gitster.siamese.dyndns.org","threadId":"12468","inReplyTo":"20080304072519.GA3070@steel.home","subject":"Re: [PATCH] Fix git init --shared=all on FreeBSD 4.11","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T08:08:24Z","receivedAt":"2008-03-04T08:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> It is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just\n> does not work (now I am just observing, no coding).\n\nIIRC, g+s on directory to make group ownership inherited was a SysVism;\nBSD did not need that as it did the sane thing by default without g+s.\n\nPerhaps we should make it conditional.\n\n"},{"id":"70988","messageId":"20080304202047.GC3295@steel.home","threadId":"12468","inReplyTo":"7vir02ap3b.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-04T20:20:47Z","receivedAt":"2008-03-04T20:20:47Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"It does not allow changing the bit to a non-root user.\nThis fixes t1301-shared-repo.sh on the platform.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nJunio C Hamano, Tue, Mar 04, 2008 09:08:24 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > It is a directory. The bit 02000 is S_ISGID on FreeBSD too. It just\n> > does not work (now I am just observing, no coding).\n> \n> IIRC, g+s on directory to make group ownership inherited was a SysVism;\n> BSD did not need that as it did the sane thing by default without g+s.\n> \n> Perhaps we should make it conditional.\n> \n\nPerhaps like this (FreeBSD 4 and older).\n\nCan someone try the change on the later versions?\n\n path.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex af27161..6f09ba2 100644\n--- a/path.c\n+++ b/path.c\n@@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)\n \t\t\t : (shared_repository == PERM_EVERYBODY\n \t\t\t    ? (S_IXGRP|S_IXOTH)\n \t\t\t    : 0));\n+#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)\n \tif (S_ISDIR(mode))\n \t\tmode |= S_ISGID;\n+#endif\n \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n-- \n1.5.4.3.469.gf84e2\n\n"},{"id":"70991","messageId":"7vr6eq43te.fsf@gitster.siamese.dyndns.org","threadId":"12468","inReplyTo":"20080304202047.GC3295@steel.home","subject":"Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T20:44:29Z","receivedAt":"2008-03-04T20:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen <raa.lkml@gmail.com> writes:\n\n> It does not allow changing the bit to a non-root user.\n> This fixes t1301-shared-repo.sh on the platform.\n\n ...\n\n> diff --git a/path.c b/path.c\n> index af27161..6f09ba2 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)\n>  \t\t\t : (shared_repository == PERM_EVERYBODY\n>  \t\t\t    ? (S_IXGRP|S_IXOTH)\n>  \t\t\t    : 0));\n> +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)\n>  \tif (S_ISDIR(mode))\n>  \t\tmode |= S_ISGID;\n> +#endif\n\nAs usual, please do something like\n\n(1) In git-compat-util.h\n\n    #ifndef MKDIR_HAS_BSD_GROUP_SEMANTICS\n    #define FORCE_BSD_GROUP_SEMANTICS S_ISGID\n    #else\n    #define FORCE_BSD_GROUP_SEMANTICS 0\n    #endif\n\n(2) In Makefile (and configure.in perhaps)\n\n    ifeq ($(uname_S),FreeBSD)\n    \tMKDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n    endif\n    ...\n    ifdef MKDIR_HAS_BSD_GROUP_SEMANTICS\n        COMPAT_CFLAGS += -DMKDIR_HAS_BSD_GROUP_SEMANTICS\n    endif\n\n(3) In the above codepath:\n\n\tif (S_ISDIR(mode))\n        \tmode |= FORCE_BSD_GROUP_SEMANTICS;\n\nor something.  Avoid #if directly based on platform names in *.c files.\n\n"},{"id":"70993","messageId":"alpine.LSU.1.00.0803042048330.22527@racer.site","threadId":"12468","inReplyTo":"20080304202047.GC3295@steel.home","subject":"Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-03-04T20:50:34Z","receivedAt":"2008-03-04T20:50:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 4 Mar 2008, Alex Riesen wrote:\n\n> +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)\n\nWe are not avoiding to clutter the source code with ugly __MINGW32__ \nconditionals with a huge effort, so that you can clutter it with \n__FreeBSD__ conditionals.\n\n\n>  \tif (S_ISDIR(mode))\n>  \t\tmode |= S_ISGID;\n\nWhat problem does FreeBSD < 4 have with that?\n\nCiao,\nDscho\n\n"},{"id":"70996","messageId":"20080304214622.GD3295@steel.home","threadId":"12468","inReplyTo":"7vr6eq43te.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-04T21:46:22Z","receivedAt":"2008-03-04T21:46:22Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100:\n> Alex Riesen <raa.lkml@gmail.com> writes:\n> \n> > It does not allow changing the bit to a non-root user.\n> > This fixes t1301-shared-repo.sh on the platform.\n> \n>  ...\n> \n> > diff --git a/path.c b/path.c\n> > index af27161..6f09ba2 100644\n> > --- a/path.c\n> > +++ b/path.c\n> > @@ -282,8 +282,10 @@ int adjust_shared_perm(const char *path)\n> >  \t\t\t : (shared_repository == PERM_EVERYBODY\n> >  \t\t\t    ? (S_IXGRP|S_IXOTH)\n> >  \t\t\t    : 0));\n> > +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)\n> >  \tif (S_ISDIR(mode))\n> >  \t\tmode |= S_ISGID;\n> > +#endif\n> \n> As usual, please do something like\n> \n\nRight. Will do.\n\n"},{"id":"70997","messageId":"20080304214656.GE3295@steel.home","threadId":"12468","inReplyTo":"alpine.LSU.1.00.0803042048330.22527@racer.site","subject":"Re: [PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD 4.11p2","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-04T21:46:56Z","receivedAt":"2008-03-04T21:46:56Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Johannes Schindelin, Tue, Mar 04, 2008 21:50:34 +0100:\n> Hi,\n> \n> On Tue, 4 Mar 2008, Alex Riesen wrote:\n> \n> > +#if !defined(__FreeBSD__) || (__FreeBSD__ > 4)\n> \n> We are not avoiding to clutter the source code with ugly __MINGW32__ \n> conditionals with a huge effort, so that you can clutter it with \n> __FreeBSD__ conditionals.\n> \n\nWill fix.\n\n> >  \tif (S_ISDIR(mode))\n> >  \t\tmode |= S_ISGID;\n> \n> What problem does FreeBSD < 4 have with that?\n> \n\nNo idea.\n\n"},{"id":"71012","messageId":"20080304231539.GA8085@limbo.localdomain","threadId":"12468","inReplyTo":"7vr6eq43te.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] Do not use GUID on dir in git init --shared=all on FreeBSD","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-03-04T23:15:39Z","receivedAt":"2008-03-04T23:15:39Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"It does not allow changing the bit to a non-root user.\nThis fixes t1301-shared-repo.sh on the platform.\n\nSigned-off-by: Alex Riesen <raa.lkml@gmail.com>\n---\n\nJunio C Hamano, Tue, Mar 04, 2008 21:44:29 +0100:\n> (2) In Makefile (and configure.in perhaps)\n\nJust the configure.ac is a bit too much for today.\n\n>     ifeq ($(uname_S),FreeBSD)\n>     \tMKDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n>     endif\n>     ...\n>     ifdef MKDIR_HAS_BSD_GROUP_SEMANTICS\n>         COMPAT_CFLAGS += -DMKDIR_HAS_BSD_GROUP_SEMANTICS\n>     endif\n\nI tweaked the names a bit.\n\n Makefile          |    4 ++++\n git-compat-util.h |    6 ++++++\n path.c            |    2 +-\n 3 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ca5aad9..6e857e6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -478,6 +478,7 @@ ifeq ($(uname_S),FreeBSD)\n \tNO_MEMMEM = YesPlease\n \tBASIC_CFLAGS += -I/usr/local/include\n \tBASIC_LDFLAGS += -L/usr/local/lib\n+\tDIR_HAS_BSD_GROUP_SEMANTICS = YesPlease\n endif\n ifeq ($(uname_S),OpenBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -747,6 +748,9 @@ ifdef THREADED_DELTA_SEARCH\n \tEXTLIBS += -lpthread\n \tLIB_OBJS += thread-utils.o\n endif\n+ifdef DIR_HAS_BSD_GROUP_SEMANTICS\n+\tCOMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS\n+endif\n \n ifeq ($(TCLTK_PATH),)\n NO_TCLTK=NoThanks\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 2a40703..5912443 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -437,4 +437,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n+#ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n+# define FORCE_DIR_SET_GID S_ISGID\n+#else\n+# define FORCE_DIR_SET_GID 0\n+#endif\n+\n #endif\ndiff --git a/path.c b/path.c\nindex af27161..f4ed979 100644\n--- a/path.c\n+++ b/path.c\n@@ -283,7 +283,7 @@ int adjust_shared_perm(const char *path)\n \t\t\t    ? (S_IXGRP|S_IXOTH)\n \t\t\t    : 0));\n \tif (S_ISDIR(mode))\n-\t\tmode |= S_ISGID;\n+\t\tmode |= FORCE_DIR_SET_GID;\n \tif ((mode & st.st_mode) != mode && chmod(path, mode) < 0)\n \t\treturn -2;\n \treturn 0;\n-- \n1.5.4.3.272.gf0c3\n\n"}]}