GNU bug report logs - #11453
`ls` in coreutils-8.17 incorrectly shows broken symlinks in /

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 11453 in the body.
You can then email your comments to 11453 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Frysinger <vapier <at> gentoo.org>
To: bug-coreutils <at> gnu.org
Subject: `ls` in coreutils-8.17 incorrectly shows broken symlinks in /
Date: Fri, 11 May 2012 14:11:54 -0400

[Message part 1 (text/plain, inline)]

coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug 
before, so looks like a regression with 8.17

easy to show:
$ sudo ln -s dev /foo
$ ls --color=auto /
... foo wrongly shows up in blinky text indicating it's a broken symlink ...
$ ls --color=auto /.
... foo correctly shows up with normal coloring indicating it's a symlink ...
$ cd / ; ls --color=auto
... foo correctly shows up with normal coloring indicating it's a symlink ...
-mike

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 11453 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 11453 <at> debbugs.gnu.org
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Fri, 11 May 2012 20:48:33 +0200

Mike Frysinger wrote:
> coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug
> before, so looks like a regression with 8.17
>
> easy to show:
> $ sudo ln -s dev /foo
> $ ls --color=auto /
> ... foo wrongly shows up in blinky text indicating it's a broken symlink ...
> $ ls --color=auto /.
> ... foo correctly shows up with normal coloring indicating it's a symlink ...
> $ cd / ; ls --color=auto
> ... foo correctly shows up with normal coloring indicating it's a symlink ...
> -mike

Rats.
That bug was introduced by my clean-up.
Here's the probable fix.
Thanks for the report!

diff --git a/src/ls.c b/src/ls.c
index 397e4ea..b548382 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3213,7 +3213,8 @@ make_link_name (char const *name, char const *linkname)
     return xstrdup (linkname);

   char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
-  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
+  bool prefix_ends_in_slash = ISSLASH (name[prefix_len - 1]);
+  stpcpy (stpncpy (p, name, prefix_len + !prefix_ends_in_slash), linkname);
   return p;
 }

Message #11 received at 11453 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 11453 <at> debbugs.gnu.org
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Fri, 11 May 2012 21:02:47 +0200

Jim Meyering wrote:
> Mike Frysinger wrote:
>> coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug
>> before, so looks like a regression with 8.17
>>
>> easy to show:
>> $ sudo ln -s dev /foo
>> $ ls --color=auto /
>> ... foo wrongly shows up in blinky text indicating it's a broken symlink ...
>> $ ls --color=auto /.
>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>> $ cd / ; ls --color=auto
>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>> -mike
>
> Rats.
> That bug was introduced by my clean-up.
> Here's the probable fix.
> Thanks for the report!
>
> diff --git a/src/ls.c b/src/ls.c
> index 397e4ea..b548382 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -3213,7 +3213,8 @@ make_link_name (char const *name, char const *linkname)
>      return xstrdup (linkname);
>
>    char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
> -  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
> +  bool prefix_ends_in_slash = ISSLASH (name[prefix_len - 1]);
> +  stpcpy (stpncpy (p, name, prefix_len + !prefix_ends_in_slash), linkname);

Humph.  That's wrong, too.

Message #14 received at 11453 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 11453 <at> debbugs.gnu.org
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Fri, 11 May 2012 21:12:24 +0200

Jim Meyering wrote:

> Mike Frysinger wrote:
>> coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug
>> before, so looks like a regression with 8.17
>>
>> easy to show:
>> $ sudo ln -s dev /foo
>> $ ls --color=auto /
>> ... foo wrongly shows up in blinky text indicating it's a broken symlink ...
>> $ ls --color=auto /.
>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>> $ cd / ; ls --color=auto
>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>> -mike
>
> Rats.
> That bug was introduced by my clean-up.
> Here's the probable fix.
> Thanks for the report!
>
> diff --git a/src/ls.c b/src/ls.c
> index 397e4ea..b548382 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -3213,7 +3213,8 @@ make_link_name (char const *name, char const *linkname)
>      return xstrdup (linkname);
>
>    char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
> -  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
> +  bool prefix_ends_in_slash = ISSLASH (name[prefix_len - 1]);
> +  stpcpy (stpncpy (p, name, prefix_len + !prefix_ends_in_slash), linkname);

While the above fixes your problem, it fails to
solve the problem when running e.g.,

  mkdir -p d/b; (cd d; ln -s b link); ls --color d

(i.e., with a single-component base directory name and a symlink
to a relative name it would still color "link" as if it were dangling)

This fixes both:

diff --git a/src/ls.c b/src/ls.c
index 397e4ea..93a3338 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3206,14 +3206,9 @@ make_link_name (char const *name, char const *linkname)
   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);
-  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
+  char *d = dir_name (name);
+  char *p = file_name_concat (d, linkname, NULL);
+  free (d);
   return p;
 }

Message #17 received at 11453 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 11453 <at> debbugs.gnu.org
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Fri, 11 May 2012 21:45:40 +0200

Jim Meyering wrote:
> Jim Meyering wrote:
>> Mike Frysinger wrote:
>>> coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug
>>> before, so looks like a regression with 8.17
>>>
>>> easy to show:
>>> $ sudo ln -s dev /foo
>>> $ ls --color=auto /
>>> ... foo wrongly shows up in blinky text indicating it's a broken symlink ...
>>> $ ls --color=auto /.
>>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>>> $ cd / ; ls --color=auto
>>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>>> -mike
>>
>> Rats.
>> That bug was introduced by my clean-up.
>> Here's the probable fix.
>> Thanks for the report!
>>
>> diff --git a/src/ls.c b/src/ls.c
>> index 397e4ea..b548382 100644
>> --- a/src/ls.c
>> +++ b/src/ls.c
>> @@ -3213,7 +3213,8 @@ make_link_name (char const *name, char const *linkname)
>>      return xstrdup (linkname);
>>
>>    char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
>> -  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
>> +  bool prefix_ends_in_slash = ISSLASH (name[prefix_len - 1]);
>> +  stpcpy (stpncpy (p, name, prefix_len + !prefix_ends_in_slash), linkname);
>
> While the above fixes your problem, it fails to
> solve the problem when running e.g.,
>
>   mkdir -p d/b; (cd d; ln -s b link); ls --color d

Actually the above is fine after all --
and it uses less memory than the patch below.
I must have been testing something else.

> (i.e., with a single-component base directory name and a symlink
> to a relative name it would still color "link" as if it were dangling)
>
> This fixes both:
>
> diff --git a/src/ls.c b/src/ls.c
> index 397e4ea..93a3338 100644
> --- a/src/ls.c
> +++ b/src/ls.c
> @@ -3206,14 +3206,9 @@ make_link_name (char const *name, char const *linkname)
>    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);
> -  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
> +  char *d = dir_name (name);
> +  char *p = file_name_concat (d, linkname, NULL);
> +  free (d);
>    return p;
>  }

Message #22 received at 11453-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mike Frysinger <vapier <at> gentoo.org>
Cc: 11453-done <at> debbugs.gnu.org
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Sat, 12 May 2012 16:27:33 +0200

Jim Meyering wrote:
> Jim Meyering wrote:
>> Mike Frysinger wrote:
>>> coreutils-8.16 works fine (confirmed), and i don't recall seeing this bug
>>> before, so looks like a regression with 8.17
>>>
>>> easy to show:
>>> $ sudo ln -s dev /foo
>>> $ ls --color=auto /
>>> ... foo wrongly shows up in blinky text indicating it's a broken symlink ...
>>> $ ls --color=auto /.
>>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>>> $ cd / ; ls --color=auto
>>> ... foo correctly shows up with normal coloring indicating it's a symlink ...
>>> -mike
>>
>> Rats.
>> That bug was introduced by my clean-up.
>> Here's the probable fix.
>> Thanks for the report!
>>
>> diff --git a/src/ls.c b/src/ls.c
>> index 397e4ea..b548382 100644
>> --- a/src/ls.c
>> +++ b/src/ls.c
>> @@ -3213,7 +3213,8 @@ make_link_name (char const *name, char const *linkname)
>>      return xstrdup (linkname);
>>
>>    char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
>> -  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
>> +  bool prefix_ends_in_slash = ISSLASH (name[prefix_len - 1]);
>> +  stpcpy (stpncpy (p, name, prefix_len + !prefix_ends_in_slash), linkname);
>
> Humph.  That's wrong, too.

Actually it's fine.
Here's that same semantic change, presented in a more readable manner,
along with NEWS and a new test.

The only way I found to reproduce this problem without write access
to "/" is if there's already an entry there that is a non-dangling
symlink-to-relative-name.  Since those exist on Fedora 17 (/lib -> /usr/lib,
/bin -> /usr/bin, etc.) and on Debian unstable, (I see relative vmlinuz
and initrd.img links), that seems good enough.  It's not worth the cost
of an additional root-only test just to ensure this bug doesn't sneak
back in.

From 6124a3842dfa8484b52e067a8ab8105c3875a4f7 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Thu, 10 May 2012 19:43:00 +0200
Subject: [PATCH] ls: color each symlink-to-relative-name in / properly

