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?= <p...@draigbrady.com>
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?= <p...@draigbrady.com>
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
