Re: [bug] :P modifier and symlink loops

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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

Cheers,

Daniel

P.S. Here's a quick test for the "loop in the last path component" case:

diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 3d7df94c9..a5657be59 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -742,6 +742,16 @@
 >glob.tmp/secret-s111/  glob.tmp/secret-s111
 >glob.tmp/secret-s444/  glob.tmp/secret-s444
 
+ ln -s loop glob.tmp/loop
+ ln -s loop glob.tmp/trap
+ { 
+   $ZTST_testdir/../Src/zsh -fc 'echo $1:P' . glob.tmp/trap
+ } always {
+   rm -f glob.tmp/trap glob.tmp/loop
+ }
+-f:the ':P' modifier handles symlink loops in the last path component
+*>*/(trap|loop)
+
 %clean
 
  # Fix unreadable-directory permissions so ztst can clean up properly