Re: [bug] :P modifier and symlink loops

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Thu, Apr 24, 2025 at 8:19 PM Mikael Magnusson <mikachu@xxxxxxxxx> wrote:
>
> On Sat, Mar 21, 2020 at 8:51 PM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Daniel Shahaf wrote on Sun, 02 Feb 2020 08:10 +0000:
> > > Stephane Chazelas wrote on Sat, 01 Feb 2020 17:57 +0000:
> > > > Ping:
> > >
> > > Thanks for the ping.  I've added this to Etc/BUGS so we don't forget
> > > it.  I worked on :P before, so I've added this to my list to
> > > investigate further, but I don't know when I'll get to it.
> > >
> > > > 2020-01-11 17:00:47 +0000, Stephane Chazelas:
> > > > Hi,
> > > >
> > > > I've got the feeling it's been discussed before, but could not
> > > > find it in the archives.
> > > >
> > > > $ ln -s loop /tmp/
> > > > $ f=/tmp/loop strace ~/install/cvs/zsh/Src/zsh -c '$f:P'
> > > > [...]
> > > > readlink("/tmp/loop", "loop", 4096)     = 4
> > > > readlink("/tmp/loop", "loop", 4096)     = 4
> > > > [...]
> > > > readlink("/tmp/loop", "loop", 4096)     = 4
> > > > readlink("/tmp/loop", "loop", 4096)     = 4
> > > > --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR,
> > > > si_addr=0x7ffec7a345e0} ---
> > > > +++ killed by SIGSEGV +++
> > > >
> > > > possibly stack overflow caused by unbound recursion or buffer
> > > > overflow on /tmp/loop/loop... but the bigger question is what to
> > > > do here.
> > > >
> > > > The ELOOP problem is usually addressed by giving up after an
> > > > arbitrary number of symlinks has been resolved (regardless of
> > > > whether there is indeed a loop or not) in the lookup of the
> > > > file, but here $f:P *has* to expand to something, so what should
> > > > that be?
> > > >
> > > > For instance, for
> > > >
> > > > cd /
> > > > file=bin/../tmp/loop/../foo/.. above?
> > > >
> > > > The only thing I can think of is expand to:
> > > >
> > > > /tmp/loop/../foo/..
> > > >
> > > > (maybe done by first doing a stat(the-file); if it returns
> > > > ELOOP, do a stat() at each stage of the resolution and give up
> > > > on the first ELOOP).
> > > >
> > > > Any other idea?
> > >
> > > The postcondition of :P is "no dot or dot-dot components and no symlinks".
> > >
> > > When the loop is on the last path component (as in ${${:-/tmp/loop}:P}
> > > and ${${:-/tmp/trap}:P} after «ln -s loop /tmp/trap») we could still print
> > > a path to the loop symlink that meets the postcondition, except for the loop
> > > symlink in the last path component.
> > >
> > > However, in ${${:-"/tmp/loop/../foo"}} we can't meet the postcondition.
> > > I think our options are either to throw an exception, like a glob with
> > > no matches does, or to keep the additional components verbatim, as you
> > > suggest.
> > >
> > > Intuitively I lean towards the first option.  We aren't a CGI script,
> > > where PATH_INFO is to be expected.  If we can't return a path without
> > > dot and dot-dot components and without symlinks, we should raise an
> > > error rather than continue silently. However, I'm open to alternatives.
> > >
> > > I think the first option could be implemented along the lines of:
> > >
> > > 1. Call realpath($arg).
> > > 2. If it returns ELOOP, call realpath(${arg:h}) and append "/${arg:t}".
> > > 3. Otherwise, throw an exception (i.e., set errflag).
> >
> > Patch series attached.
> >
> > I ended up implementing the second option — keeping the trailing
> > components verbatim — for several reasons:
> >
> > 1. It's actually documented this way for :P.  (xsymlink() has other
> > callers too, but I didn't check whether any of them specifically relied
> > on this behaviour.)
> >
> > 2. After I made the code use the realpath() wrapper function,
> > chabspath(), rather than xsymlinks() (plural), that's the behaviour
> > I observed, and I didn't go out of my way to change it.
> >
> > I suppose we could revisit :P's behaviour on symlink loops with
> > trailing components after the loop, but in the meantime, this at least
> > fixes the segfault.
> >
> > WDYT?
> >
> > Cheers,
> >
> > Daniel
>
> Patch six in this series breaks whence -s, I guess it is not a very
> common usecase since nobody complained until now.
> Expected:
> % Src/zsh -c 'which -s git'
> /usr/bin/git
> Actual:
> % Src/zsh -c 'which -s git'
> /usr/bin/git -> /usr/bin/git
>
> Reverting the patch seems to resolve the issue, but I'm not sure what
> the intent of the change was in the first place, the patch has no
> explanation for it. The series talks about symlink loops, but a
> symlink loop is effectively a broken symlink, and does not seem to be
> found by 'which' at all (with the patch reverted):
> % ln -s foo ~/bin/bar
> % ln -s bar ~/bin/foo
> % Src/zsh -c 'which -s foo'
> foo not found
>
> I know that Daniel isn't very active anymore, but if anyone else has
> any thoughts?

Ah, because of other changes, the xsymlinks version now only does one
step of resolution, so some more thought is required. It is clear from
inspecting chrealpath that its return value does not depend at all on
whether the target path is a symlink or not, though. The following
seems to work but probably does some needless work,
diff --git c/Src/utils.c w/Src/utils.c
index f6e63711ab..f699a6f5e1 100644
--- c/Src/utils.c
+++ w/Src/utils.c
@@ -985,10 +985,10 @@ void
 print_if_link(char *s, int all)
 {
     if (*s == '/') {
+       *xbuf = '\0';
        if (all) {
            char *start = s + 1;
            char xbuflink[PATH_MAX+1];
-           *xbuf = '\0';
            for (;;) {
                if (xsymlinks(start) > 0) {
                    printf(" -> ");
@@ -1003,10 +1003,12 @@ print_if_link(char *s, int all)
                }
            }
        } else {
-           if (chrealpath(&s, 'P', 0)) {
-               printf(" -> ");
-               zputs(*s ? s : "/", stdout);
-               zsfree(s);
+           if (xsymlinks(s + 1) > 0) {
+               if (chrealpath(&s, 'P', 0)) {
+                   printf(" -> ");
+                   zputs(*s ? s : "/", stdout);
+                   zsfree(s);
+               }
            }
        }
     }


-- 
Mikael Magnusson