[PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs

[PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs

[Matthieu Moy]

I talked about the problem earlier:

http://thread.gmane.org/gmane.comp.version-control.git/136720

In short: if a user wants to share a git repository using POSIX
filesystem ACLs, it seems everything works except pack file creation.
The problem is that we create the file with a mode 0600, i.e. no
permission for group, but the semantics of ACL is that this group
permission is used as ACL umask when the file has ACLs (don't ask me
why, I never understood the rational, but that's how it works).

Instead, if we let the umask do its job at the time of file creation,
then the pack file is created with the correct permission bits and
ACLs, and we don't need to chmod it after the fact. This is what this
patch serie does.

The test-case is really weak: ideally, we should have a real test with
several users interacting. But that's hardly scriptable in a portable
way, so the only testcase I added relies on getfacl and "ls -l" to find
out if the result is correct. However, I did some manual testing, and
as far as I can say, Git works very well with ACLs with this patch.

[PATCH 1/4] Add a testcase for ACL with restrictive umask.

[Matthieu Moy]

Right now, Git creates unreadable pack files on non-shared
repositories when the user has a umask of 077, even when the default
ACLs for the directory would give read/write access to a specific
user.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/t1304-default-acl.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100755 t/t1304-default-acl.sh

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
new file mode 100755
index 0000000..4ee44a1
--- /dev/null
+++ b/t/t1304-default-acl.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Matthieu Moy
+#
+
+test_description='Test repository with default ACL'
+
+. ./test-lib.sh
+
+# We need an arbitrary other user give permission to using ACLs. root
+# is a good candidate: exists on all unices, and it has permission
+# anyway, so we don't create a security hole running the testsuite.
+
+if ! setfacl -Rm u:root:rwx .; then
+    say "Skipping ACL tests: unable to use setfacl"
+    test_done
+fi
+
+setfacl -Rm d:u:"$LOGNAME":rwx .
+setfacl -Rm d:u:root:rwx .
+
+touch file.txt
+git add file.txt
+git commit -m "init"
+
+modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
+test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+	umask 077 &&
+	git gc &&
+	actual=$(modebits .git/objects/pack/*.pack) &&
+	case "$actual" in
+	-r--r-----*)
+		: happy
+		;;
+	*)
+		false
+		;;
+	esac &&
+	getfacl .git/objects/pack/*.pack > actual &&
+	grep -q "user:root:rwx" actual &&
+	grep -q "user:${LOGNAME}:rwx" actual &&
+	grep -q "mask::r--" actual &&
+	grep -q "group::---" actual
+'
+
+test_done
-- 
1.7.0.rc2.4.gc602c4

[PATCH 2/4] Move gitmkstemps to path.c

[Matthieu Moy]

This function used to be only a compatibility function, but we're
going to extend it and actually use it, so make it part of Git.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Makefile          |    1 -
 compat/mkstemps.c |   70 -----------------------------------------------------
 path.c            |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 71 deletions(-)
 delete mode 100644 compat/mkstemps.c

diff --git a/Makefile b/Makefile
index 7bf2fca..4387d42 100644
--- a/Makefile
+++ b/Makefile
@@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP
 endif
 ifdef NO_MKSTEMPS
 	COMPAT_CFLAGS += -DNO_MKSTEMPS
-	COMPAT_OBJS += compat/mkstemps.o
 endif
 ifdef NO_UNSETENV
 	COMPAT_CFLAGS += -DNO_UNSETENV
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
deleted file mode 100644
index 14179c8..0000000
--- a/compat/mkstemps.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "../git-compat-util.h"
-
-/* Adapted from libiberty's mkstemp.c. */
-
-#undef TMP_MAX
-#define TMP_MAX 16384
-
-int gitmkstemps(char *pattern, int suffix_len)
-{
-	static const char letters[] =
-		"abcdefghijklmnopqrstuvwxyz"
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-		"0123456789";
-	static const int num_letters = 62;
-	uint64_t value;
-	struct timeval tv;
-	char *template;
-	size_t len;
-	int fd, count;
-
-	len = strlen(pattern);
-
-	if (len < 6 + suffix_len) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/*
-	 * Replace pattern's XXXXXX characters with randomness.
-	 * Try TMP_MAX different filenames.
-	 */
-	gettimeofday(&tv, NULL);
-	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
-	template = &pattern[len - 6 - suffix_len];
-	for (count = 0; count < TMP_MAX; ++count) {
-		uint64_t v = value;
-		/* Fill in the random bits. */
-		template[0] = letters[v % num_letters]; v /= num_letters;
-		template[1] = letters[v % num_letters]; v /= num_letters;
-		template[2] = letters[v % num_letters]; v /= num_letters;
-		template[3] = letters[v % num_letters]; v /= num_letters;
-		template[4] = letters[v % num_letters]; v /= num_letters;
-		template[5] = letters[v % num_letters]; v /= num_letters;
-
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
-		if (fd > 0)
-			return fd;
-		/*
-		 * Fatal error (EPERM, ENOSPC etc).
-		 * It doesn't make sense to loop.
-		 */
-		if (errno != EEXIST)
-			break;
-		/*
-		 * This is a random value.  It is only necessary that
-		 * the next TMP_MAX values generated by adding 7777 to
-		 * VALUE are different with (module 2^32).
-		 */
-		value += 7777;
-	}
-	/* We return the null string if we can't find a unique file name.  */
-	pattern[0] = '\0';
-	errno = EINVAL;
-	return -1;
-}
diff --git a/path.c b/path.c
index d1fccbd..60f5b78 100644
--- a/path.c
+++ b/path.c
@@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 	return mkstemps(path, suffix_len);
 }
 
+/* Adapted from libiberty's mkstemp.c. */
+
+#undef TMP_MAX
+#define TMP_MAX 16384
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyz"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789";
+	static const int num_letters = 62;
+	uint64_t value;
+	struct timeval tv;
+	char *template;
+	size_t len;
+	int fd, count;
+
+	len = strlen(pattern);
+
+	if (len < 6 + suffix_len) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/*
+	 * Replace pattern's XXXXXX characters with randomness.
+	 * Try TMP_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	template = &pattern[len - 6 - suffix_len];
+	for (count = 0; count < TMP_MAX; ++count) {
+		uint64_t v = value;
+		/* Fill in the random bits. */
+		template[0] = letters[v % num_letters]; v /= num_letters;
+		template[1] = letters[v % num_letters]; v /= num_letters;
+		template[2] = letters[v % num_letters]; v /= num_letters;
+		template[3] = letters[v % num_letters]; v /= num_letters;
+		template[4] = letters[v % num_letters]; v /= num_letters;
+		template[5] = letters[v % num_letters]; v /= num_letters;
+
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd > 0)
+			return fd;
+		/*
+		 * Fatal error (EPERM, ENOSPC etc).
+		 * It doesn't make sense to loop.
+		 */
+		if (errno != EEXIST)
+			break;
+		/*
+		 * This is a random value.  It is only necessary that
+		 * the next TMP_MAX values generated by adding 7777 to
+		 * VALUE are different with (module 2^32).
+		 */
+		value += 7777;
+	}
+	/* We return the null string if we can't find a unique file name.  */
+	pattern[0] = '\0';
+	errno = EINVAL;
+	return -1;
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
-- 
1.7.0.rc2.4.gc602c4

[PATCH 3/4] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.

[Matthieu Moy]

gitmkstemps emulates the behavior of mkstemps, which is usually used
to create files in a shared directory like /tmp/, hence, it creates
files with permission 0600. But we use it also to create pack files,
which do not need this protection.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 cache.h   |    4 ++++
 path.c    |   16 ++++++++++++++--
 wrapper.c |   10 ++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index d478eff..0319637 100644
--- a/cache.h
+++ b/cache.h
@@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template);
 
 int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
 
+/* set default permissions by passing mode arguments to open(2) */
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
+int git_mkstemp_mode(char *pattern, int mode);
+
 /*
  * NOTE NOTE NOTE!!
  *
diff --git a/path.c b/path.c
index 60f5b78..005b836 100644
--- a/path.c
+++ b/path.c
@@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 #undef TMP_MAX
 #define TMP_MAX 16384
 
-int gitmkstemps(char *pattern, int suffix_len)
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 {
 	static const char letters[] =
 		"abcdefghijklmnopqrstuvwxyz"
@@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len)
 		template[4] = letters[v % num_letters]; v /= num_letters;
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd > 0)
 			return fd;
 		/*
@@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len)
 	return -1;
 }
 
+int git_mkstemp_mode(char *pattern, int mode)
+{
+	/* mkstemp is just mkstemps with no suffix */
+	return git_mkstemps_mode(pattern, 0, mode);
+}
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	return git_mkstemps_mode(pattern, suffix_len, 0600);
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
@@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p)
 		}
 	}
 }
