{"thread":{"id":"44297","subject":"[PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads","startedAt":"2016-10-14T13:20:56Z","lastAt":"2016-10-14T14:44:19Z","messageCount":6,"participants":["Petr Stodulka","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"304129","messageId":"1476451012-9925-2-git-send-email-pstodulk@redhat.com","threadId":"44297","inReplyTo":"1476451012-9925-1-git-send-email-pstodulk@redhat.com","subject":"[PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads","fromName":"Petr Stodulka","fromEmail":"pstodulk@redhat.com","sentAt":"2016-10-14T13:16:51Z","receivedAt":"2016-10-14T13:20:56Z","isPatch":true,"sender":{"key":"pstodulk@redhat.com","avatar":"https://avatars.githubusercontent.com/u/1656374?v=4"},"body":"git ls-tree goes into an infinite loop while serving pretty vanilla requests,\nif the refs/heads/ directory contains a symlink that's broken. Added test\nwhich check if git ends with expected exit code or timeout expires.\n---\n t/t3103-ls-tree-misc.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh\nindex 09dcf04..faf79c4 100755\n--- a/t/t3103-ls-tree-misc.sh\n+++ b/t/t3103-ls-tree-misc.sh\n@@ -21,4 +21,13 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '\n \ttest_must_fail git ls-tree -r HEAD\n '\n \n+test_expect_success 'ls-tree fails due to broken symlink instead of infinite loop' '\n+\tmkdir foo_infinit &&\n+\tcd foo_infinit &&\n+\tgit init testrepo &&\n+\tcd testrepo &&\n+\tmkdir -p .git/refs/remotes &&\n+\tln -s ../remotes/foo .git/refs/heads/bar &&\n+\ttest_expect_code 128 timeout 2 git ls-tree bar\n+'\n test_done\n-- \n2.5.5\n\n"},{"id":"304130","messageId":"1476451012-9925-1-git-send-email-pstodulk@redhat.com","threadId":"44297","inReplyTo":null,"subject":"[PATCH 0/2] infinite loop in \"git ls-tree\" for broken symlink","fromName":"Petr Stodulka","fromEmail":"pstodulk@redhat.com","sentAt":"2016-10-14T13:16:50Z","receivedAt":"2016-10-14T13:20:58Z","isPatch":true,"sender":{"key":"pstodulk@redhat.com","avatar":"https://avatars.githubusercontent.com/u/1656374?v=4"},"body":"Hi,\nI have realized that this wasn't fixed after all when refs.c\nwas \"rewritten\". Issue is caused by broken symlink under refs/heads,\nwhich causes infinite loop for \"git ls-tree\" command. It was replied\nearlier [0] and Michael previously fixed that in better way probably,\nthen my proposed patch 2/2, but it contains more changes and I have not\nenough time to study changes in refs* code, so I propose now just my\nlittle patch, which was previously modified by Michael.\n\nIf you prefer different solution, I am ok with this. It is really\njust quick proposal. Patch 1/2 contains test for this issue. If you\nwill prefer different solution with different exit code, test should\nbe corrected. Basic idea is, that timeout should'n expire with\nexit code 124.\n\n[0] http://marc.info/?l=git&m=142712669103790&w=2\n[1] https://github.com/mhagger/git, branch \"ref-broken-symlinks\"\n\nMichael Haggerty (1):\n resolve_ref_unsafe(): limit the number of \"stat_ref\" retries\n\nPetr Stodulka (1):\n Add test for ls-tree with broken symlink under refs/heads\n\n refs/files-backend.c | 6 ++++--\n refs/refs-internal.h | 6 ++++++\n t/t3103-ls-tree-misc.sh | 9 +++++++++\n 3 files changed, 19 insertions(+), 2 deletions(-)\n\n-- \n2.5.5\n\n"},{"id":"304131","messageId":"1476451012-9925-3-git-send-email-pstodulk@redhat.com","threadId":"44297","inReplyTo":"1476451012-9925-1-git-send-email-pstodulk@redhat.com","subject":"[PATCH 2/2] resolve_ref_unsafe(): limit the number of \"stat_ref\" retries","fromName":"Petr Stodulka","fromEmail":"pstodulk@redhat.com","sentAt":"2016-10-14T13:16:52Z","receivedAt":"2016-10-14T13:20:59Z","isPatch":true,"sender":{"key":"pstodulk@redhat.com","avatar":"https://avatars.githubusercontent.com/u/1656374?v=4"},"body":"From: Michael Haggerty \n\nIf there is a broken symlink where a loose reference file is expected,\nthen the attempt to open() it fails with ENOENT. This error is\nmisinterpreted to mean that the loose reference file itself has\ndisappeared due to a race, causing the lookup to be retried. But in\nthis scenario, the retries all suffer from the same problem, causing\nan infinite loop.\n\nSo put a limit (of 5) on the number of times that the stat_ref step\ncan be retried.\n\nBased-on-patch-by: Petr Stodulka \nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 6 ++++--\n refs/refs-internal.h | 6 ++++++\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex d16feb1..245a0b5 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 retries = 0;\n \n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n@@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tif (S_ISLNK(st.st_mode)) {\n \t\tstrbuf_reset(&sb_contents);\n \t\tif (strbuf_readlink(&sb_contents, path, 0) < 0) {\n-\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\tif ((errno == ENOENT || errno == EINVAL) &&\n+\t\t\t\tretries++ < MAXRETRIES) \n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n \t\t\telse\n@@ -1426,7 +1428,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 && retries++ < MAXRETRIES)\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 708b260..37e6b99 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname);\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define SYMREF_MAXDEPTH 5\n \n+/* \n+ * We allow only MAXRETRIES tries to jump on stat_ref, because of possible\n+ * infinite loop\n+ */\n+#define MAXRETRIES 5\n+\n /* Include broken references in a do_for_each_ref*() iteration: */\n #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n \n-- \n2.5.5\n\n"},{"id":"304134","messageId":"855173d3-12b1-5b6c-4910-f19a8f3395b7@redhat.com","threadId":"44297","inReplyTo":"1476451012-9925-3-git-send-email-pstodulk@redhat.com","subject":"Re: [PATCH 2/2] resolve_ref_unsafe(): limit the number of \"stat_ref\" retries","fromName":"Petr Stodulka","fromEmail":"pstodulk@redhat.com","sentAt":"2016-10-14T13:20:43Z","receivedAt":"2016-10-14T13:21:16Z","isPatch":true,"sender":{"key":"pstodulk@redhat.com","avatar":"https://avatars.githubusercontent.com/u/1656374?v=4"},"body":"FYI,\nI modified the patch slightly.\n\nOn 14.10.2016 15:16, Petr Stodulka wrote:\n> From: Michael Haggerty \n> \n> If there is a broken symlink where a loose reference file is expected,\n> then the attempt to open() it fails with ENOENT. This error is\n> misinterpreted to mean that the loose reference file itself has\n> disappeared due to a race, causing the lookup to be retried. But in\n> this scenario, the retries all suffer from the same problem, causing\n> an infinite loop.\n> \n> So put a limit (of 5) on the number of times that the stat_ref step\n> can be retried.\n> \n> Based-on-patch-by: Petr Stodulka \n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 6 ++++--\n> refs/refs-internal.h | 6 ++++++\n> 2 files changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index d16feb1..245a0b5 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 retries = 0;\n> \n> \t*type = 0;\n> \tstrbuf_reset(&sb_path);\n> @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n> \tif (S_ISLNK(st.st_mode)) {\n> \t\tstrbuf_reset(&sb_contents);\n> \t\tif (strbuf_readlink(&sb_contents, path, 0) < 0) {\n> -\t\t\tif (errno == ENOENT || errno == EINVAL)\n> +\t\t\tif ((errno == ENOENT || errno == EINVAL) &&\n> +\t\t\t\tretries++ < MAXRETRIES) \n> \t\t\t\t/* inconsistent with lstat; retry */\n> \t\t\t\tgoto stat_ref;\n> \t\t\telse\n> @@ -1426,7 +1428,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 && retries++ < MAXRETRIES)\n> \t\t\t/* inconsistent with lstat; retry */\n> \t\t\tgoto stat_ref;\n> \t\telse\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index 708b260..37e6b99 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname);\n> /* We allow \"recursive\" symbolic refs. Only within reason, though */\n> #define SYMREF_MAXDEPTH 5\n> \n> +/* \n> + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible\n> + * infinite loop\n> + */\n> +#define MAXRETRIES 5\n> +\n> /* Include broken references in a do_for_each_ref*() iteration: */\n> #define DO_FOR_EACH_INCLUDE_BROKEN 0x01\n> \n> \n\n"},{"id":"304138","messageId":"20161014134214.z3jzlx36y57cdqhu@sigill.intra.peff.net","threadId":"44297","inReplyTo":"1476451012-9925-1-git-send-email-pstodulk@redhat.com","subject":"Re: [PATCH 0/2] infinite loop in \"git ls-tree\" for broken symlink","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-14T13:42:14Z","receivedAt":"2016-10-14T13:42:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote:\n\n> I have realized that this wasn't fixed after all when refs.c\n> was \"rewritten\". Issue is caused by broken symlink under refs/heads,\n> which causes infinite loop for \"git ls-tree\" command. It was replied\n> earlier [0] and Michael previously fixed that in better way probably,\n> then my proposed patch 2/2, but it contains more changes and I have not\n> enough time to study changes in refs* code, so I propose now just my\n> little patch, which was previously modified by Michael.\n> \n> If you prefer different solution, I am ok with this. It is really\n> just quick proposal. Patch 1/2 contains test for this issue. If you\n> will prefer different solution with different exit code, test should\n> be corrected. Basic idea is, that timeout should'n expire with\n> exit code 124.\n> \n> [0] http://marc.info/?l=git&m=142712669103790&w=2\n> [1] https://github.com/mhagger/git, branch \"ref-broken-symlinks\"\n\nI think I fixed this semi-independently last week; see the thread at:\n\n http://public-inbox.org/git/20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net/\n\nI say semi-independently because I didn't actually know about your\nprevious report, but saw it in the wild myself. But I did talk with\nMichael off-list, and he suggested the belt-and-suspenders retry counter\nin my second patch.\n\nThe fix is at e8c42cb in Junio's tree, and it's currently merged to\n'next'.\n\n-Peff\n"},{"id":"304164","messageId":"9b77d6b5-79cb-e4ab-9fb9-17b66801c999@redhat.com","threadId":"44297","inReplyTo":"20161014134214.z3jzlx36y57cdqhu@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] infinite loop in \"git ls-tree\" for broken symlink","fromName":"Petr Stodulka","fromEmail":"pstodulk@redhat.com","sentAt":"2016-10-14T14:43:58Z","receivedAt":"2016-10-14T14:44:19Z","isPatch":true,"sender":{"key":"pstodulk@redhat.com","avatar":"https://avatars.githubusercontent.com/u/1656374?v=4"},"body":"Thank you for info, I totally missed that. Yes, this fixes the issue\nperfectly.\n\nPetr\n\nOn 14.10.2016 15:42, Jeff King wrote:\n> On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote:\n> \n>> I have realized that this wasn't fixed after all when refs.c\n>> was \"rewritten\". Issue is caused by broken symlink under refs/heads,\n>> which causes infinite loop for \"git ls-tree\" command. It was replied\n>> earlier [0] and Michael previously fixed that in better way probably,\n>> then my proposed patch 2/2, but it contains more changes and I have not\n>> enough time to study changes in refs* code, so I propose now just my\n>> little patch, which was previously modified by Michael.\n>>\n>> If you prefer different solution, I am ok with this. It is really\n>> just quick proposal. Patch 1/2 contains test for this issue. If you\n>> will prefer different solution with different exit code, test should\n>> be corrected. Basic idea is, that timeout should'n expire with\n>> exit code 124.\n>>\n>> [0] http://marc.info/?l=git&m=142712669103790&w=2\n>> [1] https://github.com/mhagger/git, branch \"ref-broken-symlinks\"\n> \n> I think I fixed this semi-independently last week; see the thread at:\n> \n> http://public-inbox.org/git/20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net/\n> \n> I say semi-independently because I didn't actually know about your\n> previous report, but saw it in the wild myself. But I did talk with\n> Michael off-list, and he suggested the belt-and-suspenders retry counter\n> in my second patch.\n> \n> The fix is at e8c42cb in Junio's tree, and it's currently merged to\n> 'next'.\n> \n> -Peff\n> \n\n"}]}