Problems with stale .keep files on git server

Problems with stale .keep files on git server

[Johan Herland]

Hi,

I have a problem in a server repo where it seems that some previous
"git push" command by some user has left a stale .keep file in the
repo.git/objects/pack/ directory.

Now, when trying to clone the repo on the server, the clone fails with:

$ git clone --bare /path/to/repo.git myclone.git
Cloning into bare repository myclone.git...
fatal: failed to copy file to 'myclone.git/objects/pack/pack-6195737bf980830662f9a44eced023ca4ebe083a.keep': Permission denied

(This is a local clone across filesystems, which I assume simply
copies the objects/ directory from the source repo)

Looking at the .keep file that it's trying to copy from the source
repo, it is owned by the same user as the corresponding .pack and
.idx files, but while the .pack and .idx files have 0440 permissions,
the .keep file has 0600 permission (which explains the "Permission
denied" error). The .keep file itself contains the following text:

  receive-pack 6932 on gitmain

(where gitmain is the hostname of this server)

The timestamp on the .keep file is over a month old, and there is
currently no process with ID 6932 running on this server.

AFAICS, this indicates that someone pushed this pack over a month ago,
and for some reason it failed/aborted, and left the .keep file lying
around. From browsing the source, I see that the .keep file is created
by receive-pack protect the pack from a concurrent "git gc" while it is
being created. However, I have yet to find under which conditions
receive-pack will die without removing the .keep file.

Some questions:

1. Why does the .keep file have 0600 permissions (preventing a local
   clone by any other user)

2. Under which conditions will receive-pack leave stale .keep files
   in the filesystem? Is this a bug?

3. Do I need to scan for and remove stale .keep files in a cron job
   in order to keep repos healthy and clonable?


Thanks,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: Problems with stale .keep files on git server

[Jeff King]

On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:

> Some questions:
> 
> 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.

> 2. Under which conditions will receive-pack leave stale .keep files
>    in the filesystem? Is this a bug?

I didn't look at the code, but I think we have to accept the possibility
that it may leave stale ones in the case of power failure or accidental
death by signal. So maybe there are places to fix, but we will still
have to deal with stale ones.

> 3. Do I need to scan for and remove stale .keep files in a cron job
>    in order to keep repos healthy and clonable?

If we fix (1), then hopefully it is not as much of an issue. But
probably "git gc" should clean up stale ones after a while.

-Peff

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

[Johan Herland]

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

[RFC/PATCH 2/2] repack: Remove stale .keep files before repacking

[Johan Herland]

When a push is aborted, receive-pack sometimes leave stale .keep files in the
objects/pack/ directory. Fortunately, these files are easily identified by
looking at their contents, which is of the form:

  receive-pack $pid on $host

By recognizing this format we can determine whether the .keep file is stale,
and can be safely deleted: If the $host part matches the current hostname,
and there is currently no process with $pid, we can safely assume that the
.keep file no longer refers to a running receive-pack process, and deleting
it should be perfectly safe.
---

On Thursday 31 March 2011, Jeff King wrote:
> On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:
> > 3. Do I need to scan for and remove stale .keep files in a cron job
> > 
> >    in order to keep repos healthy and clonable?
> 
> If we fix (1), then hopefully it is not as much of an issue. But
> probably "git gc" should clean up stale ones after a while.

This patch tries to automatically remove stale .keep files. However,
it's still work-in-progress, as I don't know how to portably (a) ask
for the current hostname (so that I can compare it to the one in the
.keep file), or (b) test for whether a given PID is running on the
system (to determine whether the receive-pack process that wrote the
.keep file is still alive).

Feedback appreciated.


...Johan


 git-repack.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 624feec..f59e4c4 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -56,6 +56,18 @@ PACKTMP="$PACKDIR/.tmp-$$-pack"
 rm -f "$PACKTMP"-*
 trap 'rm -f "$PACKTMP"-*' 0 1 2 3 15
 
+HOST=`hostname || echo "localhost"` # FIXME: Portability?
+for e in `cd "$PACKDIR" && find . -type f -name '*.keep' | sed -e 's/^\.\///'`
+do
+	cat "$PACKDIR/$e" | if read word pid on host
+	then
+		test "$word" = "receive-pack" -a "$on" = "on" -a "$host" = "$HOST" || continue
+		ps -p "$pid" > /dev/null && continue # FIXME: Portability?
+		rm -f "$PACKDIR/$e"
+		say Removed stale keep file $PACKDIR/$e.
+	fi
+done
+
 # There will be more repacking strategies to come...
 case ",$all_into_one," in
 ,,)