+
diff --git a/wrapper.c b/wrapper.c
index 0e3e20a..673762f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -204,6 +204,16 @@ int xmkstemp(char *template)
 	return fd;
 }
 
+int xmkstemp_mode(char *template, int mode)
+{
+	int fd;
+
+	fd = git_mkstemp_mode(template, mode);
+	if (fd < 0)
+		die_errno("Unable to create temporary file");
+	return fd;
+}
+
 /*
  * zlib wrappers to make sure we don't silently miss errors
  * at init time.
-- 
1.7.0.rc2.4.gc602c4

[PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

We used to create 0600 files, and then use chmod to set the group and
other permission bits to the umask. This usually has the same effect
as a normal file creation with a umask.

But in the presence of ACLs, the group permission plays the role of
the ACL mask: the "g" bits of newly created files are chosen according
to default ACL mask of the directory, not according to the umask, and
doing a chmod() on these "g" bits affect the ACL's mask instead of
actual group permission.

In other words, creating files with 0600 and then doing a chmod to the
umask creates files which are unreadable by users allowed in the
default ACL. To create the files without breaking ACLs, we let the
umask do it's job at the file's creation time, and get rid of the
later chmod.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin-pack-objects.c |   18 ++----------------
 t/t1304-default-acl.sh |    2 +-
 wrapper.c              |   10 +++++++---
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e1d3adf..539e75d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -464,9 +464,6 @@ static int write_one(struct sha1file *f,
 	return 1;
 }
 
-/* forward declaration for write_pack_file */
-static int adjust_perm(const char *path, mode_t mode);
-
 static void write_pack_file(void)
 {
 	uint32_t i = 0, j;
@@ -523,21 +520,17 @@ static void write_pack_file(void)
 		}
 
 		if (!pack_to_stdout) {
-			mode_t mode = umask(0);
 			struct stat st;
 			const char *idx_tmp_name;
 			char tmpname[PATH_MAX];
 
-			umask(mode);
-			mode = 0444 & ~mode;
-
 			idx_tmp_name = write_idx_file(NULL, written_list,
 						      nr_written, sha1);
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
 			free_pack_by_name(tmpname);
-			if (adjust_perm(pack_tmp_name, mode))
+			if (adjust_shared_perm(pack_tmp_name))
 				die_errno("unable to make temporary pack file readable");
 			if (rename(pack_tmp_name, tmpname))
 				die_errno("unable to rename temporary pack file");
@@ -565,7 +558,7 @@ static void write_pack_file(void)
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
 				 base_name, sha1_to_hex(sha1));
-			if (adjust_perm(idx_tmp_name, mode))
+			if (adjust_shared_perm(idx_tmp_name))
 				die_errno("unable to make temporary index file readable");
 			if (rename(idx_tmp_name, tmpname))
 				die_errno("unable to rename temporary index file");
@@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av)
 		loosen_unused_packed_objects(&revs);
 }
 
-static int adjust_perm(const char *path, mode_t mode)
-{
-	if (chmod(path, mode))
-		return -1;
-	return adjust_shared_perm(path);
-}
-
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 4ee44a1..b8e9c34 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -27,7 +27,7 @@ modebits () {
 	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
 }
 
-test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+test_expect_success 'git gc does not break ACLs with restrictive umask' '
 	umask 077 &&
 	git gc &&
 	actual=$(modebits .git/objects/pack/*.pack) &&
diff --git a/wrapper.c b/wrapper.c
index 673762f..b2c784d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush)
 int odb_mkstemp(char *template, size_t limit, const char *pattern)
 {
 	int fd;
-
+	/*
+	 * we let the umask do it's job, don't try to be more
+	 * restrictive.
+	 */
+	int mode = 0666;
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
-	fd = mkstemp(template);
+	fd = git_mkstemp_mode(template, mode);
 	if (0 <= fd)
 		return fd;
 
@@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
 	safe_create_leading_directories(template);
-	return xmkstemp(template);
+	return xmkstemp_mode(template, mode);
 }
 
 int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
-- 
1.7.0.rc2.4.gc602c4

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> By not using mkstemp() from the platform but having our own would allow us
> to not even worry about this issue (this can be seen by the removal of a
> call to umask() in [PATCH 4/4]).

Exactly. BTW, we probably want to do the same for object files. The
problem is less important since object files are created 0444, and
therefore do not break ACLs, but that would at least increase
consistancy a bit.

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> -
> +	/*
> +	 * we let the umask do it's job, don't try to be more
> +	 * restrictive.
> +	 */
> +	int mode = 0666;

Actually, it may be safer to put 0444 here. With my patch, git still
creates read-only pack files, but to be honnest, I don't understand
which piece of code cuts the 'w' bit!

New patch serie comming with additional patches for the first point,
and a corrected [PATCH 4] for the second.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/6] Add a testcase for ACL with restrictive umask.

[Matthieu Moy]

Right now, Git creates unreadable pack files on non-shared
repositories when the user has a umask of 077, even when the default
ACLs for the directory would give read/write access to a specific
user.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/t1304-default-acl.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
 create mode 100755 t/t1304-default-acl.sh

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
new file mode 100755
index 0000000..4ee44a1
--- /dev/null
+++ b/t/t1304-default-acl.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Matthieu Moy
+#
+
+test_description='Test repository with default ACL'
+
+. ./test-lib.sh
+
+# We need an arbitrary other user give permission to using ACLs. root
+# is a good candidate: exists on all unices, and it has permission
+# anyway, so we don't create a security hole running the testsuite.
+
+if ! setfacl -Rm u:root:rwx .; then
+    say "Skipping ACL tests: unable to use setfacl"
+    test_done
+fi
+
+setfacl -Rm d:u:"$LOGNAME":rwx .
+setfacl -Rm d:u:root:rwx .
+
+touch file.txt
+git add file.txt
+git commit -m "init"
+
+modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
+test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+	umask 077 &&
+	git gc &&
+	actual=$(modebits .git/objects/pack/*.pack) &&
+	case "$actual" in
+	-r--r-----*)
+		: happy
+		;;
+	*)
+		false
+		;;
+	esac &&
+	getfacl .git/objects/pack/*.pack > actual &&
+	grep -q "user:root:rwx" actual &&
+	grep -q "user:${LOGNAME}:rwx" actual &&
+	grep -q "mask::r--" actual &&
+	grep -q "group::---" actual
+'
+
+test_done
-- 
1.7.0.rc2.92.gb6a04

[PATCH 2/6] Move gitmkstemps to path.c

[Matthieu Moy]

This function used to be only a compatibility function, but we're
going to extend it and actually use it, so make it part of Git.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Makefile          |    1 -
 compat/mkstemps.c |   70 -----------------------------------------------------
 path.c            |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 71 deletions(-)
 delete mode 100644 compat/mkstemps.c

diff --git a/Makefile b/Makefile
index 7bf2fca..4387d42 100644
--- a/Makefile
+++ b/Makefile
@@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP
 endif
 ifdef NO_MKSTEMPS
 	COMPAT_CFLAGS += -DNO_MKSTEMPS
-	COMPAT_OBJS += compat/mkstemps.o
 endif
 ifdef NO_UNSETENV
 	COMPAT_CFLAGS += -DNO_UNSETENV
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
deleted file mode 100644
index 14179c8..0000000
--- a/compat/mkstemps.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "../git-compat-util.h"
-
-/* Adapted from libiberty's mkstemp.c. */
-
-#undef TMP_MAX
-#define TMP_MAX 16384
-
-int gitmkstemps(char *pattern, int suffix_len)
-{
-	static const char letters[] =
-		"abcdefghijklmnopqrstuvwxyz"
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-		"0123456789";
-	static const int num_letters = 62;
-	uint64_t value;
-	struct timeval tv;
-	char *template;
-	size_t len;
-	int fd, count;
-
-	len = strlen(pattern);
-
-	if (len < 6 + suffix_len) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/*
-	 * Replace pattern's XXXXXX characters with randomness.
-	 * Try TMP_MAX different filenames.
-	 */
-	gettimeofday(&tv, NULL);
-	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
-	template = &pattern[len - 6 - suffix_len];
-	for (count = 0; count < TMP_MAX; ++count) {
-		uint64_t v = value;
-		/* Fill in the random bits. */
-		template[0] = letters[v % num_letters]; v /= num_letters;
-		template[1] = letters[v % num_letters]; v /= num_letters;
-		template[2] = letters[v % num_letters]; v /= num_letters;
-		template[3] = letters[v % num_letters]; v /= num_letters;
-		template[4] = letters[v % num_letters]; v /= num_letters;
-		template[5] = letters[v % num_letters]; v /= num_letters;
-
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
-		if (fd > 0)
-			return fd;
-		/*
-		 * Fatal error (EPERM, ENOSPC etc).
-		 * It doesn't make sense to loop.
-		 */
-		if (errno != EEXIST)
-			break;
-		/*
-		 * This is a random value.  It is only necessary that
-		 * the next TMP_MAX values generated by adding 7777 to
-		 * VALUE are different with (module 2^32).
-		 */
-		value += 7777;
-	}
-	/* We return the null string if we can't find a unique file name.  */
-	pattern[0] = '\0';
-	errno = EINVAL;
-	return -1;
-}
diff --git a/path.c b/path.c
index d1fccbd..60f5b78 100644
--- a/path.c
+++ b/path.c
@@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 	return mkstemps(path, suffix_len);
 }
 
