[PATCH 0/2] fix infinite loop in ref resolution

[PATCH 0/2] fix infinite loop in ref resolution

[Jeff King]

This fixes an infinite loop bug dating back to the v1.8.x era.
Triggering it requires creating a broken symbolic link in the .git
directory, so I don't think it's security-interesting. It should apply
cleanly on "maint".

  [1/2]: files_read_raw_ref: avoid infinite loop on broken symlinks
  [2/2]: files_read_raw_ref: prevent infinite retry loops in general

 refs/files-backend.c        | 14 +++++++++++++-
 t/t1503-rev-parse-verify.sh |  5 +++++
 2 files changed, 18 insertions(+), 1 deletion(-)

[PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks

[Jeff King]

Our ref resolution first runs lstat() on any path we try to
look up, because we want to treat symlinks specially (by
resolving them manually and considering them symrefs). But
if the results of `readlink` do _not_ look like a ref, we
fall through to treating it like a normal file, and just
read the contents of the linked path.

Since fcb7c76 (resolve_ref_unsafe(): close race condition
reading loose refs, 2013-06-19), that "normal file" code
path will stat() the file and if we see ENOENT, will jump
back to the lstat(), thinking we've seen inconsistent
results between the two calls. But for a symbolic ref, this
isn't a race: the lstat() found the symlink, and the stat()
is looking at the path it points to. We end up in an
infinite loop calling lstat() and stat().

We can fix this by avoiding the retry-on-inconsistent jump
when we know that we found a symlink. While we're at it,
let's add a comment explaining why the symlink case gets to
this code in the first place; without that, it is not
obvious that the correct solution isn't to avoid the stat()
code path entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c        | 7 ++++++-
 t/t1503-rev-parse-verify.sh | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0709f60..d826557 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 			ret = 0;
 			goto out;
 		}
+		/*
+		 * It doesn't look like a refname; fall through to just
+		 * treating it like a non-symlink, and reading whatever it
+		 * points to.
+		 */
 	}
 
 	/* Is it a directory? */
@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	 */
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		if (errno == ENOENT)
+		if (errno == ENOENT && !S_ISLNK(st.st_mode))
 			/* inconsistent with lstat; retry */
 			goto stat_ref;
 		else
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index ab27d0d..69d5135 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '
 	test_must_fail git rev-parse --verify master@{$Np1}
 '
 
+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
+	ln -s does-not-exist .git/broken &&
+	test_must_fail git rev-parse --verify broken
+'
+
 test_done
-- 
2.10.1.506.g904834d

Re: [PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks

[Johannes Sixt]

Am 06.10.2016 um 18:48 schrieb Jeff King:
> +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> +	ln -s does-not-exist .git/broken &&
> +	test_must_fail git rev-parse --verify broken

Hm, lower-case named refs directly in .git are frowned upon, no? If we 
ever decide to forbid them outright, this ref-parse might still fail, 
but for the wrong reason. Should you not better pick an example below ref/?

-- Hannes

[PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks

[Jeff King]

On Thu, Oct 06, 2016 at 09:31:22PM +0200, Johannes Sixt wrote:

> Am 06.10.2016 um 18:48 schrieb Jeff King:
> > +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
> > +	ln -s does-not-exist .git/broken &&
> > +	test_must_fail git rev-parse --verify broken
> 
> Hm, lower-case named refs directly in .git are frowned upon, no? If we ever
> decide to forbid them outright, this ref-parse might still fail, but for the
> wrong reason. Should you not better pick an example below ref/?

I suppose so. The test case was adapted from a real-world example, but
it fails equally well with .git/refs/heads/broken. The only restriction
is that the symlink _destination_ cannot look like "refs/heads/...".

Here's a replacement 1/2. The second patch remains unchanged.

-- >8 --
Subject: files_read_raw_ref: avoid infinite loop on broken symlinks

Our ref resolution first runs lstat() on any path we try to
look up, because we want to treat symlinks specially (by
resolving them manually and considering them symrefs). But
if the results of `readlink` do _not_ look like a ref, we
fall through to treating it like a normal file, and just
read the contents of the linked path.

Since fcb7c76 (resolve_ref_unsafe(): close race condition
reading loose refs, 2013-06-19), that "normal file" code
path will stat() the file and if we see ENOENT, will jump
back to the lstat(), thinking we've seen inconsistent
results between the two calls. But for a symbolic ref, this
isn't a race: the lstat() found the symlink, and the stat()
is looking at the path it points to. We end up in an
infinite loop calling lstat() and stat().

We can fix this by avoiding the retry-on-inconsistent jump
when we know that we found a symlink. While we're at it,
let's add a comment explaining why the symlink case gets to
this code in the first place; without that, it is not
obvious that the correct solution isn't to avoid the stat()
code path entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c        | 7 ++++++-
 t/t1503-rev-parse-verify.sh | 5 +++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0709f60..d826557 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 			ret = 0;
 			goto out;
 		}
+		/*
+		 * It doesn't look like a refname; fall through to just
+		 * treating it like a non-symlink, and reading whatever it
+		 * points to.
+		 */
 	}
 
 	/* Is it a directory? */
@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	 */
 	fd = open(path, O_RDONLY);
 	if (fd < 0) {
-		if (errno == ENOENT)
+		if (errno == ENOENT && !S_ISLNK(st.st_mode))
 			/* inconsistent with lstat; retry */
 			goto stat_ref;
 		else
diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh
index ab27d0d..492edff 100755
--- a/t/t1503-rev-parse-verify.sh
+++ b/t/t1503-rev-parse-verify.sh
@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '
 	test_must_fail git rev-parse --verify master@{$Np1}
 '
 
+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
+	ln -s does-not-exist .git/refs/heads/broken &&
+	test_must_fail git rev-parse --verify broken
+'
+
 test_done
-- 
2.10.1.506.g904834d

Re: [PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks

[Michael Haggerty]

On 10/06/2016 09:41 PM, Jeff King wrote:
> [...]
> Subject: files_read_raw_ref: avoid infinite loop on broken symlinks
> [...]

This patch is

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

[PATCH 2/2] files_read_raw_ref: prevent infinite retry loops in general

[Jeff King]

Limit the number of retries to 3. That should be adequate to
prevent any races, while preventing the possibility of
infinite loops if the logic fails to handle any other
possible error modes correctly.

After the fix in the previous commit, there's no known way
to trigger an infinite loop, but I did manually verify that
this fixes the test in that commit even when the code change
is not applied.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs/files-backend.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index d826557..76e5902 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	int fd;
 	int ret = -1;
 	int save_errno;
+	int remaining_retries = 3;
 
 	*type = 0;
 	strbuf_reset(&sb_path);
@@ -1373,8 +1374,14 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	 * <-> 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().
+	 *
+	 * We'll keep a count of the retries, though, just to avoid
+	 * any confusing situation sending us into an infinite loop.
 	 */
 
+	if (remaining_retries-- <= 0)
+		goto out;
+
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-- 
2.10.1.506.g904834d

Re: [PATCH 2/2] files_read_raw_ref: prevent infinite retry loops in general

[Michael Haggerty]

On 10/06/2016 06:48 PM, Jeff King wrote:
> Limit the number of retries to 3. That should be adequate to
> prevent any races, while preventing the possibility of
> infinite loops if the logic fails to handle any other
> possible error modes correctly.
> 
> After the fix in the previous commit, there's no known way
> to trigger an infinite loop, but I did manually verify that
> this fixes the test in that commit even when the code change
> is not applied.
> [...]

This patch is

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael