zsh 5.0.5-dev-2

zsh 5.0.5-dev-2

[Peter Stephenson]

Updated test build at

http://www.zsh.org/pub/development/

The fix for building the documentation when compiling out of the source
tree is the only significant change to the source.

Unless more issues turn up I'll make a release later in the week.  I'm
on holiday next week, as is traditional.

pws

Re: zsh 5.0.5-dev-2

[Axel Beckert]

Hi,

On Tue, Aug 12, 2014 at 09:29:20PM +0100, Peter Stephenson wrote:
> Updated test build at
> 
> http://www.zsh.org/pub/development/
> 
> The fix for building the documentation when compiling out of the source
> tree is the only significant change to the source.

Builds and works fine as Debian package on amd64, locally compiled as well as
compiled on the Jenkins. Thanks!

Will upload it later (latest Wednesday evening) to Debian Experimental
to also let it build on other architectures.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi,

On Wed, Aug 13, 2014 at 12:36:38AM +0200, Axel Beckert wrote:
> On Tue, Aug 12, 2014 at 09:29:20PM +0100, Peter Stephenson wrote:
> > The fix for building the documentation when compiling out of the source
> > tree is the only significant change to the source.
> 
> Builds and works fine as Debian package on amd64, locally compiled as well as
> compiled on the Jenkins. Thanks!

I had one local build (Linux 3.15, amd64/x86_64) which stopped at
Test/A05execution.ztst but didn't investigate further. I pressed
Ctrl-C, started it again and it built fine the second time.

> Will upload it later (latest Wednesday evening) to Debian Experimental
> to also let it build on other architectures.

It built fine on most archtectures so far, only MIPS is still lagging
behind, and the kfreebsd-amd64 build -- but neither the (linux-)amd64
nor the kfreebsd-i386 -- failed.

And that build failed by stopping at exactly the same test as in that
one case on my Thinkpad:

https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=kfreebsd-amd64&ver=5.0.5-dev-2-1&stamp=1407936338

I'll try to get that build repeated, too, but since it now happened
more than once at the same test on different machines and
architectures/kernels, I think it may be worth investigating.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Aug 13, 2014 10:51 AM, "Axel Beckert" <abe@deuxchevaux.org> wrote:
>
> I had one local build (Linux 3.15, amd64/x86_64) which stopped at
> Test/A05execution.ztst but didn't investigate further. I pressed
> Ctrl-C, started it again and it built fine the second time.
>
>
>
https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=kfreebsd-amd64&ver=5.0.5-dev-2-1&stamp=1407936338

That's an interesting one because it's testing for the shell hanging - but
the test itself is supposed to time out and kill the stuck shell, which is
also not happening.  This makes me wonder if both problems are related to
the automated build system, i.e., the test is assuming something about the
execution environment that isn't true when running as an automated job.

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi,

On Wed, Aug 13, 2014 at 12:08:41PM -0700, Bart Schaefer wrote:
> On Aug 13, 2014 10:51 AM, "Axel Beckert" <abe@deuxchevaux.org> wrote:
> > I had one local build (Linux 3.15, amd64/x86_64) which stopped at
> > Test/A05execution.ztst but didn't investigate further. I pressed
> > Ctrl-C, started it again and it built fine the second time.

JFTR: That happened with the "pbuilder", which untars a minimal
system, chroots into it, installs required build-dependencies, builds
the package and cleans up. It keeps a terminal. As mentioned it
happened once, and just "Ctrl-C, Cursor-up, Enter" succeeded.

> https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=kfreebsd-amd64&ver=5.0.5-dev-2-1&stamp=1407936338

This was with sbuild, which does similar things, but as AFAIK a daemon
and without connected terminal. As you can see on
https://buildd.debian.org/status/logs.php?pkg=zsh&ver=5.0.5-dev-2-1&arch=kfreebsd-amd64
this seems now reproducible on that architecture with that build
system. At least I currently consider it reproducible there. Will
investigate later today, likely in the evening.

> That's an interesting one because it's testing for the shell hanging - but
> the test itself is supposed to time out and kill the stuck shell, which is
> also not happening.  This makes me wonder if both problems are related to
> the automated build system, i.e., the test is assuming something about the
> execution environment that isn't true when running as an automated
> job.

They didn't happen in the past (at least I can't remember any cases --
not counting any issues on Hurd, though) and I don't think the build
systems have changed that much recently.

Nevertheless, that guess still sounds very reasonable for me. So
thanks for that background information, it will help me to investigate
the issue.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Peter Stephenson]

