From 3f7bd767ed6df4dbbc36c5ab881c0de668107001 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2016 15:41:08 -0400 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 Reviewed-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 12290d249..087a8fa02 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1496,6 +1496,11 @@ stat_ref: 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? */ @@ -1519,7 +1524,7 @@ stat_ref: */ 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 -- cgit v1.2.1 From e8c42cb9ce6a566aad797cc6c5bc1279d608d819 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 6 Oct 2016 12:48:42 -0400 Subject: files_read_raw_ref: prevent infinite retry loops in general 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 Reviewed-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/files-backend.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 087a8fa02..245556435 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1451,6 +1451,7 @@ int read_raw_ref(const char *refname, unsigned char *sha1, int fd; int ret = -1; int save_errno; + int remaining_retries = 3; *type = 0; strbuf_reset(&sb_path); @@ -1466,8 +1467,14 @@ stat_ref: * <-> 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; -- cgit v1.2.1