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