+/* Adapted from libiberty's mkstemp.c. */
+
+#undef TMP_MAX
+#define TMP_MAX 16384
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyz"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789";
+	static const int num_letters = 62;
+	uint64_t value;
+	struct timeval tv;
+	char *template;
+	size_t len;
+	int fd, count;
+
+	len = strlen(pattern);
+
+	if (len < 6 + suffix_len) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/*
+	 * Replace pattern's XXXXXX characters with randomness.
+	 * Try TMP_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	template = &pattern[len - 6 - suffix_len];
+	for (count = 0; count < TMP_MAX; ++count) {
+		uint64_t v = value;
+		/* Fill in the random bits. */
+		template[0] = letters[v % num_letters]; v /= num_letters;
+		template[1] = letters[v % num_letters]; v /= num_letters;
+		template[2] = letters[v % num_letters]; v /= num_letters;
+		template[3] = letters[v % num_letters]; v /= num_letters;
+		template[4] = letters[v % num_letters]; v /= num_letters;
+		template[5] = letters[v % num_letters]; v /= num_letters;
+
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd > 0)
+			return fd;
+		/*
+		 * Fatal error (EPERM, ENOSPC etc).
+		 * It doesn't make sense to loop.
+		 */
+		if (errno != EEXIST)
+			break;
+		/*
+		 * This is a random value.  It is only necessary that
+		 * the next TMP_MAX values generated by adding 7777 to
+		 * VALUE are different with (module 2^32).
+		 */
+		value += 7777;
+	}
+	/* We return the null string if we can't find a unique file name.  */
+	pattern[0] = '\0';
+	errno = EINVAL;
+	return -1;
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
-- 
1.7.0.rc2.92.gb6a04

[PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.

[Matthieu Moy]

gitmkstemps emulates the behavior of mkstemps, which is usually used
to create files in a shared directory like /tmp/, hence, it creates
files with permission 0600. But we use it also to create pack files,
which do not need this protection.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 cache.h   |    4 ++++
 path.c    |   16 ++++++++++++++--
 wrapper.c |   10 ++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index d478eff..0319637 100644
--- a/cache.h
+++ b/cache.h
@@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template);
 
 int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
 
+/* set default permissions by passing mode arguments to open(2) */
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
+int git_mkstemp_mode(char *pattern, int mode);
+
 /*
  * NOTE NOTE NOTE!!
  *
diff --git a/path.c b/path.c
index 60f5b78..005b836 100644
--- a/path.c
+++ b/path.c
@@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 #undef TMP_MAX
 #define TMP_MAX 16384
 
-int gitmkstemps(char *pattern, int suffix_len)
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 {
 	static const char letters[] =
 		"abcdefghijklmnopqrstuvwxyz"
@@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len)
 		template[4] = letters[v % num_letters]; v /= num_letters;
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd > 0)
 			return fd;
 		/*
@@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len)
 	return -1;
 }
 
+int git_mkstemp_mode(char *pattern, int mode)
+{
+	/* mkstemp is just mkstemps with no suffix */
+	return git_mkstemps_mode(pattern, 0, mode);
+}
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	return git_mkstemps_mode(pattern, suffix_len, 0600);
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
@@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p)
 		}
 	}
 }
+
diff --git a/wrapper.c b/wrapper.c
index 0e3e20a..673762f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -204,6 +204,16 @@ int xmkstemp(char *template)
 	return fd;
 }
 
+int xmkstemp_mode(char *template, int mode)
+{
+	int fd;
+
+	fd = git_mkstemp_mode(template, mode);
+	if (fd < 0)
+		die_errno("Unable to create temporary file");
+	return fd;
+}
+
 /*
  * zlib wrappers to make sure we don't silently miss errors
  * at init time.
-- 
1.7.0.rc2.92.gb6a04

Re: [PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> gitmkstemps emulates the behavior of mkstemps, which is usually used
> to create files in a shared directory like /tmp/, hence, it creates
> files with permission 0600. But we use it also to create pack files,
> which do not need this protection.

... and the conclusion is???

	Add git_mkstemps_mode() that allows us to specify the desired
        mode, and make git_mkstemps() a wrapper that always uses 0600
        to call it.  Later we will use git_mkstemps_mode() when creating
        pack files.

Without it, it sounds as if "so we change it not to do 0600 because I only
care about packfile creation, and do not care about future bugs when
people assume that git_mkstemps() behaves just like mkstemps()", but that
is not what you did.

[PATCH 4/6 v2] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

We used to create 0600 files, and then use chmod to set the group and
other permission bits to the umask. This usually has the same effect
as a normal file creation with a umask.

But in the presence of ACLs, the group permission plays the role of
the ACL mask: the "g" bits of newly created files are chosen according
to default ACL mask of the directory, not according to the umask, and
doing a chmod() on these "g" bits affect the ACL's mask instead of
actual group permission.

In other words, creating files with 0600 and then doing a chmod to the
umask creates files which are unreadable by users allowed in the
default ACL. To create the files without breaking ACLs, we let the
umask do it's job at the file's creation time, and get rid of the
later chmod.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Change since v1:

+	/*
+	 * we let the umask do its job, don't try to be more
+	 * restrictive except to remove write permission.
+	 */
+	int mode = 0444;

=> the 0666 became a 0444 to create the file read-only (+typo in comment).

 builtin-pack-objects.c |   18 ++----------------
 t/t1304-default-acl.sh |    2 +-
 wrapper.c              |   10 +++++++---
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e1d3adf..539e75d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -464,9 +464,6 @@ static int write_one(struct sha1file *f,
 	return 1;
 }
 
-/* forward declaration for write_pack_file */
-static int adjust_perm(const char *path, mode_t mode);
-
 static void write_pack_file(void)
 {
 	uint32_t i = 0, j;
@@ -523,21 +520,17 @@ static void write_pack_file(void)
 		}
 
 		if (!pack_to_stdout) {
-			mode_t mode = umask(0);
 			struct stat st;
 			const char *idx_tmp_name;
 			char tmpname[PATH_MAX];
 
-			umask(mode);
-			mode = 0444 & ~mode;
-
 			idx_tmp_name = write_idx_file(NULL, written_list,
 						      nr_written, sha1);
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
 			free_pack_by_name(tmpname);
-			if (adjust_perm(pack_tmp_name, mode))
+			if (adjust_shared_perm(pack_tmp_name))
 				die_errno("unable to make temporary pack file readable");
 			if (rename(pack_tmp_name, tmpname))
 				die_errno("unable to rename temporary pack file");
@@ -565,7 +558,7 @@ static void write_pack_file(void)
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
 				 base_name, sha1_to_hex(sha1));
-			if (adjust_perm(idx_tmp_name, mode))
+			if (adjust_shared_perm(idx_tmp_name))
 				die_errno("unable to make temporary index file readable");
 			if (rename(idx_tmp_name, tmpname))
 				die_errno("unable to rename temporary index file");
@@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av)
 		loosen_unused_packed_objects(&revs);
 }
 
-static int adjust_perm(const char *path, mode_t mode)
-{
-	if (chmod(path, mode))
-		return -1;
-	return adjust_shared_perm(path);
-}
-
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 4ee44a1..b8e9c34 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -27,7 +27,7 @@ modebits () {
 	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
 }
 
-test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+test_expect_success 'git gc does not break ACLs with restrictive umask' '
 	umask 077 &&
 	git gc &&
 	actual=$(modebits .git/objects/pack/*.pack) &&
diff --git a/wrapper.c b/wrapper.c
index 673762f..9c71b21 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush)
 int odb_mkstemp(char *template, size_t limit, const char *pattern)
 {
 	int fd;
-
+	/*
+	 * we let the umask do its job, don't try to be more
+	 * restrictive except to remove write permission.
+	 */
+	int mode = 0444;
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
-	fd = mkstemp(template);
+	fd = git_mkstemp_mode(template, mode);
 	if (0 <= fd)
 		return fd;
 
@@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
 	safe_create_leading_directories(template);
-	return xmkstemp(template);
+	return xmkstemp_mode(template, mode);
 }
 
 int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