On Wed, 13 Aug 2014 12:08:41 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 13, 2014 10:51 AM, "Axel Beckert" <abe@deuxchevaux.org> wrote:
> >
> > I had one local build (Linux 3.15, amd64/x86_64) which stopped at
> > Test/A05execution.ztst but didn't investigate further. I pressed
> > Ctrl-C, started it again and it built fine the second time.
> >
> >
> >
> https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=kfreebsd-amd64&ver=5.0.5-dev-2-1&stamp=1407936338
> 
> That's an interesting one because it's testing for the shell hanging - but
> the test itself is supposed to time out and kill the stuck shell, which is
> also not happening.  This makes me wonder if both problems are related to
> the automated build system, i.e., the test is assuming something about the
> execution environment that isn't true when running as an automated job.

Those tests make quite nasty timing assumptions.  I'd definitely be
inclined to suspect the test rather than the shell.

pws

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Bart Schaefer]

On Aug 14,  9:50am, Peter Stephenson wrote:
}
} Those tests make quite nasty timing assumptions.  I'd definitely be
} inclined to suspect the test rather than the shell.

It is true that the test assumes ( printf "%d\n" {2..20000} ) will take
less than 5 seconds, but that should cause the test to report failure
rather than to hang.

( coproc { read -Et 5 || kill -INT $$ } ) ought to send the signal in
5 seconds regardless of what else is happening, so the question is
why is that signal either not being sent or being ignored?

I guess my first inclination would be to change the final "read -Ep" to
"read -Et 6 -p" just in case it's that step that's hanging.

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi,

On Thu, Aug 14, 2014 at 09:25:27AM -0700, Bart Schaefer wrote:
> On Aug 14,  9:50am, Peter Stephenson wrote:
> } Those tests make quite nasty timing assumptions.  I'd definitely be
> } inclined to suspect the test rather than the shell.
> 
> It is true that the test assumes ( printf "%d\n" {2..20000} ) will take
> less than 5 seconds, but that should cause the test to report failure
> rather than to hang.

So far I wasn't able to reproduce the error in a setup where I can
debug it yet. I've run the testsuite successfully about 70 times each
for the dynamically linked zsh as well as the statically linked zsh on
kfreebsd-amd64.

I'm now back to trying to reproduce it on that (linux amd64) machine
were I initially run into it a single time. With some luck, I may be
able to trigger it and then debug it.

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi again,

some more gathered thoughts and experiments around this issue:

On Thu, Aug 14, 2014 at 06:31:25PM +0200, Axel Beckert wrote:
> So far I wasn't able to reproduce the error in a setup where I can
> debug it yet. I've run the testsuite successfully about 70 times each
> for the dynamically linked zsh as well as the statically linked zsh on
> kfreebsd-amd64.

One thing I noticed in the failed build logs: Exactly before
terminating the hanging test suite there is the following message:

Unable to change MONITOR option

Full context:

../../Test/A05execution.ztst: starting.
Unable to change MONITOR option
This test takes 5 seconds to fail...
E: Caught signal ‘Terminated’: terminating immediately
make[1]: *** [test] Terminated
make[2]: *** [check] Terminated
Makefile:264: recipe for target 'test' failed
debian/rules:48: recipe for target 'build-arch' failed
Makefile:188: recipe for target 'check' failed
make: *** [build-arch] Terminated
Build killed with signal TERM after 150 minutes of inactivity

Full log:

https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=kfreebsd-amd64&ver=5.0.5-dev-2-1&stamp=1407936338

Then again this works well despite producing the above message, even many
times in a row:

% ssh localhost "cd `pwd`;./ztst.zsh A05execution.ztst"
A05execution.ztst: starting.
Unable to change MONITOR option
This test takes 5 seconds to fail...
A05execution.ztst: all tests successful.
%

Even inside the kfreebsd-amd64 chroot where I installed the freshly
and sucessfully (locally) built zsh 5.0.5-dev-2 package, I couldn't
reproduce it in a shell not being connected to a terminal.

I also thought, I remember having seen that before on zsh builds on
Debian GNU/Hurd I completely disabled the test-suite there. But at
least https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=hurd-i386&ver=5.0.5-1&stamp=1389022672,
https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=hurd-i386&ver=5.0.4-1&stamp=1387664312
and https://buildd.debian.org/status/fetch.php?pkg=zsh&arch=hurd-i386&ver=5.0.3-1&stamp=1387092681
hung in Test/V08zpty.ztst and had no such message about the MONITOR
option.

So it's probably not (directly) related to not running on a connected
terminal.

(And JFTR: There were no build failures on kfreebsd-amd64 since 2009
with 4.3.10 and later until this week:
https://buildd.debian.org/status/logs.php?pkg=zsh&arch=kfreebsd-amd64
:-)

