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

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

[Jonathan Nieder]

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(-)

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

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

[Jonathan Nieder]

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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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?

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

[Sverre Rabbelier]

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

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

[Jonathan Nieder]

Sverre Rabbelier wrote:

> I would assume PS mailed JN off list?

Yes, that's right.  Sorry for the cryptic message.

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

[Jonathan Nieder]

(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(-)

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
-- 

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

[Greg Troxel]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]


   * 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.


[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]