In order for ls --color to color each symlink, it must form the name
of each referent and then stat it to see if the link is dangling, to
a directory, to a file, etc.  When the symlink is to a relative name,
ls must concatenate the starting directory name and that relative name.
When, in addition, the starting directory was "/" or "/some-name",
the result was ill-formed, and the subsequent stat would usually fail,
making the caller color it as a dangling symlink.
* src/ls.c (make_link_name): Don't botch the case in which
dir_name(NAME) == "/" and LINKNAME is relative.
* tests/ls/root-rel-symlink-color: New file.  Test for the above.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
Reported by Mike Frysinger in http://bugs.gnu.org/11453
Bug introduced by commit v8.16-23-gbcb9078.
---
 NEWS                            |  5 ++++
 src/ls.c                        |  9 +++++++-
 tests/Makefile.am               |  1 +
 tests/ls/root-rel-symlink-color | 51 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100755 tests/ls/root-rel-symlink-color

diff --git a/NEWS b/NEWS
index 6c620b3..f9e9c70 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU coreutils NEWS                                    -*- outline -*-

 * Noteworthy changes in release ?.? (????-??-??) [?]

+** Bug fixes
+
+  ls --color would mis-color relative-named symlinks in /
+  [bug introduced in coreutils-8.17]
+

 * Noteworthy changes in release 8.17 (2012-05-10) [stable]

diff --git a/src/ls.c b/src/ls.c
index 397e4ea..9494ae9 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3213,7 +3213,14 @@ make_link_name (char const *name, char const *linkname)
     return xstrdup (linkname);

   char *p = xmalloc (prefix_len + 1 + strlen (linkname) + 1);
-  stpcpy (stpncpy (p, name, prefix_len + 1), linkname);
+
+  /* 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;
 }

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a4370a6..0bafc5f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -449,6 +449,7 @@ TESTS =						\
   ls/proc-selinux-segfault			\
   ls/readdir-mountpoint-inode			\
   ls/recursive					\
+  ls/root-rel-symlink-color			\
   ls/rt-1					\
   ls/slink-acl					\
   ls/stat-dtype					\
diff --git a/tests/ls/root-rel-symlink-color b/tests/ls/root-rel-symlink-color
new file mode 100755
index 0000000..d795432
--- /dev/null
+++ b/tests/ls/root-rel-symlink-color
@@ -0,0 +1,51 @@
+#!/bin/sh
+# Exercise the 8.17 ls bug with coloring relative-named symlinks in "/".
+
+# Copyright (C) 2012 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 <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ ls
+
+symlink_to_rel=
+for i in /*; do
+  # Skip non-symlinks:
+  env test -h "$i" || continue
+
+  # Skip dangling symlinks:
+  env test -e "$i" || continue
+
+  # Skip any symlink-to-absolute-name:
+  case $(readlink "$i") in /*) continue ;; esac
+
+  symlink_to_rel=$i
+  break
+done
+
+test -z "$symlink_to_rel" \
+  && skip_ no relative symlink in /
+
+e='\33'
+color_code='01;36'
+c_pre="$e[0m$e[${color_code}m"
+c_post="$e[0m"
+printf "$c_pre$symlink_to_rel$c_post\n" > exp || framework_failure_
+
+env TERM=xterm LS_COLORS="ln=$color_code:or=1;31;42" \
+  ls -d --color=always "$symlink_to_rel" > out || fail=1
+
+compare exp out || fail=1
+
+Exit $fail
--
1.7.10.2.484.gcd07cc5

Message #25 received at 11453 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: 11453 <at> debbugs.gnu.org, jim <at> meyering.net
Subject: Re: bug#11453: `ls` in coreutils-8.17 incorrectly shows broken
	symlinks in /
Date: Mon, 14 May 2012 06:43:53 -0600

[Message part 1 (text/plain, inline)]

On 05/12/2012 08:27 AM, Jim Meyering wrote:

> Subject: [PATCH] ls: color each symlink-to-relative-name in / properly
> 
> In order for ls --color to color each symlink, it must form the name
> of each referent and then stat it to see if the link is dangling, to
> a directory, to a file, etc.  When the symlink is to a relative name,
> ls must concatenate the starting directory name and that relative name.
> When, in addition, the starting directory was "/" or "/some-name",
> the result was ill-formed, and the subsequent stat would usually fail,
> making the caller color it as a dangling symlink.

Ooh, sounds like I'd better check that cygwin properly handles symlinks
relative to / vs. // (although cygwin doesn't permit the creation of
symlinks in // proper, as that directory is a read-only representation
of other networked machines accessible at the moment).

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.