> I'm now back to trying to reproduce it on that (linux amd64) machine
> were I initially run into it a single time. With some luck, I may be
> able to trigger it and then debug it.

For that I ran the same kind of build where I ran into it locally
once, but modified so that the test suite runs 100 times in a row
instead of only once. No hangs either, though. :-(

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Bart Schaefer]

On Aug 14,  7:34pm, Axel Beckert wrote:
} 
} One thing I noticed in the failed build logs: Exactly before
} terminating the hanging test suite there is the following message:
} 
} Unable to change MONITOR option

Yes, that's coming from the third-to-last test in the file:

  { setopt MONITOR } 2>/dev/null
  [[ -o MONITOR ]] || print -u $ZTST_fd 'Unable to change MONITOR option'

Where as the "5 seconds" message is from the very last test:

  { unsetopt MONITOR } 2>/dev/null
  coproc { read -Et 5 || kill -INT $$ }
  print -u $ZTST_fd 'This test takes 5 seconds to fail...'

So two tests have succeeded between the "Unable" message and the one
that hangs.  The inability to change MONITOR is a side-effect of no
controlling terminal, so the test warns you.  Actually the warning
could be stronger than that, because the inability to setpt MONITOR
renders the test invalid (it will always succeed, even in the case
we are trying to regress).

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi,

On Thu, Aug 14, 2014 at 09:50:53PM -0700, Bart Schaefer wrote:
> On Aug 14,  7:34pm, Axel Beckert wrote:
> } 
> } One thing I noticed in the failed build logs: Exactly before
> } terminating the hanging test suite there is the following message:
> } 
> } Unable to change MONITOR option
> 
> Yes, that's coming from the third-to-last test in the file:
> 
>   { setopt MONITOR } 2>/dev/null
>   [[ -o MONITOR ]] || print -u $ZTST_fd 'Unable to change MONITOR option'
> 
> Where as the "5 seconds" message is from the very last test:
> 
>   { unsetopt MONITOR } 2>/dev/null
>   coproc { read -Et 5 || kill -INT $$ }
>   print -u $ZTST_fd 'This test takes 5 seconds to fail...'
> 
> So two tests have succeeded between the "Unable" message and the one
> that hangs.  The inability to change MONITOR is a side-effect of no
> controlling terminal, so the test warns you.

Thanks for the explanations!

> Actually the warning could be stronger than that, because the
> inability to setpt MONITOR renders the test invalid

So instead of or in addition to the warning, can we skip this test in
case of no controlling terminal? Because the situation where it
currently shows up reproducibly (on the build daemons), it's always
without controlling terminal.

(The one case where it seems to hang for me with a controlling
terminal, I wonder if I was just not patient enough. I think I waited
for at least 20 to 30 seconds before pressing Ctrl-C, but I'm no more
sure.)

> (it will always succeed, even in the case we are trying to regress).

Well, it at least hangs under some yet to be determined conditions. So
skipping the test if it doesn't do anything useful seems a step
forward for me. :-)

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Bart Schaefer]

On Aug 15, 11:32am, Axel Beckert wrote:
} 
} > Actually the warning could be stronger than that, because the
} > inability to setpt MONITOR renders the test invalid
} 
} So instead of or in addition to the warning, can we skip this test in
} case of no controlling terminal?

The test that has the warning and the test that is breaking for you are
two different tests.  The test that's "invalid" is NOT the one that is
causing your issue, so skipping the test that has the warning would not
resolve this discussion thread.  More specifically:

} > (it will always succeed, even in the case we are trying to regress).
} 
} Well, it at least hangs under some yet to be determined conditions.

The "will always succeed" statement does NOT apply to the test that is
hanging; sorry that wasn't clear enough.  The test that is hanging in
fact does NOT WANT the monitor option and is *intended* to regress
something that happens with no controlling terminal!

Which means that maybe it still happens, though why "kill -INT" is not
sufficient to end the test may have something to do with the build
environment (e.g., perhaps the auto-build blocks the signal because it
doesn't want anything it's building to be able to stop the entire
automation).  

} (The one case where it seems to hang for me with a controlling
} terminal, I wonder if I was just not patient enough. I think I waited
} for at least 20 to 30 seconds before pressing Ctrl-C, but I'm no more
} sure.)

If you had to wait more than 5 seconds plus some rounding error, you
had waited long enough.

I've committed a change to add the 6-second timeout on the final read
at the end of the test, see if that changes anything ...

Re: zsh 5.0.5-dev-2 / Occassional hangs on Test/A05execution.ztst

[Axel Beckert]

Hi Bart,

