Subject: Re: bug#63931: ls colors one symlink too much as non-broken in symlink chain

From: Pádraig Brady <[email-masked]>
Date: Wed, 7 Jun 2023 16:01:42 +0100

On 07/06/2023 13:23, Pádraig Brady wrote:
> On 07/06/2023 10:56, Martin Schulte wrote:
>> Hello Paul, hello Pádraig,
>>
>> thanks a lot for your analysis and explanations!
>>
>>> With that in mind, the code change you proposed is reasonably innocuous,
>>> although it slows things down a bit in the usual case. Not sure it's
>>> worth doing (I guess it does fix a race but there are other unfixable
>>> races in this area....).
>>
>> I first supposed that the effect was caused by an off-by-one problem, but after understanding it, I think you are right here to ask if changing the behaviour is worth doing.
> 
> Well changing it makes things more consistent,
> and also simplifies ls.c as we can remove the make_link_name() function.
> 
>> Looking at
>>
>> $ ls {38..42}
>> ls: cannot access '41': Too many levels of symbolic links
>> ls: cannot access '42': Too many levels of symbolic links
>> 38  39  40
>>
>> shows that there would be more issues to consider in this case.
> 
> Indeed. That can be addressed with the following.
> 
> cheers,
> Pádraig
> 
> diff --git a/src/ls.c b/src/ls.c
> index fbeb9b6dc..33f692bb4 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -3480,7 +3480,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
>                    break;
> 
>                  need_lstat = (err < 0
> -                            ? errno == ENOENT
> +                            ? (errno == ENOENT || errno == ELOOP)
>                                : ! S_ISDIR (f->stat.st_mode));
>                  if (!need_lstat)
>                    break;

The complete two patches in this area are attached,
which I'll apply a bit later.

Marking this as done,

thanks,
Pádraig
From b98863193b8b194c1183c9ceccc142b441c77884 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 7 Jun 2023 12:32:46 +0100
Subject: [PATCH 1/2] ls: use more standard symlink traversal

* src/ls.c (gobble_file): stat() symlinks directly,
rather than their targets.  This will be more consistent
with how symlinks are externally accessed.
(make_link_name): Remove no longer used function.
Addresses https://bugs.gnu.org/63931
---
 src/ls.c | 43 ++++---------------------------------------
 1 file changed, 4 insertions(+), 39 deletions(-)

diff --git a/src/ls.c b/src/ls.c
index 71d94fd6a..fbeb9b6dc 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -262,7 +262,6 @@ static size_t quote_name_buf (char **inbuf, size_t bufsize, char *name,
                               struct quoting_options const *options,
                               int needs_general_quoting, size_t *width,
                               bool *pad);
-static char *make_link_name (char const *name, char const *linkname);
 static int decode_switches (int argc, char **argv);
 static bool file_ignored (char const *name);
 static uintmax_t gobble_file (char const *name, enum filetype type,
@@ -3575,23 +3574,21 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
           struct stat linkstats;
 
           get_link_name (full_name, f, command_line_arg);
-          char *linkname = make_link_name (full_name, f->linkname);
 
           /* Use the slower quoting path for this entry, though
              don't update CWD_SOME_QUOTED since alignment not affected.  */
-          if (linkname && f->quoted == 0 && needs_quoting (f->linkname))
+          if (f->linkname && f->quoted == 0 && needs_quoting (f->linkname))
             f->quoted = -1;
 
-          /* Avoid following symbolic links when possible, ie, when
+          /* Avoid following symbolic links when possible, i.e., when
              they won't be traced and when no indicator is needed.  */
-          if (linkname
+          if (f->linkname
               && (file_type <= indicator_style || check_symlink_mode)
-              && stat_for_mode (linkname, &linkstats) == 0)
+              && stat_for_mode (full_name, &linkstats) == 0)
             {
               f->linkok = true;
               f->linkmode = linkstats.st_mode;
             }
-          free (linkname);
         }
 
       if (S_ISLNK (f->stat.st_mode))
@@ -3724,38 +3721,6 @@ get_link_name (char const *filename, struct fileinfo *f, bool command_line_arg)
                   filename);
 }
 