-- 
1.7.0.rc2.92.gb6a04

[PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 path.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/path.c b/path.c
index 005b836..2886eb6 100644
--- a/path.c
+++ b/path.c
@@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	}
 	/* We return the null string if we can't find a unique file name.  */
 	pattern[0] = '\0';
-	errno = EINVAL;
+	/* Make sure errno signals an error on failure */
+	if (errno <= 0)
+		errno = EINVAL;
 	return -1;
 }
 
-- 
1.7.0.rc2.92.gb6a04

Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  path.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/path.c b/path.c
> index 005b836..2886eb6 100644
> --- a/path.c
> +++ b/path.c
> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	}
>  	/* We return the null string if we can't find a unique file name.  */
>  	pattern[0] = '\0';
> -	errno = EINVAL;
> +	/* Make sure errno signals an error on failure */
> +	if (errno <= 0)
> +		errno = EINVAL;
>  	return -1;
>  }

Please explain this change a bit better.

A successful call into system library does not have to clear errno (POSIX
even says "no function in ... POSIX ... shall set errno to 0"), and you do
not clear errno to zero anywhere in this function either, so it looks as
if you are reading an undefined errno and basing your action on that
result.

Because TMP_MAX is non-zero, you are always reading from errno left by
open() in the loop, so the above paragraph is a misunderstanding.  But
that needs to be in the log message, no?

I think you are trying to avoid stomping on the errno when we broke out of
the loop early, due to getting an error.  But errno is always valid at
this point in this codepath, and errno.h macros shall expand to integer
constant expressions with type int, distinct positive values.  So I think
you can safely remove the assignment without "if (errno <= 0)".  Returning
EINVAL from a variant of mkstemp when the error is anything but "The last
six characters were not XXXXXX" is wrong.

Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>> ---
>>  path.c |    4 +++-
>>  1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/path.c b/path.c
>> index 005b836..2886eb6 100644
>> --- a/path.c
>> +++ b/path.c
>> @@ -222,7 +222,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>  	}
>>  	/* We return the null string if we can't find a unique file name.  */
>>  	pattern[0] = '\0';
>> -	errno = EINVAL;
>> +	/* Make sure errno signals an error on failure */
>> +	if (errno <= 0)
>> +		errno = EINVAL;
>>  	return -1;
>>  }
>
> Please explain this change a bit better.

I think we agree about the

-	errno = EINVAL;

part. Setting errno to EINVAL unconditionally means discarding the
errno left from open(), so we can't know the reason for failure
anymore with this line.

> Because TMP_MAX is non-zero, you are always reading from errno left by
> open() in the loop, so the above paragraph is a misunderstanding.  But
> that needs to be in the log message, no?

Will add a sentence, yes.

> I think you are trying to avoid stomping on the errno when we broke out of
> the loop early, due to getting an error.  But errno is always valid at
> this point in this codepath, and errno.h macros shall expand to integer
> constant expressions with type int, distinct positive values.  So I think
> you can safely remove the assignment without "if (errno <= 0)".  Returning
> EINVAL from a variant of mkstemp when the error is anything but "The last
> six characters were not XXXXXX" is wrong.

Just removing the line would work with the current code, but this "if
(errno <= 0) errno = EINVAL;" allows enforcing the invariant that
errno > 0 when reaching "return -1" in a simple and reliable way (i.e.
changing the for loop later cannot break this invariant by mistake).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL for any error.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Just removing the line would work with the current code, but this "if
> (errno <= 0) errno = EINVAL;" allows enforcing the invariant that
> errno > 0 when reaching "return -1" in a simple and reliable way (i.e.
> changing the for loop later cannot break this invariant by mistake).

I do not think "errno > 0 when -1 is returned" is a meaningful invariant.
You may return EINVAL which is positive and that may keep that invariant,
but you should not be returning that errno unless the error is "template
not ending with 6-X" to begin with, so there isn't a real gain, no?

[PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files

[Matthieu Moy]

We used to unnecessarily give the read permission to group and others,
regardless of the umask, which isn't serious because the objects are
still protected by their containing directory, but isn't necessari
either.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 sha1_file.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 657825e..3316f28 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	}
 
 out:
-	if (set_shared_perm(filename, (S_IFREG|0444)))
+	if (adjust_shared_perm(filename))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
@@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 	}
 	memcpy(buffer, filename, dirlen);
 	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
-	fd = mkstemp(buffer);
+	fd = git_mkstemp_mode(buffer, 0444);
 	if (fd < 0 && dirlen && errno == ENOENT) {
 		/* Make sure the directory exists */
 		memcpy(buffer, filename, dirlen);
@@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 
 		/* Try again */
 		strcpy(buffer + dirlen - 1, "/tmp_obj_XXXXXX");
-		fd = mkstemp(buffer);
+		fd = git_mkstemp_mode(buffer, 0444);
 	}
 	return fd;
 }