On Fri, Aug 15, 2014 at 10:33:30AM -0700, Bart Schaefer wrote:
> On Aug 15, 11:32am, Axel Beckert wrote:
> } > Actually the warning could be stronger than that, because the
> } > inability to setpt MONITOR renders the test invalid
> } 
> } So instead of or in addition to the warning, can we skip this test in
> } case of no controlling terminal?
> 
> The test that has the warning and the test that is breaking for you are
> two different tests.

Ah, I understood it that way, that the warning is for the whole
remainder of the file.

> } (The one case where it seems to hang for me with a controlling
> } terminal, I wonder if I was just not patient enough. I think I waited
> } for at least 20 to 30 seconds before pressing Ctrl-C, but I'm no more
> } sure.)
> 
> If you had to wait more than 5 seconds plus some rounding error, you
> had waited long enough.

That for sure.

> I've committed a change to add the 6-second timeout on the final read
> at the end of the test, see if that changes anything ...

Will see with the next upload to Debian. Thanks for caring!

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

Re: zsh 5.0.5/6

[Peter Stephenson]

It looks like it would be precipitate to release 5.0.6 before going on
holiday tomorrow, so I'll leave it for a week.

We appear to be almost there, but that may be because we've hit the
usual "and one more thing..." season.

pws

Re: zsh 5.0.5-dev-2

[Dominic Hopf]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

Good Evening from Hamburg, Germany,

just wanted to let you know this tarball built fine as a Fedora package
without running into any issues. For the interested people, it is available
under these URLs:

    https://dmaphy.fedorapeople.org/zsh-dev/
    http://koji.fedoraproject.org/koji/taskinfo?taskID=7291896


Best Regards and a nice evening,
Dominic


On Wed, Aug 13, 2014 at 12:36 AM, Axel Beckert <abe@deuxchevaux.org> wrote:

> Hi,
>
> On Tue, Aug 12, 2014 at 09:29:20PM +0100, Peter Stephenson wrote:
> > Updated test build at
> >
> > http://www.zsh.org/pub/development/
> >
> > The fix for building the documentation when compiling out of the source
> > tree is the only significant change to the source.
>
> Builds and works fine as Debian package on amd64, locally compiled as well
> as
> compiled on the Jenkins. Thanks!
>
> Will upload it later (latest Wednesday evening) to Debian Experimental
> to also let it build on other architectures.
>
>                 Kind regards, Axel
> --
> /~\  Plain Text Ribbon Campaign                   | Axel Beckert
> \ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org
>  (Mail)
>  X   See http://www.nonhtmlmail.org/campaign.html | abe@noone.org
> (Mail+Jabber)
> / \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/
> (Web)
>



-- 
Diese E-Mail ist nicht mit GPG signiert, da ich sie vom Webinterface aus
geschrieben habe.

This mail is not signed with GPG because I wrote it from web interface.

Re: zsh 5.0.5-dev-2

[Oliver Kiddle]

Peter Stephenson wrote:
> 
> Unless more issues turn up I'll make a release later in the week.

The bug reported in 32431 and 32550 is still there.

Can we perhaps just remove or comment out that one error message for a
release?

Oliver

Re: zsh 5.0.5-dev-2

[Peter Stephenson]

On Thu, 14 Aug 2014 00:34:54 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> The bug reported in 32431 and 32550 is still there.
> 
> Can we perhaps just remove or comment out that one error message for a
> release?

zsh -fw
cd /
zsh: path expansion failed, using root directory

We certainly need to do something.  But if this is the only issue, can't
it be straightforwardly fixed like this?