-- 
1.7.4

Re: [RFC/PATCH 2/2] repack: Remove stale .keep files before repacking

[Jeff King]

On Fri, Apr 01, 2011 at 03:34:27AM +0200, Johan Herland wrote:

> On Thursday 31 March 2011, Jeff King wrote:
> > On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:
> > > 3. Do I need to scan for and remove stale .keep files in a cron job
> > > 
> > >    in order to keep repos healthy and clonable?
> > 
> > If we fix (1), then hopefully it is not as much of an issue. But
> > probably "git gc" should clean up stale ones after a while.
> 
> This patch tries to automatically remove stale .keep files. However,
> it's still work-in-progress, as I don't know how to portably (a) ask
> for the current hostname (so that I can compare it to the one in the
> .keep file), or (b) test for whether a given PID is running on the
> system (to determine whether the receive-pack process that wrote the
> .keep file is still alive).
> 
> Feedback appreciated.

Since your 1/2 turns them from an actual problem into just harmless
cruft, there's no real rush to get rid of them. Could we just do
something like "there is no matching pack file, and the mtime is 2 weeks
old"?

If there is a matching pack file, I don't think we want to get rid of
them. People can have .keep files if they want to indicate the pack
should be kept. I do admit it would be weird to write the "receive-pack"
message into them, though.

-Peff

Re: [RFC/PATCH 2/2] repack: Remove stale .keep files before repacking

[Johan Herland]

On Friday 01 April 2011, Jeff King wrote:
> On Fri, Apr 01, 2011 at 03:34:27AM +0200, Johan Herland wrote:
> > On Thursday 31 March 2011, Jeff King wrote:
> > > On Thu, Mar 31, 2011 at 12:46:25PM +0200, Johan Herland wrote:
> > > > 3. Do I need to scan for and remove stale .keep files in a cron job
> > > >    in order to keep repos healthy and clonable?
> > > 
> > > If we fix (1), then hopefully it is not as much of an issue. But
> > > probably "git gc" should clean up stale ones after a while.
> > 
> > This patch tries to automatically remove stale .keep files. However,
> > it's still work-in-progress, as I don't know how to portably (a) ask
> > for the current hostname (so that I can compare it to the one in the
> > .keep file), or (b) test for whether a given PID is running on the
> > system (to determine whether the receive-pack process that wrote the
> > .keep file is still alive).
> > 
> > Feedback appreciated.
> 
> Since your 1/2 turns them from an actual problem into just harmless
> cruft, there's no real rush to get rid of them. Could we just do
> something like "there is no matching pack file, and the mtime is 2 weeks
> old"?

True, except that in the case I encountered (and reported) yesterday, I 
believe there _was_ a matching .pack file...

> If there is a matching pack file, I don't think we want to get rid of
> them.

AFAICS, in this case we do.

> People can have .keep files if they want to indicate the pack
> should be kept. I do admit it would be weird to write the "receive-pack"
> message into them, though.

Yeah, I don't think we have to worry about that, and even if we do, we are 
free to change the "receive-pack ..." string into something far less likely 
to generate a false positive.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

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

While I am not sure if letting a clone proceed while there is a concurrent
push is even a good idea to begin with, I agree that a stale .keep file is
a problem.