-- 
1.7.0.rc2.92.gb6a04

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
> index 4ee44a1..b8e9c34 100755
> --- a/t/t1304-default-acl.sh
> +++ b/t/t1304-default-acl.sh
> @@ -27,7 +27,7 @@ modebits () {
>  	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
>  }
>  
> -test_expect_failure 'git gc does not break ACLs with restrictive umask' '
> +test_expect_success 'git gc does not break ACLs with restrictive umask' '
>  	umask 077 &&
>  	git gc &&
>  	actual=$(modebits .git/objects/pack/*.pack) &&

This does not seem to pass for me; I get 0444 instead of 0440 as you expect.

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
>> index 4ee44a1..b8e9c34 100755
>> --- a/t/t1304-default-acl.sh
>> +++ b/t/t1304-default-acl.sh
>> @@ -27,7 +27,7 @@ modebits () {
>>  	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
>>  }
>>  
>> -test_expect_failure 'git gc does not break ACLs with restrictive umask' '
>> +test_expect_success 'git gc does not break ACLs with restrictive umask' '
>>  	umask 077 &&
>>  	git gc &&
>>  	actual=$(modebits .git/objects/pack/*.pack) &&
>
> This does not seem to pass for me; I get 0444 instead of 0440 as you expect.

That is strange, it does pass (or it's skipped if I don't have
setfacl) for me.

What system are you using? Do you have ACLs enabled on the filesystem
where you run the tests? What permission do you get for packs with
Git-without-my-patch (I used to get 0400)?

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Junio C Hamano]

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> That is strange, it does pass (or it's skipped if I don't have
> setfacl) for me.
>
> What system are you using? Do you have ACLs enabled on the filesystem
> where you run the tests? What permission do you get for packs with
> Git-without-my-patch (I used to get 0400)?

On my primary box (happens to be Debian 5 but that does not matter) I do
not even have setfacl and the tests are properly skipped.

On another box with FC11 (git.git directory is on an ext4 partition), here
is what "sh -x t1304-default.acl.sh -i -v" gives me when run in 'pu'.


[-- Attachment #2: sh -x t1304-default.acl.sh -i -v 2>&1 --]
[-- Type: text/plain, Size: 10245 bytes --]

+ test_description='Test repository with default ACL'
+ . ./test-lib.sh
++ case "$GIT_TEST_TEE_STARTED, $* " in
++ ORIGINAL_TERM=screen
++ LANG=C
++ LC_ALL=C
++ PAGER=cat
++ TZ=UTC
++ TERM=dumb
++ export LANG LC_ALL PAGER TERM TZ
++ EDITOR=:
++ unset VISUAL
++ unset GIT_EDITOR
++ unset AUTHOR_DATE
++ unset AUTHOR_EMAIL
++ unset AUTHOR_NAME
++ unset COMMIT_AUTHOR_EMAIL
++ unset COMMIT_AUTHOR_NAME
++ unset EMAIL
++ unset GIT_ALTERNATE_OBJECT_DIRECTORIES
++ unset GIT_AUTHOR_DATE
++ GIT_AUTHOR_EMAIL=author@example.com
++ GIT_AUTHOR_NAME='A U Thor'
++ unset GIT_COMMITTER_DATE
++ GIT_COMMITTER_EMAIL=committer@example.com
++ GIT_COMMITTER_NAME='C O Mitter'
++ unset GIT_DIFF_OPTS
++ unset GIT_DIR
++ unset GIT_WORK_TREE
++ unset GIT_EXTERNAL_DIFF
++ unset GIT_INDEX_FILE
++ unset GIT_OBJECT_DIRECTORY
++ unset GIT_CEILING_DIRECTORIES
++ unset SHA1_FILE_DIRECTORIES
++ unset SHA1_FILE_DIRECTORY
++ GIT_MERGE_VERBOSITY=5
++ export GIT_MERGE_VERBOSITY
++ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
++ export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
++ export EDITOR
++ GIT_TEST_CMP='diff -u'
++ unset CDPATH
++ case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
+++ echo
+++ tr '[A-Z]' '[a-z]'
++ _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
++ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
++ '[' xscreen '!=' xdumb ']'
++ TERM=screen
++ export TERM
++ '[' -t 1 ']'
++ test 2 -ne 0
++ case "$1" in
++ immediate=t
++ shift
++ test 1 -ne 0
++ case "$1" in
++ verbose=t
++ shift
++ test 0 -ne 0
++ test -n ''
++ test 'Test repository with default ACL' '!=' ''
++ test '' = t
++ exec
++ test t = t
++ exec
++ test_failure=0
++ test_count=0
++ test_fixed=0
++ test_broken=0
++ test_success=0
++ GIT_EXIT_OK=
++ trap die EXIT
++ satisfied=' '
+++ pwd
++ TEST_DIRECTORY=/scratch/buildfarm/pu/t
++ test -n ''
++ test -n ''
++ git_bin_dir=/scratch/buildfarm/pu/t/../bin-wrappers
++ test -x /scratch/buildfarm/pu/t/../bin-wrappers/git
++ PATH=/scratch/buildfarm/pu/t/../bin-wrappers:/home/junio/g/Fedora-11-i686/git-active/bin:/home/junio/bin/common:/sbin:/usr/sbin:/usr/kerberos/bin:/usr/bin:/bin:/usr/local/sbin
++ GIT_EXEC_PATH=/scratch/buildfarm/pu/t/..
++ test -n ''
+++ pwd
++ GIT_TEMPLATE_DIR=/scratch/buildfarm/pu/t/../templates/blt
++ unset GIT_CONFIG
++ GIT_CONFIG_NOSYSTEM=1
++ GIT_CONFIG_NOGLOBAL=1
++ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
++ . ../GIT-BUILD-OPTIONS
+++ SHELL_PATH=/bin/sh
+++ PERL_PATH=/usr/bin/perl
+++ TAR=tar
+++ NO_CURL=
+++ NO_PERL=
+++ NO_PYTHON=
+++ pwd
+++ pwd
++ GITPERLLIB=/scratch/buildfarm/pu/t/../perl/blib/lib:/scratch/buildfarm/pu/t/../perl/blib/arch/auto/Git
++ export GITPERLLIB
++ test -d ../templates/blt
++ test -z ''
++ test -z ''
+++ pwd
++ GITPYTHONLIB=/scratch/buildfarm/pu/t/../git_remote_helpers/build/lib
++ export GITPYTHONLIB
++ test -d ../git_remote_helpers/build
++ test -x ../test-chmtime
+++ basename t1304-default-acl.sh .sh
++ test='trash directory.t1304-default-acl'
++ test -n ''
++ case "$test" in
++ TRASH_DIRECTORY='/scratch/buildfarm/pu/t/trash directory.t1304-default-acl'
++ test '!' -z ''
++ remove_trash='/scratch/buildfarm/pu/t/trash directory.t1304-default-acl'
++ rm -fr 'trash directory.t1304-default-acl'
++ test_create_repo 'trash directory.t1304-default-acl'
++ test 1 = 1
+++ pwd
++ owd=/scratch/buildfarm/pu/t
++ repo='trash directory.t1304-default-acl'
++ mkdir -p 'trash directory.t1304-default-acl'
++ cd 'trash directory.t1304-default-acl'
++ /scratch/buildfarm/pu/t/../git-init --template=/scratch/buildfarm/pu/t/../templates/blt/
Initialized empty Git repository in /scratch/buildfarm/pu/t/trash directory.t1304-default-acl/.git/
++ mv .git/hooks .git/hooks-disabled
++ cd /scratch/buildfarm/pu/t
++ cd -P 'trash directory.t1304-default-acl'
++ this_test=t1304-default-acl.sh
++ this_test=t1304
++ case $(uname -s) in
+++ uname -s
++ test_set_prereq POSIXPERM
++ satisfied=' POSIXPERM '
++ test_set_prereq BSLASHPSPEC
++ satisfied=' POSIXPERM BSLASHPSPEC '
++ test_set_prereq EXECKEEPSPID
++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID '
++ test -z ''
++ test_set_prereq PERL
++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL '
++ test -z ''
++ test_set_prereq PYTHON
++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON '
++ ln -s x y
++ test -h y
++ test_set_prereq SYMLINKS
++ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON SYMLINKS '
++ rm -f y
+ setfacl -Rm u:root:rwx .
+ setfacl -Rm d:u:junio:rwx .
+ setfacl -Rm d:u:root:rwx .
+ touch file.txt
+ git add file.txt
+ git commit -m init
[master (root-commit) 956f7d1] init
 Author: A U Thor <author@example.com>
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 file.txt
+ test_expect_success 'git gc does not break ACLs with restrictive umask' '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ test 2 = 3
+ prereq=
+ test 2 = 2
+ test_skip 'git gc does not break ACLs with restrictive umask' '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ test_count=1
+ to_skip=
+ test -z ''
+ test -n ''
+ case "$to_skip" in
+ false
+ say 'expecting success: 
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ say_color info 'expecting success: 
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ test -z info
+ shift
+ echo '* expecting success: 
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
* expecting success: 
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual

+ test_run_ '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ eval '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
++ umask 077
++ git gc
+++ modebits .git/objects/pack/pack-9a3c3444a7e8660480c6ea70aa526e8299f7cfd6.pack
+++ ls -l .git/objects/pack/pack-9a3c3444a7e8660480c6ea70aa526e8299f7cfd6.pack
+++ sed -e 's|^\(..........\).*|\1|'
++ actual=-r--r--r--
++ case "$actual" in
++ echo 'expected 440, got -r--r--r--'
expected 440, got -r--r--r--
++ false
+ eval_ret=1
+ return 0
+ '[' 0 = 0 -a 1 = 0 ']'
+ test_failure_ 'git gc does not break ACLs with restrictive umask' '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ test_failure=1
+ say_color error 'FAIL 1: git gc does not break ACLs with restrictive umask'
+ test -z error
+ shift
+ echo '* FAIL 1: git gc does not break ACLs with restrictive umask'
* FAIL 1: git gc does not break ACLs with restrictive umask
+ shift
+ echo '
	umask 077 &&
	git gc &&
	actual=$(modebits .git/objects/pack/*.pack) &&
	case "$actual" in
	-r--r-----*)
		: happy
		;;
	*)
		echo "expected 440, got $actual"; false
		;;
	esac &&
	getfacl .git/objects/pack/*.pack > actual &&
	grep -q "user:root:rwx" actual &&
	grep -q "user:${LOGNAME}:rwx" actual &&
	grep -q "mask::r--" actual &&
	grep -q "group::---" actual
'
+ sed -e 's/^/	/'
	
		umask 077 &&
		git gc &&
		actual=$(modebits .git/objects/pack/*.pack) &&
		case "$actual" in
		-r--r-----*)
			: happy
			;;
		*)
			echo "expected 440, got $actual"; false
			;;
		esac &&
		getfacl .git/objects/pack/*.pack > actual &&
		grep -q "user:root:rwx" actual &&
		grep -q "user:${LOGNAME}:rwx" actual &&
		grep -q "mask::r--" actual &&
		grep -q "group::---" actual
	
+ test t = ''
+ GIT_EXIT_OK=t
+ exit 1
+ die
+ code=1
+ test -n t
+ exit 1

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Junio C Hamano]

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

By the way, here is what the same command gets when run on FreeBSD 8.0


[-- Attachment #2: sh -x t1304-default.acl.sh -i -v 2>&1 --]
[-- Type: text/plain, Size: 5677 bytes --]

+ test_description='Test repository with default ACL'
+ . ./test-lib.sh
+ ORIGINAL_TERM=screen
+ LANG=C
+ LC_ALL=C
+ PAGER=cat
+ TZ=UTC
+ TERM=dumb
+ export LANG LC_ALL PAGER TERM TZ
+ EDITOR=:
+ unset VISUAL
+ unset GIT_EDITOR
+ unset AUTHOR_DATE
+ unset AUTHOR_EMAIL
+ unset AUTHOR_NAME
+ unset COMMIT_AUTHOR_EMAIL
+ unset COMMIT_AUTHOR_NAME
+ unset EMAIL
+ unset GIT_ALTERNATE_OBJECT_DIRECTORIES
+ unset GIT_AUTHOR_DATE
+ GIT_AUTHOR_EMAIL=author@example.com
+ GIT_AUTHOR_NAME='A U Thor'
+ unset GIT_COMMITTER_DATE
+ GIT_COMMITTER_EMAIL=committer@example.com
+ GIT_COMMITTER_NAME='C O Mitter'
+ unset GIT_DIFF_OPTS
+ unset GIT_DIR
+ unset GIT_WORK_TREE
+ unset GIT_EXTERNAL_DIFF
+ unset GIT_INDEX_FILE
+ unset GIT_OBJECT_DIRECTORY
+ unset GIT_CEILING_DIRECTORIES
+ unset SHA1_FILE_DIRECTORIES
+ unset SHA1_FILE_DIRECTORY
+ GIT_MERGE_VERBOSITY=5
+ export GIT_MERGE_VERBOSITY
+ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
+ export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
+ export EDITOR
+ GIT_TEST_CMP='diff -u'
+ unset CDPATH
+ echo
+ tr '[A-Z]' '[a-z]'
+ _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
+ _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
+ [ xscreen != xdumb ]
+ TERM=screen
+ export TERM
+ [ -t 1 ]
+ test 2 -ne 0
+ immediate=t
+ shift
+ test 1 -ne 0
+ verbose=t
+ shift
+ test 0 -ne 0
+ test -n ''
+ test 'Test repository with default ACL' != ''
+ test '' = t
+ exec
+ test t = t
+ exec
+ test_failure=0
+ test_count=0
+ test_fixed=0
+ test_broken=0
+ test_success=0
+ GIT_EXIT_OK=''
+ trap die EXIT
+ satisfied=' '
+ pwd
+ TEST_DIRECTORY=/usr/home/junio/buildfarm/pu/t
+ test -n ''
+ test -n ''
+ git_bin_dir=/usr/home/junio/buildfarm/pu/t/../bin-wrappers
+ test -x /usr/home/junio/buildfarm/pu/t/../bin-wrappers/git
+ PATH=/usr/home/junio/buildfarm/pu/t/../bin-wrappers:/home/junio/g/FreeBSD-8.0-RELEASE-i386/git-active/bin:/home/junio/bin/common:/usr/local/bin:/sbin:/usr/sbin:/usr/bin:/bin
+ GIT_EXEC_PATH=/usr/home/junio/buildfarm/pu/t/..
+ test -n ''
+ pwd
+ GIT_TEMPLATE_DIR=/usr/home/junio/buildfarm/pu/t/../templates/blt
+ unset GIT_CONFIG
+ GIT_CONFIG_NOSYSTEM=1
+ GIT_CONFIG_NOGLOBAL=1
+ export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
+ . ../GIT-BUILD-OPTIONS
+ SHELL_PATH=/bin/sh
+ PERL_PATH=/usr/bin/perl
+ TAR=tar
+ NO_CURL=''
+ NO_PERL=''
+ NO_PYTHON=''
+ pwd
+ pwd
+ GITPERLLIB=/usr/home/junio/buildfarm/pu/t/../perl/blib/lib:/usr/home/junio/buildfarm/pu/t/../perl/blib/arch/auto/Git
+ export GITPERLLIB
+ test -d ../templates/blt
+ test -z ''
+ test -z ''
+ pwd
+ GITPYTHONLIB=/usr/home/junio/buildfarm/pu/t/../git_remote_helpers/build/lib
+ export GITPYTHONLIB
+ test -d ../git_remote_helpers/build
+ test -x ../test-chmtime
+ basename t1304-default-acl.sh .sh
+ test='trash directory.t1304-default-acl'
+ test -n ''
+ TRASH_DIRECTORY='/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl'
+ test ! -z ''
+ remove_trash='/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl'
+ rm -fr 'trash directory.t1304-default-acl'
+ test_create_repo 'trash directory.t1304-default-acl'
+ test 1 = 1
+ pwd
+ owd=/usr/home/junio/buildfarm/pu/t
+ repo='trash directory.t1304-default-acl'
+ mkdir -p 'trash directory.t1304-default-acl'
+ cd 'trash directory.t1304-default-acl'
+ /usr/home/junio/buildfarm/pu/t/../git-init --template=/usr/home/junio/buildfarm/pu/t/../templates/blt/
Initialized empty Git repository in /usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl/.git/
+ mv .git/hooks .git/hooks-disabled
+ cd /usr/home/junio/buildfarm/pu/t
+ cd -P 'trash directory.t1304-default-acl'
+ this_test=t1304-default-acl.sh
+ this_test=t1304
+ uname -s
+ test_set_prereq POSIXPERM
+ satisfied=' POSIXPERM '
+ test_set_prereq BSLASHPSPEC
+ satisfied=' POSIXPERM BSLASHPSPEC '
+ test_set_prereq EXECKEEPSPID
+ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID '
+ test -z ''
+ test_set_prereq PERL
+ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL '
+ test -z ''
+ test_set_prereq PYTHON
+ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON '
+ ln -s x y
+ test -h y
+ test_set_prereq SYMLINKS
+ satisfied=' POSIXPERM BSLASHPSPEC EXECKEEPSPID PERL PYTHON SYMLINKS '
+ rm -f y
+ setfacl -Rm u:root:rwx .
setfacl: illegal option -- R
usage: setfacl [-bdhkn] [-m entries] [-M file] [-x entries] [-X file] [file ...]
+ say 'Skipping ACL tests: unable to use setfacl'
+ say_color info 'Skipping ACL tests: unable to use setfacl'
+ test -z info
+ shift
+ echo '* Skipping ACL tests: unable to use setfacl'
* Skipping ACL tests: unable to use setfacl
+ test_done
+ GIT_EXIT_OK=t
+ test_results_dir=/usr/home/junio/buildfarm/pu/t/test-results
+ mkdir -p /usr/home/junio/buildfarm/pu/t/test-results
+ test_results_path=/usr/home/junio/buildfarm/pu/t/test-results/t1304-default-acl-56364
+ echo 'total 0'
+ echo 'success 0'
+ echo 'fixed 0'
+ echo 'broken 0'
+ echo 'failed 0'
+ echo ''
+ test 0 != 0
+ test 0 != 0
+ msg='0 test(s)'
+ say_color pass 'passed all 0 test(s)'
+ test -z pass
+ shift
+ echo '* passed all 0 test(s)'
* passed all 0 test(s)
+ test -d '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl'
+ dirname '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl'
+ cd /usr/home/junio/buildfarm/pu/t
+ basename '/usr/home/junio/buildfarm/pu/t/trash directory.t1304-default-acl'
+ rm -rf 'trash directory.t1304-default-acl'
+ exit 0
+ die
+ code=0
+ test -n t
+ exit 0

Re: [PATCH 4/4] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> On another box with FC11 (git.git directory is on an ext4 partition), here
> is what "sh -x t1304-default.acl.sh -i -v" gives me when run in
> 'pu'.

Actually, it wasn't such a subtle configuration. You have umask=022 on
your machine, and I had umask=077 on the one I did the test. My test
script was setting umask after . test-lib.sh, it should have done it
before (so that the created repository is created with restrictive
umask right away).

While I was there, I also added a testcase for object files.

> By the way, here is what the same command gets when run on FreeBSD 8.0
> [...]
> + setfacl -Rm u:root:rwx .
> setfacl: illegal option -- R

Thanks. I fixed the script not to use -R. But that's without any
warranty, I don't have a FreeBSD machine for testing ...

New serie comming soon, which should adress all your comments.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/4] Allow Git repositories to be shared using POSIX ACLs

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> In short: if a user wants to share a git repository using POSIX
> filesystem ACLs, it seems everything works except pack file creation.
> The problem is that we create the file with a mode 0600, i.e. no
> permission for group,...

Modulo that it is more like "we let mkstemp() to choose whatever file
mode", this is a good analysis.

I think some versions of glibc used to have mkstemp() that creates 0644 or
0664, meaning that the modes left by various implementations of mkstemp()
are different from each other, and also different from what we want.  With
aef5aed (pack-objects: quickfix for permission modes., 2007-04-22) and
b6b32cc (Fix 'quickfix' on pack-objects., 2007-04-22), were attempts to
work around that issue.

By not using mkstemp() from the platform but having our own would allow us
to not even worry about this issue (this can be seen by the removal of a
call to umask() in [PATCH 4/4]).  I see your patches as a "bite-the-bullet
and do the right thing" solution.

> The test-case is really weak: ideally, we should have a real test with
> several users interacting. But that's hardly scriptable in a portable
> way, so the only testcase I added relies on getfacl and "ls -l" to find
> out if the result is correct. However, I did some manual testing, and
> as far as I can say, Git works very well with ACLs with this patch.

Thanks.

[PATCH 0/6 v3] Allow Git repositories to be shared using POSIX ACLs

[Matthieu Moy]

This version should include fixes for all of Junio's comment:

[PATCH 1/6] Add a testcase for ACL with restrictive umask.
=> This should pass with any umask, and includes a fix for FreeBSD
(but I'm not sure it's sufficient)

[PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.
=> better log message suggested by Junio.

[PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL on exit.
=> I actually got rid of this return EINVAL.

[PATCH 1/6] Add a testcase for ACL with restrictive umask.

[Matthieu Moy]

Right now, Git creates unreadable pack files on non-shared
repositories when the user has a umask of 077, even when the default
ACLs for the directory would give read/write access to a specific
user.

Loose object files are created world-readable, which doesn't break ACLs,
but isn't necessarily desirable.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 t/t1304-default-acl.sh |   67 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)
 create mode 100755 t/t1304-default-acl.sh

diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
new file mode 100755
index 0000000..07dd6af
--- /dev/null
+++ b/t/t1304-default-acl.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Matthieu Moy
+#
+
+test_description='Test repository with default ACL'
+
+# Create the test repo with restrictive umask
+# => this must come before . ./test-lib.sh
+umask 077
+
+. ./test-lib.sh
+
+# We need an arbitrary other user give permission to using ACLs. root
+# is a good candidate: exists on all unices, and it has permission
+# anyway, so we don't create a security hole running the testsuite.
+
+if ! setfacl -m u:root:rwx .; then
+    say "Skipping ACL tests: unable to use setfacl"
+    test_done
+fi
+
+modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
+check_perms_and_acl () {
+	actual=$(modebits "$1") &&
+	case "$actual" in
+	-r--r-----*)
+		: happy
+		;;
+	*)
+		echo "Got permission '$actual', expected '-r--r-----'"
+		false
+		;;
+	esac &&
+	getfacl "$1" > actual &&
+	grep -q "user:root:rwx" actual &&
+	grep -q "user:${LOGNAME}:rwx" actual &&
+	grep -q "mask::r--" actual &&
+	grep -q "group::---" actual || false
+}
+
+dirs_to_set="./ .git/ .git/objects/ .git/objects/pack/"
+
+test_expect_success 'Setup test repo' '
+	setfacl -m u:root:rwx          $dirs_to_set &&
+	setfacl -d -m u:"$LOGNAME":rwx $dirs_to_set &&
+	setfacl -d -m u:root:rwx       $dirs_to_set &&
+
+	touch file.txt &&
+	git add file.txt &&
+	git commit -m "init"
+'
+
+test_expect_failure 'Objects creation does not break ACLs with restrictive umask' '
+	# SHA1 for empty blob
+	check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
+'
+
+test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+	git gc &&
+	check_perms_and_acl .git/objects/pack/*.pack
+'
+
+test_done
-- 
1.7.0.54.gb6a04.dirty

[PATCH 2/6] Move gitmkstemps to path.c

[Matthieu Moy]

This function used to be only a compatibility function, but we're
going to extend it and actually use it, so make it part of Git.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Makefile          |    1 -
 compat/mkstemps.c |   70 -----------------------------------------------------
 path.c            |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 71 deletions(-)
 delete mode 100644 compat/mkstemps.c

diff --git a/Makefile b/Makefile
index 7bf2fca..4387d42 100644
--- a/Makefile
+++ b/Makefile
@@ -1200,7 +1200,6 @@ ifdef NO_MKDTEMP
 endif
 ifdef NO_MKSTEMPS
 	COMPAT_CFLAGS += -DNO_MKSTEMPS
-	COMPAT_OBJS += compat/mkstemps.o
 endif
 ifdef NO_UNSETENV
 	COMPAT_CFLAGS += -DNO_UNSETENV
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
deleted file mode 100644
index 14179c8..0000000
--- a/compat/mkstemps.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "../git-compat-util.h"
-
-/* Adapted from libiberty's mkstemp.c. */
-
-#undef TMP_MAX
-#define TMP_MAX 16384
-
-int gitmkstemps(char *pattern, int suffix_len)
-{
-	static const char letters[] =
-		"abcdefghijklmnopqrstuvwxyz"
-		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-		"0123456789";
-	static const int num_letters = 62;
-	uint64_t value;
-	struct timeval tv;
-	char *template;
-	size_t len;
-	int fd, count;
-
-	len = strlen(pattern);
-
-	if (len < 6 + suffix_len) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
-		errno = EINVAL;
-		return -1;
-	}
-
-	/*
-	 * Replace pattern's XXXXXX characters with randomness.
-	 * Try TMP_MAX different filenames.
-	 */
-	gettimeofday(&tv, NULL);
-	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
-	template = &pattern[len - 6 - suffix_len];
-	for (count = 0; count < TMP_MAX; ++count) {
-		uint64_t v = value;
-		/* Fill in the random bits. */
-		template[0] = letters[v % num_letters]; v /= num_letters;
-		template[1] = letters[v % num_letters]; v /= num_letters;
-		template[2] = letters[v % num_letters]; v /= num_letters;
-		template[3] = letters[v % num_letters]; v /= num_letters;
-		template[4] = letters[v % num_letters]; v /= num_letters;
-		template[5] = letters[v % num_letters]; v /= num_letters;
-
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
-		if (fd > 0)
-			return fd;
-		/*
-		 * Fatal error (EPERM, ENOSPC etc).
-		 * It doesn't make sense to loop.
-		 */
-		if (errno != EEXIST)
-			break;
-		/*
-		 * This is a random value.  It is only necessary that
-		 * the next TMP_MAX values generated by adding 7777 to
-		 * VALUE are different with (module 2^32).
-		 */
-		value += 7777;
-	}
-	/* We return the null string if we can't find a unique file name.  */
-	pattern[0] = '\0';
-	errno = EINVAL;
-	return -1;
-}
diff --git a/path.c b/path.c
index 79aa104..ab2e368 100644
--- a/path.c
+++ b/path.c
@@ -157,6 +157,75 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 	return mkstemps(path, suffix_len);
 }
 