diff --git a/Src/utils.c b/Src/utils.c
index 998e46a..75fadd5 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -780,6 +780,8 @@ xsymlink(char *s)
 {
     if (*s != '/')
 	return NULL;
+    if (!s[1])
+	return ztrdup("/");
     *xbuf = '\0';
     xsymlinks(s + 1);
     if (!*xbuf) {

-- 
Peter Stephenson <p.stephenson@samsung.com>  Principal Software Engineer
Tel: +44 (0)1223 434724                Samsung Cambridge Solution Centre
St John's House, St John's Innovation Park, Cowley Road,
Cambridge, CB4 0DS, UK

Re: zsh 5.0.5-dev-2

[Peter Stephenson]

On Thu, 14 Aug 2014 09:34:42 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Thu, 14 Aug 2014 00:34:54 +0200
> Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > The bug reported in 32431 and 32550 is still there.
> > 
> > Can we perhaps just remove or comment out that one error message for a
> > release?
> 
> zsh -fw
> cd /
> zsh: path expansion failed, using root directory
> 
> We certainly need to do something.  But if this is the only issue, can't
> it be straightforwardly fixed like this?

If someone can reassure me that the weird undocumented interaction with
xbuf bypassing parameter passing starts here --- i.e. we are resetting
xbuf here, then xsymlinks fiddles with it and this code looks at the
result --- then I can't see how this change can be wrong as far as it
goes.  The question is, is there more to it?

> diff --git a/Src/utils.c b/Src/utils.c
> index 998e46a..75fadd5 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -780,6 +780,8 @@ xsymlink(char *s)
>  {
>      if (*s != '/')
>  	return NULL;
> +    if (!s[1])
> +	return ztrdup("/");
>      *xbuf = '\0';
>      xsymlinks(s + 1);
>      if (!*xbuf) {

pws

Re: zsh 5.0.5-dev-2

[Bart Schaefer]

On Aug 14, 10:32am, Peter Stephenson wrote:
} Subject: Re: zsh 5.0.5-dev-2
}
} > > Can we perhaps just remove or comment out that one error message for a
} > > release?
} > 
} > zsh -fw
} > cd /
} > zsh: path expansion failed, using root directory
} > 
} > We certainly need to do something.  But if this is the only issue, can't
} > it be straightforwardly fixed like this?
} 
} If someone can reassure me that the weird undocumented interaction with
} xbuf bypassing parameter passing starts here --- i.e. we are resetting
} xbuf here, then xsymlinks fiddles with it and this code looks at the
} result --- then I can't see how this change can be wrong as far as it
} goes.  The question is, is there more to it?

I don't have time to compose a more thorough response here, but I think
this may have been discussed in the previous thread.  I'll look at it
again in 10 hours or so if it will help ...

Re: zsh 5.0.5-dev-2

[Peter Stephenson]

On Thu, 14 Aug 2014 09:20:45 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 14, 10:32am, Peter Stephenson wrote:
> } Subject: Re: zsh 5.0.5-dev-2
> }
> } > > Can we perhaps just remove or comment out that one error message for a
> } > > release?
> } > 
> } > zsh -fw
> } > cd /
> } > zsh: path expansion failed, using root directory
> } > 
> } > We certainly need to do something.  But if this is the only issue, can't
> } > it be straightforwardly fixed like this?
> } 
> } If someone can reassure me that the weird undocumented interaction with
> } xbuf bypassing parameter passing starts here --- i.e. we are resetting
> } xbuf here, then xsymlinks fiddles with it and this code looks at the
> } result --- then I can't see how this change can be wrong as far as it
> } goes.  The question is, is there more to it?
> 
> I don't have time to compose a more thorough response here, but I think
> this may have been discussed in the previous thread.

Yes, it says anything that resolves to the root directory has the same
problem.

It's entirely unclear to me from looking at xsymlinks() when an empty
xbuf would actually constitute a failure.  If it can't expand something
as a symbolic link it simply treats it literally; and any number of ".."
is valid in a path.  There's no stat so it doesn't care if the path
actually exists.  I don't think we worry about "superroots" any more.

If we don't have an answer now I'd be inclined to comment out the
warning.

pws

Re: zsh 5.0.5-dev-2

[Bart Schaefer]

On Aug 14,  8:54pm, Peter Stephenson wrote:
}
} It's entirely unclear to me from looking at xsymlinks() when an empty
} xbuf would actually constitute a failure.

This occurs in the (t0 == -1) case when the reason that readlink() has
failed is because the path so far concatenated with the next segment of
the split path has produced a string longer than PATH_MAX.  In that case
(xbuflen + pplen > sizeof(xbuf)) we completely truncate xbuf and give up.

} If it can't expand something
} as a symbolic link it simply treats it literally;

Yes, but for a /path/to/somereallylongname, appending somereallylongname
to the expansion of xbuf so far, might overflow xbuf.

} If we don't have an answer now I'd be inclined to comment out the
} warning.

Maybe just make xsymlinks() have a ternary return value instead of a
boolean one, and return an actual error value in the case where we've
truncated on overflow instead of as part of normal expansion to root?

I don't know if all of these changes are necessary.  It has always
bothered me that the return value of the recursive calls to xsymlinks()
are ignored, but maybe this isn't the right way to handle them.

Incidentally, anybody recall what the obviously obsolete comment above
the xsymlinks() definition in Src/utils.c is about?

diff --git a/Src/utils.c b/Src/utils.c
index 998e46a..076a33c 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -716,7 +716,6 @@ slashsplit(char *s)
 }
 
 /* expands symlinks and .. or . expressions */
-/* if flag = 0, only expand .. and . expressions */
 
 /**/
 static int
