* [BUG] [PATCH] infinite loop due to broken symlink
@ 2015-03-23 16:04 Petr Stodulka
2015-03-25 22:53 ` Michael Haggerty
0 siblings, 1 reply; 4+ messages in thread
From: Petr Stodulka @ 2015-03-23 16:04 UTC (permalink / raw)
To: git
Hi guys,
git goes into an infinite loop due to broken symlink (minimal reproducer
[0]). Affected code is in function
"resolve_ref_unsafe" in file refs.c - notice 'stat_ref'. There is comment
about problem with race condition, hovewer in that case it's regular broken
symlink which cause infinite loop. Possible patch could be something
like this:
-------------------------------------------------------
diff --git a/refs.c b/refs.c
index e23542b..9efe8d2 100644
--- a/refs.c
+++ b/refs.c
@@ -1356,6 +1356,7 @@ static struct ref_dir *get_loose_refs(struct
ref_cache *refs)
/* We allow "recursive" symbolic refs. Only within reason, though */
#define MAXDEPTH 5
#define MAXREFLEN (1024)
+#define MAXLOOP 1024
/*
* Called by resolve_gitlink_ref_recursive() after it failed to read
@@ -1482,6 +1483,7 @@ const char *resolve_ref_unsafe(const char
*refname, int resolve_flags, unsigned
char buffer[256];
static char refname_buffer[256];
int bad_name = 0;
+ int loop_counter = 0;
if (flags)
*flags = 0;
@@ -1546,7 +1548,8 @@ const char *resolve_ref_unsafe(const char
*refname, int resolve_flags, unsigned
if (S_ISLNK(st.st_mode)) {
len = readlink(path, buffer, sizeof(buffer)-1);
if (len < 0) {
- if (errno == ENOENT || errno == EINVAL)
+ if (loop_counter++ < MAXLOOP &&
+ (errno == ENOENT || errno == EINVAL))
/* inconsistent with lstat;
retry */
goto stat_ref;
else
@@ -1579,7 +1582,7 @@ const char *resolve_ref_unsafe(const char
*refname, int resolve_flags, unsigned
*/
fd = open(path, O_RDONLY);
if (fd < 0) {
- if (errno == ENOENT)
+ if (loop_counter++ < MAXLOOP && errno == ENOENT)
/* inconsistent with lstat; retry */
goto stat_ref;
else
-------------------------------------------------------
If I understand well that simple check of broken symlink is not possible
due to race conditions.
Regards,
Petr
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1204193
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [BUG] [PATCH] infinite loop due to broken symlink
2015-03-23 16:04 [BUG] [PATCH] infinite loop due to broken symlink Petr Stodulka
@ 2015-03-25 22:53 ` Michael Haggerty
2015-03-25 23:21 ` Junio C Hamano
2015-03-26 9:32 ` Petr Stodulka
0 siblings, 2 replies; 4+ messages in thread
From: Michael Haggerty @ 2015-03-25 22:53 UTC (permalink / raw)
To: Petr Stodulka, git
On 03/23/2015 05:04 PM, Petr Stodulka wrote:
> git goes into an infinite loop due to broken symlink (minimal reproducer
> [0]). Affected code is in function
> "resolve_ref_unsafe" in file refs.c - notice 'stat_ref'. There is comment
> about problem with race condition, hovewer in that case it's regular broken
> symlink which cause infinite loop.
Thanks for the bug report. I can confirm the problem. In fact, I noticed
the same problem when I was working on a refactoring in the area, but I
still haven't submitted those patches. Luckily, modern Git doesn't use
symlinks in the loose reference hierarchy, so most users will never
encounter this problem.
In fact I think it is only the open() call that can lead to the infinite
loop. Meanwhile, there is another problem caused by falling through the
symlink-handling code, namely that st reflects the lstat() of the
symlink rather than the stat() of the file that it points to.
My approach was to run stat() on the path if the symlink-handling code
falls through. You can see my work in progress in my GitHub repo [1].
> Possible patch could be something
> like this:
>
> -------------------------------------------------------
> diff --git a/refs.c b/refs.c
> index e23542b..9efe8d2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1356,6 +1356,7 @@ static struct ref_dir *get_loose_refs(struct
> ref_cache *refs)
> /* We allow "recursive" symbolic refs. Only within reason, though */
> #define MAXDEPTH 5
> #define MAXREFLEN (1024)
> +#define MAXLOOP 1024
>
> /*
> * Called by resolve_gitlink_ref_recursive() after it failed to read
> @@ -1482,6 +1483,7 @@ const char *resolve_ref_unsafe(const char
> *refname, int resolve_flags, unsigned
> char buffer[256];
> static char refname_buffer[256];
> int bad_name = 0;
> + int loop_counter = 0;
>
> if (flags)
> *flags = 0;
> @@ -1546,7 +1548,8 @@ const char *resolve_ref_unsafe(const char
> *refname, int resolve_flags, unsigned
> if (S_ISLNK(st.st_mode)) {
> len = readlink(path, buffer, sizeof(buffer)-1);
> if (len < 0) {
> - if (errno == ENOENT || errno == EINVAL)
> + if (loop_counter++ < MAXLOOP &&
> + (errno == ENOENT || errno == EINVAL))
> /* inconsistent with lstat;
> retry */
> goto stat_ref;
> else
> @@ -1579,7 +1582,7 @@ const char *resolve_ref_unsafe(const char
> *refname, int resolve_flags, unsigned
> */
> fd = open(path, O_RDONLY);
> if (fd < 0) {
> - if (errno == ENOENT)
> + if (loop_counter++ < MAXLOOP && errno == ENOENT)
> /* inconsistent with lstat; retry */
> goto stat_ref;
> else
> -------------------------------------------------------
>
> If I understand well that simple check of broken symlink is not possible
> due to race conditions.
This change also prevents the infinite loop, in fact in a more failproof
way that doesn't depend on errno values being interpreted correctly. I
would suggest a few stylistic changes, like for example here [2]. On the
other hand, this change doesn't solve the lstat()/stat() problem.
Nevertheless, I wouldn't object to a fix like this being accepted in
addition to the stat() fix when it's ready. It doesn't hurt to wear both
a belt *and* suspenders.
Michael
[1] https://github.com/mhagger/git, branch "wip/refactor-resolve-ref".
See especially commit d2425d8ac8cf73494b318078b92f5b1c510a68fb.
[2] https://github.com/mhagger/git, branch "ref-broken-symlinks"
--
Michael Haggerty
mhagger@alum.mit.edu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BUG] [PATCH] infinite loop due to broken symlink
2015-03-25 22:53 ` Michael Haggerty
@ 2015-03-25 23:21 ` Junio C Hamano
2015-03-26 9:32 ` Petr Stodulka
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2015-03-25 23:21 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Petr Stodulka, git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Thanks for the bug report. I can confirm the problem. In fact, I noticed
> the same problem when I was working on a refactoring in the area, but I
> still haven't submitted those patches.
Thanks. It is nice to know that the refs.c API refactoring is still
being worked on.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BUG] [PATCH] infinite loop due to broken symlink
2015-03-25 22:53 ` Michael Haggerty
2015-03-25 23:21 ` Junio C Hamano
@ 2015-03-26 9:32 ` Petr Stodulka
1 sibling, 0 replies; 4+ messages in thread
From: Petr Stodulka @ 2015-03-26 9:32 UTC (permalink / raw)
To: Michael Haggerty, git
On 25.3.2015 23:53 Michael Haggerty wrote:
> On 03/23/2015 05:04 PM, Petr Stodulka wrote:
>> git goes into an infinite loop due to broken symlink (minimal reproducer
>> [0]). Affected code is in function
>> "resolve_ref_unsafe" in file refs.c - notice 'stat_ref'. There is comment
>> about problem with race condition, hovewer in that case it's regular broken
>> symlink which cause infinite loop.
> Thanks for the bug report. I can confirm the problem. In fact, I noticed
> the same problem when I was working on a refactoring in the area, but I
> still haven't submitted those patches. Luckily, modern Git doesn't use
> symlinks in the loose reference hierarchy, so most users will never
> encounter this problem.
>
> In fact I think it is only the open() call that can lead to the infinite
> loop. Meanwhile, there is another problem caused by falling through the
> symlink-handling code, namely that st reflects the lstat() of the
> symlink rather than the stat() of the file that it points to.
>
> My approach was to run stat() on the path if the symlink-handling code
> falls through. You can see my work in progress in my GitHub repo [1].
>
Yes, I thought up about similar solution, but I wasn't sure about this way
because of race condition (I don't know well code of git yet). In the case
of approved lstat/stat patch, I am more familiar with this solution.
>> Possible patch could be something
>> like this:
>>
>> -------------------------------------------------------
>> diff --git a/refs.c b/refs.c
>> index e23542b..9efe8d2 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1356,6 +1356,7 @@ static struct ref_dir *get_loose_refs(struct
>> ref_cache *refs)
>> /* We allow "recursive" symbolic refs. Only within reason, though */
>> #define MAXDEPTH 5
>> #define MAXREFLEN (1024)
>> +#define MAXLOOP 1024
>>
>> /*
>> * Called by resolve_gitlink_ref_recursive() after it failed to read
>> @@ -1482,6 +1483,7 @@ const char *resolve_ref_unsafe(const char
>> *refname, int resolve_flags, unsigned
>> char buffer[256];
>> static char refname_buffer[256];
>> int bad_name = 0;
>> + int loop_counter = 0;
>>
>> if (flags)
>> *flags = 0;
>> @@ -1546,7 +1548,8 @@ const char *resolve_ref_unsafe(const char
>> *refname, int resolve_flags, unsigned
>> if (S_ISLNK(st.st_mode)) {
>> len = readlink(path, buffer, sizeof(buffer)-1);
>> if (len < 0) {
>> - if (errno == ENOENT || errno == EINVAL)
>> + if (loop_counter++ < MAXLOOP &&
>> + (errno == ENOENT || errno == EINVAL))
>> /* inconsistent with lstat;
>> retry */
>> goto stat_ref;
>> else
>> @@ -1579,7 +1582,7 @@ const char *resolve_ref_unsafe(const char
>> *refname, int resolve_flags, unsigned
>> */
>> fd = open(path, O_RDONLY);
>> if (fd < 0) {
>> - if (errno == ENOENT)
>> + if (loop_counter++ < MAXLOOP && errno == ENOENT)
>> /* inconsistent with lstat; retry */
>> goto stat_ref;
>> else
>> -------------------------------------------------------
>>
>> If I understand well that simple check of broken symlink is not possible
>> due to race conditions.
> This change also prevents the infinite loop, in fact in a more failproof
> way that doesn't depend on errno values being interpreted correctly. I
> would suggest a few stylistic changes, like for example here [2]. On the
> other hand, this change doesn't solve the lstat()/stat() problem.
>
> Nevertheless, I wouldn't object to a fix like this being accepted in
> addition to the stat() fix when it's ready. It doesn't hurt to wear both
> a belt *and* suspenders.
>
> Michael
>
> [1] https://github.com/mhagger/git, branch "wip/refactor-resolve-ref".
> See especially commit d2425d8ac8cf73494b318078b92f5b1c510a68fb.
> [2] https://github.com/mhagger/git, branch "ref-broken-symlinks"
>
When stat/lstat is ready, probably this will not be valuable anymore,
but it
could be reliable 'stop' for some unexpected/unknown possibly ways in
future.
Petr.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-03-26 9:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-23 16:04 [BUG] [PATCH] infinite loop due to broken symlink Petr Stodulka
2015-03-25 22:53 ` Michael Haggerty
2015-03-25 23:21 ` Junio C Hamano
2015-03-26 9:32 ` Petr Stodulka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.