+/* Adapted from libiberty's mkstemp.c. */
+
+#undef TMP_MAX
+#define TMP_MAX 16384
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	static const char letters[] =
+		"abcdefghijklmnopqrstuvwxyz"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+		"0123456789";
+	static const int num_letters = 62;
+	uint64_t value;
+	struct timeval tv;
+	char *template;
+	size_t len;
+	int fd, count;
+
+	len = strlen(pattern);
+
+	if (len < 6 + suffix_len) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
+		errno = EINVAL;
+		return -1;
+	}
+
+	/*
+	 * Replace pattern's XXXXXX characters with randomness.
+	 * Try TMP_MAX different filenames.
+	 */
+	gettimeofday(&tv, NULL);
+	value = ((size_t)(tv.tv_usec << 16)) ^ tv.tv_sec ^ getpid();
+	template = &pattern[len - 6 - suffix_len];
+	for (count = 0; count < TMP_MAX; ++count) {
+		uint64_t v = value;
+		/* Fill in the random bits. */
+		template[0] = letters[v % num_letters]; v /= num_letters;
+		template[1] = letters[v % num_letters]; v /= num_letters;
+		template[2] = letters[v % num_letters]; v /= num_letters;
+		template[3] = letters[v % num_letters]; v /= num_letters;
+		template[4] = letters[v % num_letters]; v /= num_letters;
+		template[5] = letters[v % num_letters]; v /= num_letters;
+
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd > 0)
+			return fd;
+		/*
+		 * Fatal error (EPERM, ENOSPC etc).
+		 * It doesn't make sense to loop.
+		 */
+		if (errno != EEXIST)
+			break;
+		/*
+		 * This is a random value.  It is only necessary that
+		 * the next TMP_MAX values generated by adding 7777 to
+		 * VALUE are different with (module 2^32).
+		 */
+		value += 7777;
+	}
+	/* We return the null string if we can't find a unique file name.  */
+	pattern[0] = '\0';
+	errno = EINVAL;
+	return -1;
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
-- 
1.7.0.54.gb6a04.dirty

