Re: [PATCH 1/4] resolve_ref: close race condition for packed refs

From: Jeff King <peff@peff.net>
Date: May 16, 2013, 03:47 UTC
Message-ID: <20130516034721.GD13296@sigill.intra.peff.net>
In-Reply-To: <51901E05.1080905@alum.mit.edu>

On Mon, May 13, 2013 at 12:56:05AM +0200, Michael Haggerty wrote:

Show 5 quoted lines

> > + * This should be called from resolve_ref_unsafe when a loose ref cannot be
> > + * accessed; err must represent the errno from the last attempt to access the
> > + * loose ref, and the other options are forwarded from resolve_safe_unsaefe.
> 
> s/resolve_ref_unsaefe/resolve_ref_unsafe/

Oops, thanks.

Show 7 quoted lines

> > -			return NULL;
> > +			return handle_loose_ref_failure(errno, refname, sha1,
> > +							reading, flag);
> 
> I probably would have separated the rest of the patch, which is a pure
> refactoring, from this last chunk, which is a functional change.  But
> that's just me.

Yeah, I go back and forth on whether it is better to have strict refactors followed by changes or not. Sometimes it is hard to understand the motivation for the refactor without seeing the change, and you end up explaining it twice.

My usual rule of thumb is:

  - If you are factoring out some code, and then are going to change
    that code, make it two separate changes. That keeps the diffs
    readable (the first one is pure movement and you do not need to look
    closely, and the second shows a sane diff of the change).

  - If you are factoring out some code, and then adding more callers,
    it's OK to do it together. The new caller provides the motivation
    for the refactor.

This is the latter case. But I'm open to arguments that the rule is not a good one. :)

Show 5 quoted lines

> I suggest adding a comment here mentioning briefly the race condition
> that the call to handle_loose_ref_failure() is meant to address;
> otherwise somebody not thinking of race conditions might have the clever
> idea of "inlining" the call to "return NULL" because it seems redundant
> with the check of ENOENT following the lstat() call above.

Yeah, I thought I had mentioned that at the top of handle_loose_ref_failure, but I see that I didn't. Probably something like this squashed on top makes sense:

diff --git a/refs.c b/refs.c
index c127baf..1a7e4ef 100644
--- a/refs.c
+++ b/refs.c
@@ -1111,7 +1111,7 @@ static int get_packed_ref(const char *refname, unsigned char *sha1)
 /*
  * This should be called from resolve_ref_unsafe when a loose ref cannot be
  * accessed; err must represent the errno from the last attempt to access the
- * loose ref, and the other options are forwarded from resolve_safe_unsaefe.
+ * loose ref, and the other options are forwarded from resolve_safe_unsafe.
  */
 static const char *handle_loose_ref_failure(int err,
 					    const char *refname,
@@ -1200,9 +1200,16 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 * a ref
 		 */
 		fd = open(path, O_RDONLY);
-		if (fd < 0)
+		if (fd < 0) {
+			/*
+			 * We need to check again here for ENOENT and fall back
+			 * to the packed-refs file to avoid a race condition in
+			 * which the ref is packed and pruned between our
+			 * lstat() and open() calls.
+			 */
 			return handle_loose_ref_failure(errno, refname, sha1,
 							reading, flag);
+		}
 		len = read_in_full(fd, buffer, sizeof(buffer)-1);
 		close(fd);
 		if (len < 0)