I am kind of surprised that we are not using atexit(3) to clean them just
like we do for lockfiles; wouldn't that be a better solution?

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

[Jeff King]

On Fri, Apr 01, 2011 at 02:39:21PM -0700, Junio C Hamano wrote:

> Johan Herland <johan@herland.net> writes:
> 
> > 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.
> 
> While I am not sure if letting a clone proceed while there is a concurrent
> push is even a good idea to begin with, I agree that a stale .keep file is
> a problem.
> 
> I am kind of surprised that we are not using atexit(3) to clean them just
> like we do for lockfiles; wouldn't that be a better solution?

We definitely should do that, but it would also be nice if a power
failure, kill -9, or segfault in receive-pack didn't leave a repo
unusable.

-Peff

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

[Shawn Pearce]

On Fri, Apr 1, 2011 at 17:39, Junio C Hamano <gitster@pobox.com> wrote:
> While I am not sure if letting a clone proceed while there is a concurrent
> push is even a good idea to begin with,

What? Why?

Are you suggesting that Git hosting sites disable readers while there
is a push occurring?

We have tried hard to design Git to be concurrent reader/writer safe,
*except* the actual garbage collection part of `git gc` that deletes
loose objects. There is no reason to prevent concurrent readers while
there is a push in progress.

The only problem is a cpio based clone, which may link the objects
directory before the refs, and miss linking the new pack but wind up
linking the new ref, making the clone corrupt. But that is a bug in
the cpio clone implementation. Using file:// to use the classical pipe
is safe here, because the refs are scanned before the objects are.
IMHO, if you think clone during push is unsafe because of this, we
should fix the cpio clone path to do a `git ls-remote` on the source,
cache the refs in memory, copy the objects, then write out a
packed-refs file containing the refs we snapshotted *before* linking
the objects directory into the new clone.

-- 
Shawn.

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

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> On Fri, Apr 1, 2011 at 17:39, Junio C Hamano <gitster@pobox.com> wrote:
>> While I am not sure if letting a clone proceed while there is a concurrent
>> push is even a good idea to begin with,
>
> What? Why?
>
> Are you suggesting that Git hosting sites disable readers while there
> is a push occurring?

This is an irrelevant comment isn't it?  Hosting sites coming via git
protocol will not suffer from this "in-progress .keep not readable" issue
at all.

I was responding to the motivation stated in the commit log message, the
file-based "cp -r" copy or cpio clone, which are _not_ a safe thing to do.

Because "leftover .keep" alone is a good justification, I was hinting to
drop that other motivation from the description altogether.

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

[Johan Herland]

On Saturday 02 April 2011, Junio C Hamano wrote:
> I was responding to the motivation stated in the commit log message, the
> file-based "cp -r" copy or cpio clone, which are _not_ a safe thing to
> do.

Hmpf. I didn't know that clone --local --no-hardlinks was unsafe. If it's 
not safe, should it still be the default behavior for a cross-filesystem 
clone?

Furthermore, this lack of safety is not at all mentioned in the clone 
documentation...

> Because "leftover .keep" alone is a good justification, I was hinting to
> drop that other motivation from the description altogether.

Whatever works best for you. What about this commit message instead?


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". Sometimes, when 
the push fails or is aborted, the .keep file is left stale in the repo. This 
causes problems for other users of the same repo, since the permissions on 
the .keep file (0600) make it inaccessible even though the rest of the repo 
is accessible (0444 modulo shared_repository setting).

There is 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.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Johan Herland]

On Friday 01 April 2011, Shawn Pearce wrote:
> The only problem is a cpio based clone, which may link the objects
> directory before the refs, and miss linking the new pack but wind up
> linking the new ref, making the clone corrupt. But that is a bug in
> the cpio clone implementation. Using file:// to use the classical pipe
> is safe here, because the refs are scanned before the objects are.
> IMHO, if you think clone during push is unsafe because of this, we
> should fix the cpio clone path to do a `git ls-remote` on the source,
> cache the refs in memory, copy the objects, then write out a
> packed-refs file containing the refs we snapshotted *before* linking
> the objects directory into the new clone.

