diff options
| author | Jeff King <peff@peff.net> | 2016-10-06 15:41:08 -0400 |
|---|---|---|
| committer | Junio C Hamano <gitster@pobox.com> | 2016-10-10 10:53:16 -0700 |
| commit | 3f7bd767ed6df4dbbc36c5ab881c0de668107001 (patch) | |
| tree | ba275a1c1068e5ba91e0be2f60a58ee0198c36c6 | |
| parent | 6406bdc0b95715a087fdeeb0f6adf3deb80a25b8 (diff) | |
| download | git-3f7bd767ed6df4dbbc36c5ab881c0de668107001.tar.gz | |
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>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
| -rw-r--r-- | refs/files-backend.c | 7 | ||||
| -rwxr-xr-x | t/t1503-rev-parse-verify.sh | 5 |
2 files changed, 11 insertions, 1 deletions
diff --git a/refs/files-backend.c b/refs/files-backend.c index 12290d2496..087a8fa024 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 diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh index ab27d0db5c..492edffa9c 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 |