@@ -753,6 +752,7 @@ xsymlinks(char *s)
 		strcat(xbuf, *pp);
 	    } else {
 		*xbuf = 0;
+		ret = -1;
 		break;
 	    }
 	} else {
@@ -760,9 +760,11 @@ xsymlinks(char *s)
 	    metafy(xbuf3, t0, META_NOALLOC);
 	    if (*xbuf3 == '/') {
 		strcpy(xbuf, "");
-		xsymlinks(xbuf3 + 1);
+		if (xsymlinks(xbuf3 + 1) < 0)
+		    ret = -1;
 	    } else
-		xsymlinks(xbuf3);
+		if (xsymlinks(xbuf3) < 0)
+		    ret = -1;
 	}
     }
     freearray(opp);
@@ -781,11 +783,10 @@ xsymlink(char *s)
     if (*s != '/')
 	return NULL;
     *xbuf = '\0';
-    xsymlinks(s + 1);
-    if (!*xbuf) {
+    if (xsymlinks(s + 1) < 0)
 	zwarn("path expansion failed, using root directory");
+    if (!*xbuf)
 	return ztrdup("/");
-    }
     return ztrdup(xbuf);
 }
 
@@ -795,7 +796,7 @@ print_if_link(char *s)
 {
     if (*s == '/') {
 	*xbuf = '\0';
-	if (xsymlinks(s + 1))
+	if (xsymlinks(s + 1) > 0)
 	    printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout);
     }
 }

-- 
Barton E. Schaefer

Re: zsh 5.0.5-dev-2

[Peter Stephenson]

On Thu, 14 Aug 2014 21:44:12 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Maybe just make xsymlinks() have a ternary return value instead of a
> boolean one, and return an actual error value in the case where we've
> truncated on overflow instead of as part of normal expansion to root?

Sounds reasonable for now.  Thanks.

> I don't know if all of these changes are necessary.  It has always
> bothered me that the return value of the recursive calls to xsymlinks()
> are ignored, but maybe this isn't the right way to handle them.

Yes, the way the calls a nested is a bit of a mess.

> Incidentally, anybody recall what the obviously obsolete comment above
> the xsymlinks() definition in Src/utils.c is about?

No, I noticed that but don't remember the history.

pws

Re: zsh 5.0.5-dev-2

[Han Pingtian]

Hi,

Looks like on the 747 line of Src/utils.c:

747         sprintf(xbuf2, "%s/%s", xbuf, *pp);

The "cd .." will trigger a buffer overflow if I compile zsh with 
-D FORTIFY_SOURCE=2 . Shall we return -1 here if it will overflow xbuf2?

On Thu, Aug 14, 2014 at 09:44:12PM -0700, Bart Schaefer wrote:
> On Aug 14,  8:54pm, Peter Stephenson wrote:
> }
> } It's entirely unclear to me from looking at xsymlinks() when an empty
> } xbuf would actually constitute a failure.
> 
> This occurs in the (t0 == -1) case when the reason that readlink() has
> failed is because the path so far concatenated with the next segment of
> the split path has produced a string longer than PATH_MAX.  In that case
> (xbuflen + pplen > sizeof(xbuf)) we completely truncate xbuf and give up.
> 
> } If it can't expand something
> } as a symbolic link it simply treats it literally;
> 
> Yes, but for a /path/to/somereallylongname, appending somereallylongname
> to the expansion of xbuf so far, might overflow xbuf.
> 
> } If we don't have an answer now I'd be inclined to comment out the
> } warning.
> 
> Maybe just make xsymlinks() have a ternary return value instead of a
> boolean one, and return an actual error value in the case where we've
> truncated on overflow instead of as part of normal expansion to root?
> 
> I don't know if all of these changes are necessary.  It has always
> bothered me that the return value of the recursive calls to xsymlinks()
> are ignored, but maybe this isn't the right way to handle them.
> 
> Incidentally, anybody recall what the obviously obsolete comment above
> the xsymlinks() definition in Src/utils.c is about?
> 
> diff --git a/Src/utils.c b/Src/utils.c
> index 998e46a..076a33c 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -716,7 +716,6 @@ slashsplit(char *s)
>  }
> 
>  /* expands symlinks and .. or . expressions */
> -/* if flag = 0, only expand .. and . expressions */
> 
>  /**/
>  static int
> @@ -753,6 +752,7 @@ xsymlinks(char *s)
>  		strcat(xbuf, *pp);
>  	    } else {
>  		*xbuf = 0;
> +		ret = -1;
>  		break;
>  	    }
>  	} else {
> @@ -760,9 +760,11 @@ xsymlinks(char *s)
>  	    metafy(xbuf3, t0, META_NOALLOC);
>  	    if (*xbuf3 == '/') {
>  		strcpy(xbuf, "");
> -		xsymlinks(xbuf3 + 1);
> +		if (xsymlinks(xbuf3 + 1) < 0)
> +		    ret = -1;
>  	    } else
> -		xsymlinks(xbuf3);
> +		if (xsymlinks(xbuf3) < 0)
> +		    ret = -1;
>  	}
>      }
>      freearray(opp);
> @@ -781,11 +783,10 @@ xsymlink(char *s)
>      if (*s != '/')
>  	return NULL;
>      *xbuf = '\0';
> -    xsymlinks(s + 1);
> -    if (!*xbuf) {
> +    if (xsymlinks(s + 1) < 0)
>  	zwarn("path expansion failed, using root directory");
> +    if (!*xbuf)
>  	return ztrdup("/");
> -    }
>      return ztrdup(xbuf);
>  }
> 
> @@ -795,7 +796,7 @@ print_if_link(char *s)
>  {
>      if (*s == '/') {
>  	*xbuf = '\0';
> -	if (xsymlinks(s + 1))
> +	if (xsymlinks(s + 1) > 0)
>  	    printf(" -> "), zputs(*xbuf ? xbuf : "/", stdout);
>      }
>  }
> 
> -- 
> Barton E. Schaefer

