[PATCH v2 3/3] resolve_ref_unsafe(): close race condition reading loose refs

[Michael Haggerty <mhagger@alum.mit.edu>]

We read loose references in two steps.  The code is roughly:

    lstat()
    if error ENOENT:
        loose ref is missing; look for corresponding packed ref
    else if S_ISLNK:
        readlink()
        if error:
            report failure
    else if S_ISDIR:
        report failure
    else
        open()
        if error:
            report failure
        read()

The problem is that the first filesystem call, to lstat(), is not
atomic with the second filesystem call, to readlink() or open().
Therefore it is possible for another process to change the file
between our two calls, for example:

* If the other process deletes the file, our second call will fail
  with ENOENT, which we *should* interpret as "loose ref is missing;
  look for corresponding packed ref".  This can arise if the other
  process is pack-refs; it might have just written a new packed-refs
  file containing the old contents of the reference then deleted the
  loose ref.

* If the other process changes a symlink into a plain file, our call
  to readlink() will fail with EINVAL, which we *should* respond to by
  trying to open() and read() the file.

The old code treats the reference as missing in both of these cases,
which is incorrect.

So instead, handle errors more selectively: if the result of
readline()/open() is a failure that is inconsistent with the result of
the previous lstat(), then something is fishy.  In this case jump back
and start over again with a fresh call to lstat().

One race is still possible and undetected: another process could
change the file from a regular file into a symlink between the call to
lstat and the call to open().  The open() call would silently follow
the symlink and not know that something is wrong.  This situation
could be detected in two ways:

* On systems that support O_NOFOLLOW, pass that option to the open().

* On other systems, call fstat() on the fd returned by open() and make
  sure that it agrees with the stat info from the original lstat().

However, we don't use symlinks anymore, so this situation is unlikely.
Moreover, it doesn't appear that treating a symlink as a regular file
would have grave consequences; after all, this is exactly how the code
handles non-relative symlinks.  So this commit leaves that race
unaddressed.

Note that this solves only the part of the race within
resolve_ref_unsafe. In the situation described above, we may still be
depending on a cached view of the packed-refs file; that race will be
dealt with in a future patch.

This problem was reported and diagnosed by Jeff King <peff@peff.net>,
and this solution is derived from his patch.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 867cf9f..70b2c82 100644
--- a/refs.c
+++ b/refs.c
@@ -1252,6 +1252,16 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 
 		git_snpath(path, sizeof(path), "%s", refname);
 
+		/*
+		 * We might have to loop back here to avoid a race
+		 * condition: first we lstat() the file, then we try
+		 * to read it as a link or as a file.  But if somebody
+		 * changes the type of the file (file <-> directory
+		 * <-> symlink) between the lstat() and reading, then
+		 * we don't want to report that as an error but rather
+		 * try again starting with the lstat().
+		 */
+	stat_ref:
 		if (lstat(path, &st) < 0) {
 			if (errno == ENOENT)
 				return handle_missing_loose_ref(refname, sha1,
@@ -1263,8 +1273,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		/* Follow "normalized" - ie "refs/.." symlinks by hand */
 		if (S_ISLNK(st.st_mode)) {
 			len = readlink(path, buffer, sizeof(buffer)-1);
-			if (len < 0)
-				return NULL;
+			if (len < 0) {
+				if (errno == ENOENT || errno == EINVAL)
+					/* inconsistent with lstat; retry */
+					goto stat_ref;
+				else
+					return NULL;
+			}
 			buffer[len] = 0;
 			if (!prefixcmp(buffer, "refs/") &&
 					!check_refname_format(buffer, 0)) {
@@ -1287,8 +1302,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea
 		 * a ref
 		 */
 		fd = open(path, O_RDONLY);
-		if (fd < 0)
-			return NULL;
+		if (fd < 0) {
+			if (errno == ENOENT)
+				/* inconsistent with lstat; retry */
+				goto stat_ref;
+			else
+				return NULL;
+		}
 		len = read_in_full(fd, buffer, sizeof(buffer)-1);
 		close(fd);
 		if (len < 0)
-- 
1.8.3.1