Re: another packed-refs race

From

Jeff King <peff@peff.net>

Date

May 3, 2013, 21:21 UTC

Message-ID

<20130503212144.GA17698@sigill.intra.peff.net>

In-Reply-To

<20130503083847.GA16542@sigill.intra.peff.net>

On Fri, May 03, 2013 at 04:38:47AM -0400, Jeff King wrote:

> For reference, here's a script that demonstrates the problem during
> enumeration (sometimes for-each-ref fails to realize that
> refs/heads/master exists at all):
> 
>   # run this in one terminal
>   git init repo &&
>   cd repo &&
>   git commit --allow-empty -m foo &&
>   base=`git rev-parse HEAD` &&
>   while true; do
>     # this re-creates the loose ref in .git/refs/heads/master
>     git update-ref refs/heads/master $base &&

The correct script looks like this (it just flips between two objects):

  git init -q repo &&
  cd repo &&
  git commit -q --allow-empty -m one &&
  one=`git rev-parse HEAD` &&
  git commit -q --allow-empty -m two &&
  two=`git rev-parse HEAD` &&
  sha1=$one &&
  while true; do
    # this re-creates the loose ref in .git/refs/heads/master
    if test "$sha1" = "$one"; then
      sha1=$two
    else
      sha1=$one
    fi &&
    git update-ref refs/heads/master $sha1 &&

    # we can remove packed-refs safely, as we know that
    # its only value is now stale. Real git would not do
    # this, but we are simulating the case that "master"
    # simply wasn't included in the last packed-refs file.
    rm -f .git/packed-refs &&

    # and now we repack, which will create an up-to-date
    # packed-refs file, and then delete the loose ref
    git pack-refs --all --prune
  done

And a racy lookup check could look like this:

  cd repo &&
  while true; do
    ref=`git rev-parse --verify master`
    echo "==> $ref"
    test -z "$ref" && break
  done

The fix is actually two-fold:

  1. Re-load the packed-refs file after each loose object lookup
     failure. This is made more palatable by using stat() to avoid
     re-reading the file in the common case that it wasn't updated.

  2. The loose ref reading itself is actually not atomic. We call
     lstat() on the ref to find out whether it exists (and whether it is
     a symlink). If we get ENOENT, we fall back to finding the loose
     ref.  If it does exist and is a regular file, we proceed to open()
     it. But if the ref gets packed and pruned in the interim, our open
     will fail and we just return NULL to say "oops, I guess it doesn't
     exist". We want the same fallback-to-packed behavior we would get
     if the lstat failed.

     We could potentially do the same when we readlink() a symbolic
     link, but I don't think it is necessary. We do not pack symbolic
     refs, so if readlink gets ENOENT, it's OK to say "nope, the ref
     does not exist".

---

diff --git a/refs.c b/refs.c
index de2d8eb..45a7ee6 100644
--- a/refs.c
+++ b/refs.c
@@ -708,6 +708,7 @@ static struct ref_cache {
 	struct ref_cache *next;
 	struct ref_entry *loose;
 	struct ref_entry *packed;
+	struct stat packed_validity;
 	/* The submodule name, or "" for the main repo. */
 	char name[FLEX_ARRAY];
 } *ref_cache;
@@ -717,6 +718,7 @@ static void clear_packed_ref_cache(struct ref_cache *refs)
 	if (refs->packed) {
 		free_ref_entry(refs->packed);
 		refs->packed = NULL;
+		memset(&refs->packed_validity, 0, sizeof(refs->packed_validity));
 	}
 }
 
@@ -876,19 +878,57 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)
 	}
 }
 
+/*
+ * Returns 1 if the cached stat information matches the
+ * current state of the file, and 0 otherwise. This should
+ * probably be refactored to share code with ce_match_stat_basic,
+ * which has platform-specific knobs for which fields to respect.
+ */
+static int check_stat_validity(const struct stat *old, const char *fn)
+{
+	static struct stat null;
+	struct stat cur;
+
+	if (stat(fn, &cur))
+		return errno == ENOENT && !memcmp(old, &null, sizeof(null));
+	return cur.st_ino == old->st_ino &&
+	       cur.st_size == old->st_size &&
+	       cur.st_mtime == old->st_mtime;
+}
+
+/*
+ * Call fstat, but zero out the stat structure if for whatever
+ * reason we can't get an answer.
+ */
+static int safe_fstat(int fd, struct stat *out)
+{
+	int r = fstat(fd, out);
+	if (r)
+		memset(out, 0, sizeof(*out));
+	return r;
+}
+
 static struct ref_dir *get_packed_refs(struct ref_cache *refs)
 {
+	const char *packed_refs_file;
+
+	if (*refs->name)
+		packed_refs_file = git_path_submodule(refs->name, "packed-refs");
+	else
+		packed_refs_file = git_path("packed-refs");
+
+	if (refs->packed &&
+	    !check_stat_validity(&refs->packed_validity, packed_refs_file))
+		clear_packed_ref_cache(refs);
+
 	if (!refs->packed) {
-		const char *packed_refs_file;
 		FILE *f;
 
 		refs->packed = create_dir_entry(refs, "", 0, 0);
-		if (*refs->name)
-			packed_refs_file = git_path_submodule(refs->name, "packed-refs");
-		else
-			packed_refs_file = git_path("packed-refs");
+
 		f = fopen(packed_refs_file, "r");
 		if (f) {
+			safe_fstat(fileno(f), &refs->packed_validity);
 			read_packed_refs(f, get_ref_dir(refs->packed));
 			fclose(f);
 		}
@@ -1108,6 +1148,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		git_snpath(path, sizeof(path), "%s", refname);
 
 		if (lstat(path, &st) < 0) {
+			/*
+			 * this lets us reuse this code path
+			 * for later syscall failures; it should
+			 * almost certainly just get factored out into a
+			 * function though
+			 */
+fallback_to_packed:
 			if (errno != ENOENT)
 				return NULL;
 			/*
@@ -1156,7 +1203,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 */
 		fd = open(path, O_RDONLY);
 		if (fd < 0)
-			return NULL;
+			goto fallback_to_packed;
 		len = read_in_full(fd, buffer, sizeof(buffer)-1);
 		close(fd);
 		if (len < 0)