{"thread":{"id":"44236","subject":"[PATCH 0/2] fix infinite loop in ref resolution","startedAt":"2016-10-06T16:47:32Z","lastAt":"2016-10-10T10:39:14Z","messageCount":7,"participants":["Jeff King","Johannes Sixt","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"303498","messageId":"20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net","threadId":"44236","inReplyTo":null,"subject":"[PATCH 0/2] fix infinite loop in ref resolution","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-06T16:47:24Z","receivedAt":"2016-10-06T16:47:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes an infinite loop bug dating back to the v1.8.x era.\nTriggering it requires creating a broken symbolic link in the .git\ndirectory, so I don't think it's security-interesting. It should apply\ncleanly on \"maint\".\n\n  [1/2]: files_read_raw_ref: avoid infinite loop on broken symlinks\n  [2/2]: files_read_raw_ref: prevent infinite retry loops in general\n\n refs/files-backend.c        | 14 +++++++++++++-\n t/t1503-rev-parse-verify.sh |  5 +++++\n 2 files changed, 18 insertions(+), 1 deletion(-)\n"},{"id":"303499","messageId":"20161006164842.yzrdigfimzvgkubv@sigill.intra.peff.net","threadId":"44236","inReplyTo":"20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net","subject":"[PATCH 2/2] files_read_raw_ref: prevent infinite retry loops in general","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-06T16:48:42Z","receivedAt":"2016-10-06T16:49:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Limit the number of retries to 3. That should be adequate to\nprevent any races, while preventing the possibility of\ninfinite loops if the logic fails to handle any other\npossible error modes correctly.\n\nAfter the fix in the previous commit, there's no known way\nto trigger an infinite loop, but I did manually verify that\nthis fixes the test in that commit even when the code change\nis not applied.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d826557..76e5902 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1353,6 +1353,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tint fd;\n \tint ret = -1;\n \tint save_errno;\n+\tint remaining_retries = 3;\n \n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n@@ -1373,8 +1374,14 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t * <-> symlink) between the lstat() and reading, then\n \t * we don't want to report that as an error but rather\n \t * try again starting with the lstat().\n+\t *\n+\t * We'll keep a count of the retries, though, just to avoid\n+\t * any confusing situation sending us into an infinite loop.\n \t */\n \n+\tif (remaining_retries-- <= 0)\n+\t\tgoto out;\n+\n \tif (lstat(path, &st) < 0) {\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-- \n2.10.1.506.g904834d\n"},{"id":"303500","messageId":"20161006164825.otms5ovz2vzanimw@sigill.intra.peff.net","threadId":"44236","inReplyTo":"20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net","subject":"[PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-06T16:48:25Z","receivedAt":"2016-10-06T16:49:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our ref resolution first runs lstat() on any path we try to\nlook up, because we want to treat symlinks specially (by\nresolving them manually and considering them symrefs). But\nif the results of `readlink` do _not_ look like a ref, we\nfall through to treating it like a normal file, and just\nread the contents of the linked path.\n\nSince fcb7c76 (resolve_ref_unsafe(): close race condition\nreading loose refs, 2013-06-19), that \"normal file\" code\npath will stat() the file and if we see ENOENT, will jump\nback to the lstat(), thinking we've seen inconsistent\nresults between the two calls. But for a symbolic ref, this\nisn't a race: the lstat() found the symlink, and the stat()\nis looking at the path it points to. We end up in an\ninfinite loop calling lstat() and stat().\n\nWe can fix this by avoiding the retry-on-inconsistent jump\nwhen we know that we found a symlink. While we're at it,\nlet's add a comment explaining why the symlink case gets to\nthis code in the first place; without that, it is not\nobvious that the correct solution isn't to avoid the stat()\ncode path entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c        | 7 ++++++-\n t/t1503-rev-parse-verify.sh | 5 +++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0709f60..d826557 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t\tret = 0;\n \t\t\tgoto out;\n \t\t}\n+\t\t/*\n+\t\t * It doesn't look like a refname; fall through to just\n+\t\t * treating it like a non-symlink, and reading whatever it\n+\t\t * points to.\n+\t\t */\n \t}\n \n \t/* Is it a directory? */\n@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t */\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT && !S_ISLNK(st.st_mode))\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex ab27d0d..69d5135 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '\n \ttest_must_fail git rev-parse --verify master@{$Np1}\n '\n \n+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '\n+\tln -s does-not-exist .git/broken &&\n+\ttest_must_fail git rev-parse --verify broken\n+'\n+\n test_done\n-- \n2.10.1.506.g904834d\n\n"},{"id":"303523","messageId":"1c39b371-eb41-05d9-3c48-bd41764c9c9a@kdbg.org","threadId":"44236","inReplyTo":"20161006164825.otms5ovz2vzanimw@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-10-06T19:31:22Z","receivedAt":"2016-10-06T19:31:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.10.2016 um 18:48 schrieb Jeff King:\n> +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '\n> +\tln -s does-not-exist .git/broken &&\n> +\ttest_must_fail git rev-parse --verify broken\n\nHm, lower-case named refs directly in .git are frowned upon, no? If we \never decide to forbid them outright, this ref-parse might still fail, \nbut for the wrong reason. Should you not better pick an example below ref/?\n\n-- Hannes\n\n"},{"id":"303527","messageId":"20161006194108.qy7qpg6qpodpeacx@sigill.intra.peff.net","threadId":"44236","inReplyTo":"1c39b371-eb41-05d9-3c48-bd41764c9c9a@kdbg.org","subject":"[PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-06T19:41:08Z","receivedAt":"2016-10-06T19:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 06, 2016 at 09:31:22PM +0200, Johannes Sixt wrote:\n\n> Am 06.10.2016 um 18:48 schrieb Jeff King:\n> > +test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '\n> > +\tln -s does-not-exist .git/broken &&\n> > +\ttest_must_fail git rev-parse --verify broken\n> \n> Hm, lower-case named refs directly in .git are frowned upon, no? If we ever\n> decide to forbid them outright, this ref-parse might still fail, but for the\n> wrong reason. Should you not better pick an example below ref/?\n\nI suppose so. The test case was adapted from a real-world example, but\nit fails equally well with .git/refs/heads/broken. The only restriction\nis that the symlink _destination_ cannot look like \"refs/heads/...\".\n\nHere's a replacement 1/2. The second patch remains unchanged.\n\n-- >8 --\nSubject: files_read_raw_ref: avoid infinite loop on broken symlinks\n\nOur ref resolution first runs lstat() on any path we try to\nlook up, because we want to treat symlinks specially (by\nresolving them manually and considering them symrefs). But\nif the results of `readlink` do _not_ look like a ref, we\nfall through to treating it like a normal file, and just\nread the contents of the linked path.\n\nSince fcb7c76 (resolve_ref_unsafe(): close race condition\nreading loose refs, 2013-06-19), that \"normal file\" code\npath will stat() the file and if we see ENOENT, will jump\nback to the lstat(), thinking we've seen inconsistent\nresults between the two calls. But for a symbolic ref, this\nisn't a race: the lstat() found the symlink, and the stat()\nis looking at the path it points to. We end up in an\ninfinite loop calling lstat() and stat().\n\nWe can fix this by avoiding the retry-on-inconsistent jump\nwhen we know that we found a symlink. While we're at it,\nlet's add a comment explaining why the symlink case gets to\nthis code in the first place; without that, it is not\nobvious that the correct solution isn't to avoid the stat()\ncode path entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs/files-backend.c        | 7 ++++++-\n t/t1503-rev-parse-verify.sh | 5 +++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0709f60..d826557 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1403,6 +1403,11 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t\tret = 0;\n \t\t\tgoto out;\n \t\t}\n+\t\t/*\n+\t\t * It doesn't look like a refname; fall through to just\n+\t\t * treating it like a non-symlink, and reading whatever it\n+\t\t * points to.\n+\t\t */\n \t}\n \n \t/* Is it a directory? */\n@@ -1426,7 +1431,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \t */\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (errno == ENOENT)\n+\t\tif (errno == ENOENT && !S_ISLNK(st.st_mode))\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\ndiff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh\nindex ab27d0d..492edff 100755\n--- a/t/t1503-rev-parse-verify.sh\n+++ b/t/t1503-rev-parse-verify.sh\n@@ -139,4 +139,9 @@ test_expect_success 'master@{n} for various n' '\n \ttest_must_fail git rev-parse --verify master@{$Np1}\n '\n \n+test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '\n+\tln -s does-not-exist .git/refs/heads/broken &&\n+\ttest_must_fail git rev-parse --verify broken\n+'\n+\n test_done\n-- \n2.10.1.506.g904834d\n\n"},{"id":"303702","messageId":"b82fe6e6-8cb5-bcfa-7cd9-0ad065f0fe6f@alum.mit.edu","threadId":"44236","inReplyTo":"20161006194108.qy7qpg6qpodpeacx@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] files_read_raw_ref: avoid infinite loop on broken symlinks","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-10-10T10:32:54Z","receivedAt":"2016-10-10T10:33:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/06/2016 09:41 PM, Jeff King wrote:\n> [...]\n> Subject: files_read_raw_ref: avoid infinite loop on broken symlinks\n> [...]\n\nThis patch is\n\nReviewed-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nMichael\n\n"},{"id":"303703","messageId":"d01fa7f9-827c-fc56-2d4d-69ba87ae851a@alum.mit.edu","threadId":"44236","inReplyTo":"20161006164842.yzrdigfimzvgkubv@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] files_read_raw_ref: prevent infinite retry loops in general","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-10-10T10:34:05Z","receivedAt":"2016-10-10T10:39:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/06/2016 06:48 PM, Jeff King wrote:\n> Limit the number of retries to 3. That should be adequate to\n> prevent any races, while preventing the possibility of\n> infinite loops if the logic fails to handle any other\n> possible error modes correctly.\n> \n> After the fix in the previous commit, there's no known way\n> to trigger an infinite loop, but I did manually verify that\n> this fixes the test in that commit even when the code change\n> is not applied.\n> [...]\n\nThis patch is\n\nReviewed-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nMichael\n\n"}]}