GNU bug report logs - #63931
ls colors one symlink too much as non-broken in symlink chain

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 63931 in the body.
You can then email your comments to 63931 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: Martin Schulte <gnu <at> schrader-schulte.de>
To: bug-coreutils <at> gnu.org
Subject: ls colors one symlink too much as non-broken in symlink chain
Date: Tue, 6 Jun 2023 19:24:26 +0200

Hello coreutils-maintainers,

I create a long chain of symlinks - ls colors the 41st element as ok while the kernel already gives up after 40 symlinks:

$ uname -a
Linux martnix4 5.10.0-23-amd64 #1 SMP Debian 5.10.179-1 (2023-05-12) x86_64 GNU/Linux
$ ls --version | head -n 1
ls (GNU coreutils) 8.32
$ mkdir empty
$ cd empty
$ last=00 ; touch $last ; for ln in {01..50}; do ln -s $last $ln; last=$ln; done
$ ls --color -l {38..42}
lrwxrwxrwx 1 u g 2 Jun  6 19:00 38 -> 37   <- 38 colored as symlink
lrwxrwxrwx 1 u g 2 Jun  6 19:00 39 -> 38   <- 39 colored as symlink
lrwxrwxrwx 1 u g 2 Jun  6 19:00 40 -> 39   <- 40 colored as symlink
lrwxrwxrwx 1 u g 2 Jun  6 19:00 41 -> 40   <- 41 colored as symlink
lrwxrwxrwx 1 u g 2 Jun  6 19:00 42 -> 41   <- 42 and 41 colored as broken link
$ cat 40
$ cat 41
cat: 41: Too many levels of symbolic links

This could be reproduced with "ls (GNU coreutils) 9.3.45-6a618" compiled from the git repository.

Best regards,

Martin

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Martin Schulte <gnu <at> schrader-schulte.de>, 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Tue, 6 Jun 2023 21:32:43 +0100

On 06/06/2023 18:24, Martin Schulte wrote:
> Hello coreutils-maintainers,
> 
> I create a long chain of symlinks - ls colors the 41st element as ok while the kernel already gives up after 40 symlinks:
> 
> $ uname -a
> Linux martnix4 5.10.0-23-amd64 #1 SMP Debian 5.10.179-1 (2023-05-12) x86_64 GNU/Linux
> $ ls --version | head -n 1
> ls (GNU coreutils) 8.32
> $ mkdir empty
> $ cd empty
> $ last=00 ; touch $last ; for ln in {01..50}; do ln -s $last $ln; last=$ln; done
> $ ls --color -l {38..42}
> lrwxrwxrwx 1 u g 2 Jun  6 19:00 38 -> 37   <- 38 colored as symlink
> lrwxrwxrwx 1 u g 2 Jun  6 19:00 39 -> 38   <- 39 colored as symlink
> lrwxrwxrwx 1 u g 2 Jun  6 19:00 40 -> 39   <- 40 colored as symlink
> lrwxrwxrwx 1 u g 2 Jun  6 19:00 41 -> 40   <- 41 colored as symlink
> lrwxrwxrwx 1 u g 2 Jun  6 19:00 42 -> 41   <- 42 and 41 colored as broken link
> $ cat 40
> $ cat 41
> cat: 41: Too many levels of symbolic links
> 
> This could be reproduced with "ls (GNU coreutils) 9.3.45-6a618" compiled from the git repository.
> 
> Best regards,

This is because we lstat() the link, and stat() the target,
I suppose as a small optimization in the normal case.
The following adjusts to stat the link instead,
which works for your case and passes existing tests.

I'll think a bit more about it and add a test before applying.

cheers,
Pádraig

diff --git a/src/ls.c b/src/ls.c
index 71d94fd6a..77be54248 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3586,7 +3586,7 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
              they won't be traced and when no indicator is needed.  */
           if (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;

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Martin Schulte <gnu <at> schrader-schulte.de>, 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Tue, 6 Jun 2023 15:10:49 -0700

On 6/6/23 13:32, Pádraig Brady wrote:
> I'll think a bit more about it and add a test before applying.

I wouldn't test that the 'ls' output exactly matches the 'cat' output. 
For example, it should be OK for stat_for_mode to pass the parent 
directory fd (instead of AT_FDCWD) to statx, for efficiency, and in that 
case the number of symbolic links traversed by 'ls' won't match the 
number traversed by 'cat', because the full name might traverse symlinks 
that statx (dirfd, ...) won't.

And it's OK if these two numbers don't match, as users shouldn't assume 
that the ELOOP limit is a constant.

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

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

From: Martin Schulte <gnu <at> schrader-schulte.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Pádraig Brady
 <P <at> draigBrady.com>
Cc: 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Wed, 7 Jun 2023 11:56:08 +0200

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.

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.

Martin

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Martin Schulte <gnu <at> schrader-schulte.de>
Cc: 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Wed, 7 Jun 2023 13:23:55 +0100

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;

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Martin Schulte <gnu <at> schrader-schulte.de>
Cc: 63931-done <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Wed, 7 Jun 2023 16:01:42 +0100

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

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

[0001-ls-use-more-standard-symlink-traversal.patch (text/x-patch, attachment)]

[0002-ls-display-command-line-symlinks-that-return-ELOOP.patch (text/x-patch, attachment)]

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>,
 Martin Schulte <gnu <at> schrader-schulte.de>
Cc: 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Wed, 7 Jun 2023 13:17:15 -0700

On 2023-06-07 05:23, Pádraig Brady wrote:
> -                            ? errno == ENOENT
> +                            ? (errno == ENOENT || errno == ELOOP)

This also needs a change in the succeeding comment, which starts "stat 
failed because of ENOENT"

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Martin Schulte <gnu <at> schrader-schulte.de>
Cc: 63931 <at> debbugs.gnu.org
Subject: Re: bug#63931: ls colors one symlink too much as non-broken in
 symlink chain
Date: Wed, 7 Jun 2023 21:54:23 +0100

On 07/06/2023 21:17, Paul Eggert wrote:
> On 2023-06-07 05:23, Pádraig Brady wrote:
>> -                            ? errno == ENOENT
>> +                            ? (errno == ENOENT || errno == ELOOP)
> 
> This also needs a change in the succeeding comment, which starts "stat
> failed because of ENOENT"

Fixed.

cheers,
Pádraig

Previous Next

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