Re: zsh 5.0.5-dev-2

[Bart Schaefer]

On Aug 15,  7:23pm, Han Pingtian wrote:
} Subject: Re: zsh 5.0.5-dev-2
}
} Hi,
} 
} Looks like on the 747 line of Src/utils.c:
} 
} 747         sprintf(xbuf2, "%s/%s", xbuf, *pp);
} 
} The "cd .." will trigger a buffer overflow if I compile zsh with 
} -D FORTIFY_SOURCE=2 . Shall we return -1 here if it will overflow xbuf2?

I think Fortify errors because xbuf2 and xbuf are the same size and
the sprintf format is appending at least one character.  In practice
there would have to be a path segment PATH_MAX bytes long followed by
a file (directory) name at least PATH_MAX bytes long, which ought to
be impossible if the file system is well-behaved; in any other case
the readlink() will already have failed on the previous segment and
it already has either generated a partial expansion or returned -1.

If we're really worried about this, I think the solution would be to make
xbuf2 larger, e.g., PATH_MAX*3 or something.  Does the fortify error go
away if you increase the size of xbuf2?

Re: zsh 5.0.5-dev-2

[Han Pingtian]

On Fri, Aug 15, 2014 at 10:17:01AM -0700, Bart Schaefer wrote:
> On Aug 15,  7:23pm, Han Pingtian wrote:
> } Subject: Re: zsh 5.0.5-dev-2
> }
> } Hi,
> } 
> } Looks like on the 747 line of Src/utils.c:
> } 
> } 747         sprintf(xbuf2, "%s/%s", xbuf, *pp);
> } 
> } The "cd .." will trigger a buffer overflow if I compile zsh with 
> } -D FORTIFY_SOURCE=2 . Shall we return -1 here if it will overflow xbuf2?
> 
> I think Fortify errors because xbuf2 and xbuf are the same size and
> the sprintf format is appending at least one character.  In practice
> there would have to be a path segment PATH_MAX bytes long followed by
> a file (directory) name at least PATH_MAX bytes long, which ought to
> be impossible if the file system is well-behaved; in any other case
> the readlink() will already have failed on the previous segment and
> it already has either generated a partial expansion or returned -1.
> 
> If we're really worried about this, I think the solution would be to make
> xbuf2 larger, e.g., PATH_MAX*3 or something.  Does the fortify error go
> away if you increase the size of xbuf2?

I have tried PATH_MAX*3 and PATH_MAX*3 - 1, they both can fix the
buffer overflow here. I also tried PATH_MAX*2 + 1/2/3, they don't 
fix this problem. 

Re: zsh 5.0.5-dev-2

[Bart Schaefer]

On Aug 16,  8:35am, Han Pingtian wrote:
}
} I have tried PATH_MAX*3 and PATH_MAX*3 - 1, they both can fix the
} buffer overflow here. I also tried PATH_MAX*2 + 1/2/3, they don't 
} fix this problem. 

I suspect Fortify is reporting a potential error rather than a real
one, because we'd presumably have seen other problems before this if
"cd .." actually caused an 8kb buffer on the stack to overflow.

The whole symlink-chasing code is probably ripe to be rewritten with
zsh-heap (or even malloc + static pointers) allocation unless we have
reason to beleive that's a serious performance issue.