Looking at clone_local() in builtin/clone.c (which I guess is the
culprit here), wouldn't we fix this simply by swapping the two parts
of the function, so that the refs part is done before the objects
part? Like this:


static const struct ref *clone_local(const char *src_repo,
				     const char *dest_repo)
{
	const struct ref *ret;
	struct strbuf src = STRBUF_INIT;
	struct strbuf dest = STRBUF_INIT;
	struct remote *remote;
	struct transport *transport;

	remote = remote_get(src_repo);
	transport = transport_get(remote, src_repo);
	ret = transport_get_remote_refs(transport);
	transport_disconnect(transport);

	if (option_shared)
		add_to_alternates_file(src_repo);
	else {
		strbuf_addf(&src, "%s/objects", src_repo);
		strbuf_addf(&dest, "%s/objects", dest_repo);
		copy_or_link_directory(&src, &dest);
		strbuf_release(&src);
		strbuf_release(&dest);
	}

	if (0 <= option_verbosity)
		printf("done.\n");
	return ret;
}


Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> Hmpf. I didn't know that clone --local --no-hardlinks was unsafe. If it's 
> not safe, should it still be the default behavior for a cross-filesystem 
> clone?

Unsafe is not quite the right word to use here in the sense that it
wouldn't lead to any repository _corruption_ per-se, but if you ended up
copying such a transient .keep file, the pack will stay forever in your
clone target unless you notice and remove it yourself.

Having said that, I expect that the majority of use of a filesystem level
local clone these days is to clone your own repository, likely on your own
machine, and you have absolute control on both ends (e.g. you wouldn't be
running a repack on the source while running a clone---you would more
likely to see the repack finish and then clone).  So in that sense I would
still think that file level clone being the default on a local machine is
a reasonable default.

> 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". Sometimes, when 
> the push fails or is aborted, the .keep file is left stale in the repo. This 
> causes problems for other users of the same repo, since the permissions on 
> the .keep file (0600) make it inaccessible even though the rest of the repo 
> is accessible (0444 modulo shared_repository setting).

I was also wondering why you initialized with 0444 in your patch and then
even adjusted for shared repository settings.

This is a tangent, but wouldn't it be wrong for index-pack to always leave
the idx and pack files in 0444 with an explicit chmod() in the first
place?  I suspect that we simply forgot to fix it when we introduced
adjust_shared_perm().

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

[Johan Herland]

On Saturday 02 April 2011, Junio C Hamano wrote:
> Johan Herland <johan@herland.net> writes:
> > 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".
> > Sometimes, when the push fails or is aborted, the .keep file is left
> > stale in the repo. This causes problems for other users of the same
> > repo, since the permissions on the .keep file (0600) make it
> > inaccessible even though the rest of the repo is accessible (0444
> > modulo shared_repository setting).
> 
> I was also wondering why you initialized with 0444 in your patch and then
> even adjusted for shared repository settings.

I was simply emulating what is currently done for idx and pack files (see 
below).

> This is a tangent, but wouldn't it be wrong for index-pack to always
> leave the idx and pack files in 0444 with an explicit chmod() in the
> first place?  I suspect that we simply forgot to fix it when we
> introduced adjust_shared_perm().

Yeah, probablby, but AFAICS in the receive-pack case, final_pack_name and 
final_index_name are both NULL (neither are specified on the index-pack 
command line passed from receive-pack), so the explicit chmod(..., 0444) is 
never called. Instead the pack and idx files are both opened from 
odb_mkstemp() (via open_pack_file() and write_idx_file(), respectively), 
which uses mode 0444. We then call move_temp_to_file(), which calls 
adjust_shared_perm().


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net