-/* If LINKNAME is a relative name and NAME contains one or more
-   leading directories, return LINKNAME with those directories
-   prepended; otherwise, return a copy of LINKNAME.
-   If LINKNAME is NULL, return NULL.  */
-
-static char *
-make_link_name (char const *name, char const *linkname)
-{
-  if (!linkname)
-    return NULL;
-
-  if (IS_ABSOLUTE_FILE_NAME (linkname))
-    return xstrdup (linkname);
-
-  /* The link is to a relative name.  Prepend any leading directory
-     in 'name' to the link name.  */
-  size_t prefix_len = dir_len (name);
-  if (prefix_len == 0)
-    return xstrdup (linkname);
-
-  char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
-
-  /* PREFIX_LEN usually specifies a string not ending in slash.
-     In that case, extend it by one, since the next byte *is* a slash.
-     Otherwise, the prefix is "/", so leave the length unchanged.  */
-  if ( ! ISSLASH (name[prefix_len - 1]))
-    ++prefix_len;
-
-  stpcpy (stpncpy (p, name, prefix_len), linkname);
-  return p;
-}
-
 /* Return true if the last component of NAME is '.' or '..'
    This is so we don't try to recurse on '././././. ...' */
 
-- 
2.40.1

From bd976b92521bbf1a619768bb944f8a1ae15ee439 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 7 Jun 2023 13:42:36 +0100
Subject: [PATCH 2/2] ls: display command line symlinks that return ELOOP

* src/ls.c (gobble_file): Ensure we lstat() a symlink
specified on the command line, if we receive ELOOP from stat().
* tests/ls/symlink-loop.sh: Add a new test.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug fix.
Fixes https://bugs.gnu.org/63931
---
 NEWS                     |  4 ++++
 src/ls.c                 |  2 +-
 tests/local.mk           |  1 +
 tests/ls/symlink-loop.sh | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100755 tests/ls/symlink-loop.sh

diff --git a/NEWS b/NEWS
index 97f180ed1..e47701962 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   Previously such file names would have caused the strip process to fail.
   [This bug was present in "the beginning".]
 
+  ls now shows symlinks specified on the command line that can't be traversed.
+  Previously a "Too many levels of symbolic links" diagnostic was given.
+  [This bug was present in "the beginning".]
+
   'pr --length=1 --double-space' no longer enters an infinite loop.
   [This bug was present in "the beginning".]
 
diff --git a/src/ls.c b/src/ls.c
index fbeb9b6dc..33f692bb4 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3480,7 +3480,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
                 break;
 
               need_lstat = (err < 0
-                            ? errno == ENOENT
+                            ? (errno == ENOENT || errno == ELOOP)
                             : ! S_ISDIR (f->stat.st_mode));
               if (!need_lstat)
                 break;
diff --git a/tests/local.mk b/tests/local.mk
index fb5c9c4a5..23a518a22 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -643,6 +643,7 @@ all_tests =					\
   tests/ls/stat-free-color.sh			\
   tests/ls/stat-free-symlinks.sh		\
   tests/ls/stat-vs-dirent.sh			\
+  tests/ls/symlink-loop.sh			\
   tests/ls/symlink-quote.sh			\
   tests/ls/symlink-slash.sh			\
   tests/ls/time-style-diag.sh			\
diff --git a/tests/ls/symlink-loop.sh b/tests/ls/symlink-loop.sh
new file mode 100755
index 000000000..0e0b0bf47
--- /dev/null
+++ b/tests/ls/symlink-loop.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+# Exercise ls symlink ELOOP handling
+
+# Copyright (C) 2023 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ ls
+
+ln -s loop loop || framework_failure_
+cat loop >/dev/null 2>&1 && skip_ 'symlink loops not detected'
+
+# With coreutils <= 9.3 we would dereference symlinks on the command line
+# and thus fail to display a symlink that could not be traversed.
+ls loop || fail=1
+ls -l loop || fail=1
+ls -l --color=always loop || fail=1
+
+# Ensure these still fail
+returns_ 2 ls -H loop || fail=1
+returns_ 2 ls -L loop || fail=1
+
+Exit $fail
-- 
2.40.1