threads / patch / 28556

patchMakefile: do not set setgid bit on directories on GNU/kFreeBSD

Subject: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

## tl;dr

7 messages between Oct 3, 2011 and Oct 24, 2011. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Jonathan Nieder· Oct 3, 2011, 06:41 UTC · lore

The g+s bit on directories to make group ownership inherited is a SysVism --- BSD and most of its descendants do not need it since they do the sane thing by default without g+s. In fact, on some filesystems (but not all --- tmpfs works this way but UFS does not), the kernel of FreeBSD does not even allow non-root users to set setgid bit on directories and produces errors when one tries:

	$ git init --shared dir
	fatal: Could not make /tmp/dir/.git/refs writable by group

Since the setgid bit would only mean "do what you were going to do already", it's better to avoid setting it. Accordingly, ever since v1.5.5-rc0~59^2 (Do not use GUID on dir in git init --share=all on FreeBSD, 2008-03-05), git on true FreeBSD has done exactly that. Set DIR_HAS_BSD_GROUP_SEMANTICS in the makefile for GNU/kFreeBSD, too, so machines that use glibc with the kernel of FreeBSD get the same fix.

This fixes t0001-init.sh and t1301-shared-repo.sh on GNU/kFreeBSD when running tests with --root pointing to a directory that uses tmpfs.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sorry to have taken so long to send this one out.  Anyway, it seems
to me like the right thing to do.  Petr, what do you think?
 Makefile |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to Makefile +1 −0
diff --git a/Makefile b/Makefile
index 8d6d4515..924749ed 100644
--- a/Makefile
+++ b/Makefile
@@ -820,6 +820,7 @@ ifeq ($(uname_S),GNU/kFreeBSD)
 	NO_STRLCPY = YesPlease
 	NO_MKSTEMPS = YesPlease
 	HAVE_PATHS_H = YesPlease
+	DIR_HAS_BSD_GROUP_SEMANTICS = YesPlease
 endif
 ifeq ($(uname_S),UnixWare)
 	CC = cc

-- 
1.7.7.rc1
Jonathan Nieder· Oct 3, 2011, 07:19 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

Jonathan Nieder wrote:
Show 6 quoted lines
> Since the setgid bit would only mean "do what you were going to do
> already", it's better to avoid setting it.  Accordingly, ever since
> v1.5.5-rc0~59^2 (Do not use GUID on dir in git init --share=all on
> FreeBSD, 2008-03-05), git on true FreeBSD has done exactly that.  Set
> DIR_HAS_BSD_GROUP_SEMANTICS in the makefile for GNU/kFreeBSD, too, so
> machines that use glibc with the kernel of FreeBSD get the same fix.
[...]
> Sorry to have taken so long to send this one out.  Anyway, it seems
> to me like the right thing to do.  Petr, what do you think?
fwiw:
Acked-by: Petr Salinger <Petr.Salinger@seznam.cz>
Thanks for looking it over.
Junio C Hamano· Oct 3, 2011, 19:16 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 17 quoted lines
> Jonathan Nieder wrote:
>
>> Since the setgid bit would only mean "do what you were going to do
>> already", it's better to avoid setting it.  Accordingly, ever since
>> v1.5.5-rc0~59^2 (Do not use GUID on dir in git init --share=all on
>> FreeBSD, 2008-03-05), git on true FreeBSD has done exactly that.  Set
>> DIR_HAS_BSD_GROUP_SEMANTICS in the makefile for GNU/kFreeBSD, too, so
>> machines that use glibc with the kernel of FreeBSD get the same fix.
> [...]
>> Sorry to have taken so long to send this one out.  Anyway, it seems
>> to me like the right thing to do.  Petr, what do you think?
>
> fwiw:
>
> Acked-by: Petr Salinger <Petr.Salinger@seznam.cz>
>
> Thanks for looking it over.
Sorry, this is very confusing. Are JN and PS one and the same person?
Sverre Rabbelier· Oct 3, 2011, 19:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

Heya,
On Mon, Oct 3, 2011 at 21:16, Junio C Hamano <gitster@pobox.com> wrote:
> Sorry, this is very confusing. Are JN and PS one and the same person?
I would assume PS mailed JN off list?
-- 
Cheers,

