zsh-workers
 help / color / Atom feed
* [bug] :P modifier and symlink loops
@ 2020-01-11 17:00 Stephane Chazelas
  2020-02-01 17:57 ` Stephane Chazelas
  0 siblings, 1 reply; 5+ messages in thread
From: Stephane Chazelas @ 2020-01-11 17:00 UTC (permalink / raw)
  To: Zsh hackers list

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?

-- 
Stephane

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [bug] :P modifier and symlink loops
  2020-01-11 17:00 [bug] :P modifier and symlink loops Stephane Chazelas
@ 2020-02-01 17:57 ` Stephane Chazelas
  2020-02-02  8:10   ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: Stephane Chazelas @ 2020-02-01 17:57 UTC (permalink / raw)
  To: Zsh hackers list

Ping:

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?

-- 
Stephane

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [bug] :P modifier and symlink loops
  2020-02-01 17:57 ` Stephane Chazelas
@ 2020-02-02  8:10   ` Daniel Shahaf
  2020-02-02 17:03     ` Stephane Chazelas
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Shahaf @ 2020-02-02  8:10 UTC (permalink / raw)
  To: Stephane Chazelas; +Cc: Zsh hackers list

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [bug] :P modifier and symlink loops
  2020-02-02  8:10   ` Daniel Shahaf
@ 2020-02-02 17:03     ` Stephane Chazelas
  2020-02-03  5:19       ` Daniel Shahaf
  0 siblings, 1 reply; 5+ messages in thread
From: Stephane Chazelas @ 2020-02-02 17:03 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

2020-02-02 08:10:21 +0000, Daniel Shahaf:
[...]
> 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.

That works for me, I agree it's a pathological condition that
may be worth reporting to the user, and to do that, there's
probably no other alternative than to exit the current shell
process.

> I think the first option could be implemented along the lines of:
> 
> 1. Call realpath($arg).
> 2. If it returns ELOOP,

... and doesn't end in /..

the . path components would have to be removed first as well.

> call realpath(${arg:h}) and append "/${arg:t}".

And what if *that* realpath() fails with ELOOP? Do we carry on
with $arg:h:h?

> 3. Otherwise, throw an exception (i.e., set errflag).
[...]

-- 
Stephane

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [bug] :P modifier and symlink loops
  2020-02-02 17:03     ` Stephane Chazelas
@ 2020-02-03  5:19       ` Daniel Shahaf
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Shahaf @ 2020-02-03  5:19 UTC (permalink / raw)
  To: Stephane Chazelas; +Cc: Zsh hackers list

Stephane Chazelas wrote on Sun, 02 Feb 2020 17:03 +0000:
> 2020-02-02 08:10:21 +0000, Daniel Shahaf:
> [...]
> > 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.  
> 
> That works for me, I agree it's a pathological condition that
> may be worth reporting to the user, and to do that, there's
> probably no other alternative than to exit the current shell
> process.
> 
> > I think the first option could be implemented along the lines of:
> > 
> > 1. Call realpath($arg).
> > 2. If it returns ELOOP,  
> 
> ... and doesn't end in /..

I assume you mean "ends in slash-dot-dot".

> the . path components would have to be removed first as well.

Good point, but I'd expect slightly different behaviour.  Dot and
dot-dot components before the last, may-be-loop components don't need
special handling; they'll get resolved in the ordinary way.  However,
if [[ $arg == (*/|*/.|*/..) ]], the ELOOP special case shouldn't be
applied.

The cases that $arg is "", ".", or ".." are not going to report ELOOP,
are they?

> > call realpath(${arg:h}) and append "/${arg:t}".  
> 
> And what if *that* realpath() fails with ELOOP? Do we carry on
> with $arg:h:h?

No; we'll call zerr() [which sets errflag] and bail out, just like the
NO_MATCH option does.  Only the last path component (after the last slash,
or the entire string if it has no slashes) will be allowed to be a loop.

> > 3. Otherwise, throw an exception (i.e., set errflag).  
> [...]
> 

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-11 17:00 [bug] :P modifier and symlink loops Stephane Chazelas
2020-02-01 17:57 ` Stephane Chazelas
2020-02-02  8:10   ` Daniel Shahaf
2020-02-02 17:03     ` Stephane Chazelas
2020-02-03  5:19       ` Daniel Shahaf

zsh-workers

Archives are clonable: git clone --mirror http://inbox.vuxu.org/zsh-workers

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git