[PATCH 1/2] index-pack: Create .keep files with same permissions and .pack/.idx

[Johan Herland <johan@herland.net>]

While pushing to a remote repo, Git transiently adds a .keep file for the
pack being pushed, to protect it from a concurrent "git gc". However, the
permissions on this .keep file are such that if a different user attempts
a local cross-filesystem clone ("git clone --no-hardlinks") on the server
while the .keep file is present (either because of a concurrent push, or
because of a prior failed push that left a stale .keep file), the clone
will fail because the second user cannot access the .keep file created by
the first user.

There's no reason why the permission mode of a .keep file should be any
different from the permission mode of the corresponding .pack/.idx files.
Therefore, adjust the permission of .keep files from 0600 to 0444 modulo
the shared_repository setting.

In the above scenario, the .keep file is now accessible to the second user,
and will not prevent the clone.

Signed-off-by: Johan Herland <johan@herland.net>
---

On Thursday 31 March 2011, Jeff King wrote:
> On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:
> > 1. Why does the .keep file have 0600 permissions (preventing a local
> >    clone by any other user)
> 
> The relevant code is in 6e180cd (Make sure objects/pack exists before
> creating a new pack, 2009-02-24). I don't see anything particular about
> the mode, so I suspect it was simply habit to make tempfiles restricted.
> 
> There is nothing secret in the contents, so I don't see any reason to
> loosen it to the same permissions as the packfiles themselves.

This patch attempts to fix the permissions on .keep files.


...Johan


 builtin/index-pack.c |    9 ++++++---
 environment.c        |    4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5a67c81..586c9ac 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -792,10 +792,11 @@ static void final(const char *final_pack_name, const char 
*curr_pack_name,
 	if (keep_msg) {
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
-		if (!keep_name)
+		if (!keep_name) {
 			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
-		else
-			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
+			keep_name = name;
+		} else
+			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0444);
 
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
@@ -811,6 +812,8 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 				    keep_name);
 			report = "keep";
 		}
+		if (adjust_shared_perm(keep_name))
+			error("unable to set permission to '%s'", keep_name);
 	}
 
 	if (final_pack_name != curr_pack_name) {
diff --git a/environment.c b/environment.c
index f4549d3..86bf8f4 100644
--- a/environment.c
+++ b/environment.c
@@ -191,13 +191,13 @@ int odb_pack_keep(char *name, size_t namesz, unsigned char *sha1)
 
 	snprintf(name, namesz, "%s/pack/pack-%s.keep",
 		 get_object_directory(), sha1_to_hex(sha1));
-	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
+	fd = open(name, O_RDWR|O_CREAT|O_EXCL, 0444);
 	if (0 <= fd)
 		return fd;
 
 	/* slow path */
 	safe_create_leading_directories(name);
-	return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
+	return open(name, O_RDWR|O_CREAT|O_EXCL, 0444);
 }
 
 char *get_index_file(void)
-- 
1.7.4