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