For the nonce I'll just commit xbuf2[PATH_MAX*3].

-- 
Barton E. Schaefer

Re: zsh 5.0.5-dev-2

[Han Pingtian]

On Sun, Aug 17, 2014 at 10:30:30AM -0700, Bart Schaefer wrote:
> On Aug 16,  8:35am, Han Pingtian wrote:
> }
> } I have tried PATH_MAX*3 and PATH_MAX*3 - 1, they both can fix the
> } buffer overflow here. I also tried PATH_MAX*2 + 1/2/3, they don't 
> } fix this problem. 
> 
> I suspect Fortify is reporting a potential error rather than a real
> one, because we'd presumably have seen other problems before this if
> "cd .." actually caused an 8kb buffer on the stack to overflow.
> 
> The whole symlink-chasing code is probably ripe to be rewritten with
> zsh-heap (or even malloc + static pointers) allocation unless we have
> reason to beleive that's a serious performance issue.
> 
> For the nonce I'll just commit xbuf2[PATH_MAX*3].

FYI.

I have tried to print the length of xbuf and *pp before the sprintf(). Looks
like when overflow being triggered, the length of xbuf is 8188, and the
length of *pp is 10. 

After changing xbuf2's length to PATH_MAX*3, the result is the same:
when zsh print 

zsh: path expansion failed, using root directory

the length of xbuf is 8188 and length of *pp is 10.

All the tests was performed on top of 2be0d8bdef401b6bca0c80a7bd78d658e862e38e,
I haven't yet merged your new commit.

Thanks.

Re: zsh 5.0.5-dev-2

[Bart Schaefer]

On Aug 18, 10:56am, Han Pingtian wrote:
} Subject: Re: zsh 5.0.5-dev-2
}
} On Sun, Aug 17, 2014 at 10:30:30AM -0700, Bart Schaefer wrote:
} > 
} > I suspect Fortify is reporting a potential error rather than a real
} > one, because we'd presumably have seen other problems before this if
} > "cd .." actually caused an 8kb buffer on the stack to overflow.
} 
} I have tried to print the length of xbuf and *pp before the sprintf().
} Looks like when overflow being triggered, the length of xbuf is 8188,
} and the length of *pp is 10.

I must not previously have been understanding exactly what you tested.

I now suspect you've deliberately constructed and (with chaselinks not
set?) cd'd one level down at a time into a path that's at least 8188
characters long, and then setopt chaselinks and done "cd .." from the
bottom directory in that path.  Is that correct?

Maybe you previously posted exactly what test you were doing and I just
lost track of it.

Anyway, if that's along the lines of what you've done, then I retract
my "potential error rather than real" remark.

Re: zsh 5.0.5-dev-2

[Han Pingtian]

On Sun, Aug 17, 2014 at 11:36:19PM -0700, Bart Schaefer wrote:
> On Aug 18, 10:56am, Han Pingtian wrote:
> } Subject: Re: zsh 5.0.5-dev-2
> }
> } On Sun, Aug 17, 2014 at 10:30:30AM -0700, Bart Schaefer wrote:
> } > 
> } > I suspect Fortify is reporting a potential error rather than a real
> } > one, because we'd presumably have seen other problems before this if
> } > "cd .." actually caused an 8kb buffer on the stack to overflow.
> } 
> } I have tried to print the length of xbuf and *pp before the sprintf().
> } Looks like when overflow being triggered, the length of xbuf is 8188,
> } and the length of *pp is 10.
> 
> I now suspect you've deliberately constructed and (with chaselinks not
> set?) cd'd one level down at a time into a path that's at least 8188
> characters long, and then setopt chaselinks and done "cd .." from the
> bottom directory in that path.  Is that correct?
> 
Sorry for the confusing. I found this problem with this command which
comes from 32274:

    % for i in `seq 1000`; do mkdir 0123456789; cd 0123456789; done; cd ..

And the filesystem is tmpfs. I don't set chaselinks, but looks like it
will be set within cd_try_chdir():

1119     if (!chasinglinks)
1120         dochaselinks = fixdir(buf);
... ...
1134     if (dochaselinks)
1135         chasinglinks = 1;

then xsymlink() will be called.

Symlink chasing

[Peter Stephenson]

On Sun, 17 Aug 2014 10:30:30 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The whole symlink-chasing code is probably ripe to be rewritten with
> zsh-heap (or even malloc + static pointers) allocation unless we have
> reason to beleive that's a serious performance issue.

I'd be very surprised if it causes a performance problem if sensibly
written.

Ideally, it would be good not to rely on PATH_MAX at all, though that's
a very long way off.

pws