[PATCH 3/6] git_mkstemp_mode, xmkstemp_mode: variants of gitmkstemps with mode argument.

[Matthieu Moy]

gitmkstemps emulates the behavior of mkstemps, which is usually used
to create files in a shared directory like /tmp/, hence, it creates
files with permission 0600.

Add git_mkstemps_mode() that allows us to specify the desired mode, and
make git_mkstemps() a wrapper that always uses 0600 to call it. Later we
will use git_mkstemps_mode() when creating pack files.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 cache.h   |    4 ++++
 path.c    |   16 ++++++++++++++--
 wrapper.c |   10 ++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index d478eff..0319637 100644
--- a/cache.h
+++ b/cache.h
@@ -641,6 +641,10 @@ int git_mkstemp(char *path, size_t n, const char *template);
 
 int git_mkstemps(char *path, size_t n, const char *template, int suffix_len);
 
+/* set default permissions by passing mode arguments to open(2) */
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
+int git_mkstemp_mode(char *pattern, int mode);
+
 /*
  * NOTE NOTE NOTE!!
  *
diff --git a/path.c b/path.c
index ab2e368..925039a 100644
--- a/path.c
+++ b/path.c
@@ -162,7 +162,7 @@ int git_mkstemps(char *path, size_t len, const char *template, int suffix_len)
 #undef TMP_MAX
 #define TMP_MAX 16384
 
-int gitmkstemps(char *pattern, int suffix_len)
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 {
 	static const char letters[] =
 		"abcdefghijklmnopqrstuvwxyz"
@@ -204,7 +204,7 @@ int gitmkstemps(char *pattern, int suffix_len)
 		template[4] = letters[v % num_letters]; v /= num_letters;
 		template[5] = letters[v % num_letters]; v /= num_letters;
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
+		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd > 0)
 			return fd;
 		/*
@@ -226,6 +226,17 @@ int gitmkstemps(char *pattern, int suffix_len)
 	return -1;
 }
 
+int git_mkstemp_mode(char *pattern, int mode)
+{
+	/* mkstemp is just mkstemps with no suffix */
+	return git_mkstemps_mode(pattern, 0, mode);
+}
+
+int gitmkstemps(char *pattern, int suffix_len)
+{
+	return git_mkstemps_mode(pattern, suffix_len, 0600);
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;
@@ -718,3 +729,4 @@ int daemon_avoid_alias(const char *p)
 		}
 	}
 }
