infinite loop due to broken symlink

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:

-------------------------------------------------------

refs.c +1 -0

index e23542b..9efe8d2 100644
@@ -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

refs.c +1 -0

@@ -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;

refs.c +2 -1

@@ -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

refs.c +1 -1

@@ -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.

Quoted 5 lines · Petr Stodulka

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].

Quoted 50 lines

Possible patch could be something
like this:

-------------------------------------------------------

refs.c +1 -0

index e23542b..9efe8d2 100644
@@ -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

refs.c +1 -0

@@ -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;

refs.c +2 -1

@@ -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

refs.c +1 -1

@@ -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
-------------------------------------------------------

Quoted 2 lines

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"

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.