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

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

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

@@ -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) &&

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

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

@@ -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;
-}

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

@@ -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!!
  *

@@ -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)
 		}
 	}
 }
+

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

@@ -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;
 }
 

@@ -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;
 }