+
diff --git a/wrapper.c b/wrapper.c
index 0e3e20a..673762f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -204,6 +204,16 @@ int xmkstemp(char *template)
 	return fd;
 }
 
+int xmkstemp_mode(char *template, int mode)
+{
+	int fd;
+
+	fd = git_mkstemp_mode(template, mode);
+	if (fd < 0)
+		die_errno("Unable to create temporary file");
+	return fd;
+}
+
 /*
  * zlib wrappers to make sure we don't silently miss errors
  * at init time.
-- 
1.7.0.54.gb6a04.dirty

[PATCH 4/6] Use git_mkstemp_mode and xmkstemp_mode in odb_mkstemp, not chmod later.

[Matthieu Moy]

We used to create 0600 files, and then use chmod to set the group and
other permission bits to the umask. This usually has the same effect
as a normal file creation with a umask.

But in the presence of ACLs, the group permission plays the role of
the ACL mask: the "g" bits of newly created files are chosen according
to default ACL mask of the directory, not according to the umask, and
doing a chmod() on these "g" bits affect the ACL's mask instead of
actual group permission.

In other words, creating files with 0600 and then doing a chmod to the
umask creates files which are unreadable by users allowed in the
default ACL. To create the files without breaking ACLs, we let the
umask do it's job at the file's creation time, and get rid of the
later chmod.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 builtin-pack-objects.c |   18 ++----------------
 t/t1304-default-acl.sh |    2 +-
 wrapper.c              |   10 +++++++---
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index e1d3adf..539e75d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -464,9 +464,6 @@ static int write_one(struct sha1file *f,
 	return 1;
 }
 
-/* forward declaration for write_pack_file */
-static int adjust_perm(const char *path, mode_t mode);
-
 static void write_pack_file(void)
 {
 	uint32_t i = 0, j;
@@ -523,21 +520,17 @@ static void write_pack_file(void)
 		}
 
 		if (!pack_to_stdout) {
-			mode_t mode = umask(0);
 			struct stat st;
 			const char *idx_tmp_name;
 			char tmpname[PATH_MAX];
 
-			umask(mode);
-			mode = 0444 & ~mode;
-
 			idx_tmp_name = write_idx_file(NULL, written_list,
 						      nr_written, sha1);
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.pack",
 				 base_name, sha1_to_hex(sha1));
 			free_pack_by_name(tmpname);
-			if (adjust_perm(pack_tmp_name, mode))
+			if (adjust_shared_perm(pack_tmp_name))
 				die_errno("unable to make temporary pack file readable");
 			if (rename(pack_tmp_name, tmpname))
 				die_errno("unable to rename temporary pack file");
@@ -565,7 +558,7 @@ static void write_pack_file(void)
 
 			snprintf(tmpname, sizeof(tmpname), "%s-%s.idx",
 				 base_name, sha1_to_hex(sha1));
-			if (adjust_perm(idx_tmp_name, mode))
+			if (adjust_shared_perm(idx_tmp_name))
 				die_errno("unable to make temporary index file readable");
 			if (rename(idx_tmp_name, tmpname))
 				die_errno("unable to rename temporary index file");
@@ -2125,13 +2118,6 @@ static void get_object_list(int ac, const char **av)
 		loosen_unused_packed_objects(&revs);
 }
 
-static int adjust_perm(const char *path, mode_t mode)
-{
-	if (chmod(path, mode))
-		return -1;
-	return adjust_shared_perm(path);
-}
-
 int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 {
 	int use_internal_rev_list = 0;
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 07dd6af..8472dbb 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -59,7 +59,7 @@ test_expect_failure 'Objects creation does not break ACLs with restrictive umask
 	check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
 '
 
-test_expect_failure 'git gc does not break ACLs with restrictive umask' '
+test_expect_success 'git gc does not break ACLs with restrictive umask' '
 	git gc &&
 	check_perms_and_acl .git/objects/pack/*.pack
 '
diff --git a/wrapper.c b/wrapper.c
index 673762f..9c71b21 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -277,10 +277,14 @@ int git_inflate(z_streamp strm, int flush)
 int odb_mkstemp(char *template, size_t limit, const char *pattern)
 {
 	int fd;
-
+	/*
+	 * we let the umask do its job, don't try to be more
+	 * restrictive except to remove write permission.
+	 */
+	int mode = 0444;
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
-	fd = mkstemp(template);
+	fd = git_mkstemp_mode(template, mode);
 	if (0 <= fd)
 		return fd;
 
@@ -289,7 +293,7 @@ int odb_mkstemp(char *template, size_t limit, const char *pattern)
 	snprintf(template, limit, "%s/%s",
 		 get_object_directory(), pattern);
 	safe_create_leading_directories(template);
-	return xmkstemp(template);
+	return xmkstemp_mode(template, mode);
 }
 
 int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
-- 
1.7.0.54.gb6a04.dirty

[PATCH 5/6] git_mkstemps_mode: don't set errno to EINVAL on exit.

[Matthieu Moy]

When reaching the end of git_mkstemps_mode, at least one call to open()
has been done, and errno has been set accordingly. Setting errno is
therefore not necessary, and actually harmfull since callers can't
distinguish e.g. permanent failure from ENOENT, which can just mean that
we need to create the containing directory.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 path.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/path.c b/path.c
index 925039a..6f44921 100644
--- a/path.c
+++ b/path.c
@@ -222,7 +222,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	}
 	/* We return the null string if we can't find a unique file name.  */
 	pattern[0] = '\0';
-	errno = EINVAL;
 	return -1;
 }
 
-- 
1.7.0.54.gb6a04.dirty

[PATCH 6/6] Use git_mkstemp_mode instead of plain mkstemp to create object files

[Matthieu Moy]

We used to unnecessarily give the read permission to group and others,
regardless of the umask, which isn't serious because the objects are
still protected by their containing directory, but isn't necessary
either.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 sha1_file.c            |    6 +++---
 t/t1304-default-acl.sh |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 657825e..3316f28 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2206,7 +2206,7 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	}
 
 out:
-	if (set_shared_perm(filename, (S_IFREG|0444)))
+	if (adjust_shared_perm(filename))
 		return error("unable to set permission to '%s'", filename);
 	return 0;
 }
@@ -2262,7 +2262,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 	}
 	memcpy(buffer, filename, dirlen);
 	strcpy(buffer + dirlen, "tmp_obj_XXXXXX");
-	fd = mkstemp(buffer);
+	fd = git_mkstemp_mode(buffer, 0444);
 	if (fd < 0 && dirlen && errno == ENOENT) {
 		/* Make sure the directory exists */
 		memcpy(buffer, filename, dirlen);
@@ -2272,7 +2272,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
 
 		/* Try again */
 		strcpy(buffer + dirlen - 1, "/tmp_obj_XXXXXX");
-		fd = mkstemp(buffer);
+		fd = git_mkstemp_mode(buffer, 0444);
 	}
 	return fd;
 }
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index 8472dbb..cc30be4 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -54,7 +54,7 @@ test_expect_success 'Setup test repo' '
 	git commit -m "init"
 '
 
-test_expect_failure 'Objects creation does not break ACLs with restrictive umask' '
+test_expect_success 'Objects creation does not break ACLs with restrictive umask' '
 	# SHA1 for empty blob
 	check_perms_and_acl .git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391
 '
-- 
1.7.0.54.gb6a04.dirty