Sverre Rabbelier
Jonathan Nieder· Oct 22, 2011, 11:11 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

(people cc-ed: your input would be welcome on [*] below. See commit 81a24b52, "Do not use GUID on dir in git init --shared=all on FreeBSD" for context)

Hi Junio,
>From Documentation/RelNotes/1.7.7.1.txt:
 * On some BSD systems, adding +s bit on directories is detrimental
   (it is not necessary on BSD to begin with). The installation
   procedure has been updated to take this into account.

I assume this is referring to 0b20dd8f (Makefile: do not set setgid bit on directories on GNU/kFreeBSD, 2011-10-03), which admittedly does have a subject line that suggests it would be about that (sorry about that). The change was actually about "git init -s" which sets the setgid bit on SysV-style systems to allow shared access to a repository (and can provoke errors on BSD-style systems, depending on how permissive the filesystem in use wants to be).

More to the point, the patch was just taking a fix that arrived for FreeBSD in v1.5.5 days and making it also apply to machines using an (obscure) GNU userland/FreeBSD kernel mixture.

By the way, maybe other BSD-style ports (NetBSD, OpenBSD) should be setting DIR_HAS_BSD_GROUP_SEMANTICS to get this fix, too[*]? Then the release notes could look something like this:

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/RelNotes/1.7.7.1.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Show changes to diff +3 −2
diff --git i/Documentation/RelNotes/1.7.7.1.txt w/Documentation/RelNotes/1.7.7.1.txt
index fecfac8a..e3c29ff0 100644
--- i/Documentation/RelNotes/1.7.7.1.txt
+++ w/Documentation/RelNotes/1.7.7.1.txt
@@ -5,8 +5,9 @@ Fixes since v1.7.7
 ------------------
 
  * On some BSD systems, adding +s bit on directories is detrimental
-   (it is not necessary on BSD to begin with). The installation
-   procedure has been updated to take this into account.
+   (it is not necessary on BSD to begin with). "git init --shared"
+   has been updated to take this into account without extra makefile
+   settings on platforms the Makefile knows about.
 
  * After incorrectly written third-party tools store a tag object in
    HEAD, git diagnosed it as a repository corruption and refused to
Greg Troxel· Oct 24, 2011, 23:07 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Makefile: do not set setgid bit on directories on GNU/kFreeBSD

   * On some BSD systems, adding +s bit on directories is detrimental
     (it is not necessary on BSD to begin with). The installation
     procedure has been updated to take this into account.

I looked at the NetBSD 5 sources, and as expected files are created (unconditionally) with the gid of the parent directory.

Setting the setgid flag is only allowed if the inode's gid is in the process gid set. This is really about files that might be executed, but the check is independent of regular file/directory.

"git init --shared" creates a repository, mode 2775, and that normally seems fine. It seems good to have the sgid bit on, in case the repository is transferred to another machine with different semantics, and it's a clue to humans about the intended behavior, even if it's non-optional on BSD.

I created a directory, mode 755, owned by me, and with group that I *do not* belong to. Then, "git init --shared" produced:

  fatal: Could not make /home/gdt/FOO/.git/refs writable by group
but really the issue was setting the sgid bit:

# all with git version 1.7.6.3 13 $ l -d .git/refs/ drwxr-xr-x 2 gdt kmem 512 Oct 24 18:53 .git/refs/ 14 $ chmod g+w .git/refs/ 15 $ l -d .git/refs/ drwxrwxr-x 2 gdt kmem 512 Oct 24 18:53 .git/refs/ 16 $ chmod g+s .git/refs/ chmod: .git/refs/: Operation not permitted

However, this is a pathological situation, because I've created a shared repository that I can write to because I own it, and group kmem people can write to because they're in the group, but I couldn't write to other group kmem resources.

Is this not-allowed-to-set-setgid issue the problem the patch is trying to avoid? Or something else?

I did run the regression tests at one point and don't remember this failing.

So all in all I am agnostic as to whether DIR_HAS_BSD_GROUP_SEMANTICS should be defined on NetBSD; personally I prefer to see the setgid bits.

← back to recent threads