* [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink @ 2016-10-14 13:16 Petr Stodulka 2016-10-14 13:16 ` [PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads Petr Stodulka ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Petr Stodulka @ 2016-10-14 13:16 UTC (permalink / raw) To: git; +Cc: pstodulk Hi, I have realized that this wasn't fixed after all when refs.c was "rewritten". Issue is caused by broken symlink under refs/heads, which causes infinite loop for "git ls-tree" command. It was replied earlier [0] and Michael previously fixed that in better way probably, then my proposed patch 2/2, but it contains more changes and I have not enough time to study changes in refs* code, so I propose now just my little patch, which was previously modified by Michael. If you prefer different solution, I am ok with this. It is really just quick proposal. Patch 1/2 contains test for this issue. If you will prefer different solution with different exit code, test should be corrected. Basic idea is, that timeout should'n expire with exit code 124. [0] http://marc.info/?l=git&m=142712669103790&w=2 [1] https://github.com/mhagger/git, branch "ref-broken-symlinks" Michael Haggerty (1): resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka (1): Add test for ls-tree with broken symlink under refs/heads refs/files-backend.c | 6 ++++-- refs/refs-internal.h | 6 ++++++ t/t3103-ls-tree-misc.sh | 9 +++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads 2016-10-14 13:16 [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Petr Stodulka @ 2016-10-14 13:16 ` Petr Stodulka 2016-10-14 13:16 ` [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka 2016-10-14 13:42 ` [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Jeff King 2 siblings, 0 replies; 6+ messages in thread From: Petr Stodulka @ 2016-10-14 13:16 UTC (permalink / raw) To: git; +Cc: pstodulk git ls-tree goes into an infinite loop while serving pretty vanilla requests, if the refs/heads/ directory contains a symlink that's broken. Added test which check if git ends with expected exit code or timeout expires. --- t/t3103-ls-tree-misc.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh index 09dcf04..faf79c4 100755 --- a/t/t3103-ls-tree-misc.sh +++ b/t/t3103-ls-tree-misc.sh @@ -21,4 +21,13 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' ' test_must_fail git ls-tree -r HEAD ' +test_expect_success 'ls-tree fails due to broken symlink instead of infinite loop' ' + mkdir foo_infinit && + cd foo_infinit && + git init testrepo && + cd testrepo && + mkdir -p .git/refs/remotes && + ln -s ../remotes/foo .git/refs/heads/bar && + test_expect_code 128 timeout 2 git ls-tree bar +' test_done -- 2.5.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries 2016-10-14 13:16 [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Petr Stodulka 2016-10-14 13:16 ` [PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads Petr Stodulka @ 2016-10-14 13:16 ` Petr Stodulka 2016-10-14 13:20 ` Petr Stodulka 2016-10-14 13:42 ` [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Jeff King 2 siblings, 1 reply; 6+ messages in thread From: Petr Stodulka @ 2016-10-14 13:16 UTC (permalink / raw) To: git; +Cc: pstodulk, Michael Haggerty From: Michael Haggerty <mhagger@alum.mit.edu> If there is a broken symlink where a loose reference file is expected, then the attempt to open() it fails with ENOENT. This error is misinterpreted to mean that the loose reference file itself has disappeared due to a race, causing the lookup to be retried. But in this scenario, the retries all suffer from the same problem, causing an infinite loop. So put a limit (of 5) on the number of times that the stat_ref step can be retried. Based-on-patch-by: Petr Stodulka <pstodulk@redhat.com> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> --- refs/files-backend.c | 6 ++++-- refs/refs-internal.h | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index d16feb1..245a0b5 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 retries = 0; *type = 0; strbuf_reset(&sb_path); @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, if (S_ISLNK(st.st_mode)) { strbuf_reset(&sb_contents); if (strbuf_readlink(&sb_contents, path, 0) < 0) { - if (errno == ENOENT || errno == EINVAL) + if ((errno == ENOENT || errno == EINVAL) && + retries++ < MAXRETRIES) /* inconsistent with lstat; retry */ goto stat_ref; else @@ -1426,7 +1428,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 && retries++ < MAXRETRIES) /* inconsistent with lstat; retry */ goto stat_ref; else diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 708b260..37e6b99 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname); /* We allow "recursive" symbolic refs. Only within reason, though */ #define SYMREF_MAXDEPTH 5 +/* + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible + * infinite loop + */ +#define MAXRETRIES 5 + /* Include broken references in a do_for_each_ref*() iteration: */ #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 -- 2.5.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries 2016-10-14 13:16 ` [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka @ 2016-10-14 13:20 ` Petr Stodulka 0 siblings, 0 replies; 6+ messages in thread From: Petr Stodulka @ 2016-10-14 13:20 UTC (permalink / raw) To: git; +Cc: Michael Haggerty [-- Attachment #1.1: Type: text/plain, Size: 2571 bytes --] FYI, I modified the patch slightly. On 14.10.2016 15:16, Petr Stodulka wrote: > From: Michael Haggerty <mhagger@alum.mit.edu> > > If there is a broken symlink where a loose reference file is expected, > then the attempt to open() it fails with ENOENT. This error is > misinterpreted to mean that the loose reference file itself has > disappeared due to a race, causing the lookup to be retried. But in > this scenario, the retries all suffer from the same problem, causing > an infinite loop. > > So put a limit (of 5) on the number of times that the stat_ref step > can be retried. > > Based-on-patch-by: Petr Stodulka <pstodulk@redhat.com> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> > --- > refs/files-backend.c | 6 ++++-- > refs/refs-internal.h | 6 ++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index d16feb1..245a0b5 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 retries = 0; > > *type = 0; > strbuf_reset(&sb_path); > @@ -1390,7 +1391,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, > if (S_ISLNK(st.st_mode)) { > strbuf_reset(&sb_contents); > if (strbuf_readlink(&sb_contents, path, 0) < 0) { > - if (errno == ENOENT || errno == EINVAL) > + if ((errno == ENOENT || errno == EINVAL) && > + retries++ < MAXRETRIES) > /* inconsistent with lstat; retry */ > goto stat_ref; > else > @@ -1426,7 +1428,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 && retries++ < MAXRETRIES) > /* inconsistent with lstat; retry */ > goto stat_ref; > else > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index 708b260..37e6b99 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -255,6 +255,12 @@ int rename_ref_available(const char *old_refname, const char *new_refname); > /* We allow "recursive" symbolic refs. Only within reason, though */ > #define SYMREF_MAXDEPTH 5 > > +/* > + * We allow only MAXRETRIES tries to jump on stat_ref, because of possible > + * infinite loop > + */ > +#define MAXRETRIES 5 > + > /* Include broken references in a do_for_each_ref*() iteration: */ > #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 > > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink 2016-10-14 13:16 [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Petr Stodulka 2016-10-14 13:16 ` [PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads Petr Stodulka 2016-10-14 13:16 ` [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka @ 2016-10-14 13:42 ` Jeff King 2016-10-14 14:43 ` Petr Stodulka 2 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2016-10-14 13:42 UTC (permalink / raw) To: Petr Stodulka; +Cc: git On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote: > I have realized that this wasn't fixed after all when refs.c > was "rewritten". Issue is caused by broken symlink under refs/heads, > which causes infinite loop for "git ls-tree" command. It was replied > earlier [0] and Michael previously fixed that in better way probably, > then my proposed patch 2/2, but it contains more changes and I have not > enough time to study changes in refs* code, so I propose now just my > little patch, which was previously modified by Michael. > > If you prefer different solution, I am ok with this. It is really > just quick proposal. Patch 1/2 contains test for this issue. If you > will prefer different solution with different exit code, test should > be corrected. Basic idea is, that timeout should'n expire with > exit code 124. > > [0] http://marc.info/?l=git&m=142712669103790&w=2 > [1] https://github.com/mhagger/git, branch "ref-broken-symlinks" I think I fixed this semi-independently last week; see the thread at: http://public-inbox.org/git/20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net/ I say semi-independently because I didn't actually know about your previous report, but saw it in the wild myself. But I did talk with Michael off-list, and he suggested the belt-and-suspenders retry counter in my second patch. The fix is at e8c42cb in Junio's tree, and it's currently merged to 'next'. -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink 2016-10-14 13:42 ` [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Jeff King @ 2016-10-14 14:43 ` Petr Stodulka 0 siblings, 0 replies; 6+ messages in thread From: Petr Stodulka @ 2016-10-14 14:43 UTC (permalink / raw) To: Jeff King; +Cc: git [-- Attachment #1.1: Type: text/plain, Size: 1645 bytes --] Thank you for info, I totally missed that. Yes, this fixes the issue perfectly. Petr On 14.10.2016 15:42, Jeff King wrote: > On Fri, Oct 14, 2016 at 03:16:50PM +0200, Petr Stodulka wrote: > >> I have realized that this wasn't fixed after all when refs.c >> was "rewritten". Issue is caused by broken symlink under refs/heads, >> which causes infinite loop for "git ls-tree" command. It was replied >> earlier [0] and Michael previously fixed that in better way probably, >> then my proposed patch 2/2, but it contains more changes and I have not >> enough time to study changes in refs* code, so I propose now just my >> little patch, which was previously modified by Michael. >> >> If you prefer different solution, I am ok with this. It is really >> just quick proposal. Patch 1/2 contains test for this issue. If you >> will prefer different solution with different exit code, test should >> be corrected. Basic idea is, that timeout should'n expire with >> exit code 124. >> >> [0] http://marc.info/?l=git&m=142712669103790&w=2 >> [1] https://github.com/mhagger/git, branch "ref-broken-symlinks" > > I think I fixed this semi-independently last week; see the thread at: > > http://public-inbox.org/git/20161006164723.ocg2nbgtulpjcksp@sigill.intra.peff.net/ > > I say semi-independently because I didn't actually know about your > previous report, but saw it in the wild myself. But I did talk with > Michael off-list, and he suggested the belt-and-suspenders retry counter > in my second patch. > > The fix is at e8c42cb in Junio's tree, and it's currently merged to > 'next'. > > -Peff > [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-14 14:44 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-14 13:16 [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Petr Stodulka 2016-10-14 13:16 ` [PATCH 1/2] Add test for ls-tree with broken symlink under refs/heads Petr Stodulka 2016-10-14 13:16 ` [PATCH 2/2] resolve_ref_unsafe(): limit the number of "stat_ref" retries Petr Stodulka 2016-10-14 13:20 ` Petr Stodulka 2016-10-14 13:42 ` [PATCH 0/2] infinite loop in "git ls-tree" for broken symlink Jeff King 2016-10-14 14:43 ` Petr Stodulka
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).