* [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).