From zefram@fysh.org

Created by zefram@fysh.org

$ cat t1
use Test​::More tests => 1;
{
  package BadHash;
  sub TIEHASH { bless({}, $_[0]) }
  sub EXISTS { 0 }
  sub FETCH { undef }
  sub STORE { }
  sub DELETE { die "BadHash deleting\n" }
}
my(@​events, @​value);
@​value = eval {
  @​value = sub {
  @​value = sub {
  my %a;
  tie %a, "BadHash";
  local $a{foo} = "bar";
  die "d3";
  ("dd2a", "dd2b");
  }->();
  ("cc3a", "cc3b");
  }->();
  ("bb2a", "bb2b");
};
print $@​ ? "caught​: $@​" : "no exception\n";
ok 1;
1;
$ /opt/perl-5.20.2/bin/perl t1
1..1
Attempt to free unreferenced scalar​: SV 0x1219eb0 at t1 line 24.
caught​: BadHash deleting
zsh​: segmentation fault /opt/perl-5.20.2/bin/perl t1
$ /opt/perl-5.18.4/bin/perl t1
1..1
BadHash deleting
# Looks like your test exited with 255 before it could output anything.
$

The essence of this situation is that non-local unwinding is invoked
(via die) and during save-stack unwinding something else dies (in this
case the tied delete undoing the hash element localisation). It doesn't
have to be a tie causing the second exception; this is just the first way
I found to do it without using XS modules. An object destructor won't
do, because it runs in an extra eval scope that would catch the exception.

On pre-5.20 perls, the second exception propagates in a sane manner.
It's not actually caught by the eval, because that scope was already
popped in an earlier stage of unwinding for the first exception.
(The scope stack gets unwound to the destination before any save stack
unwinding.) If you wrap a second eval{} around the existing one, then
the outer eval{} will catch the second exception. Not great semantics,
but all the data structures stay consistent.

On 5.20, things get screwed up. The order in which things get unwound
is more logical​: the save stack gets unwound in pieces, as scopes are
popped from the scope stack. This means that the second exception
should be caught by the eval. But now an exception happening during
this piecemeal save stack unwinding causes data inconsistencies, as seen
by the "unreferenced scalar" diagnostic and the SEGV. (In my testing,
if Test​::More is omitted then the SEGV doesn't happen, which is why I've
included it in the above. But the "unreferenced scalar" still happens.)

I believe this is down to commit 2537512,
in Perl 5.19.4, which introduced this piecemeal save stack unwinding
during scope stack unwinding. Examining the code there, I don't see
exactly how it gets screwed up enough to cause what we see. But it does
seem to be relying on LEAVE_SCOPE() (which unwinds the save stack, not
the scope stack) never causing a non-local control transfer, in order
to have the necessary CvDEPTH adjustment go ahead.

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.20.2:

Configured by root at Fri Mar 20 11:06:52 UTC 2015.

Summary of my perl5 (revision 5 version 20 subversion 2) configuration:
   
  Platform:
    osname=linux, osvers=3.2.0-4-amd64, archname=x86_64-linux-ld
    uname='linux ukmcwzefram.photobox.priv 3.2.0-4-amd64 #1 smp debian 3.2.60-1+deb7u3 x86_64 gnulinux '
    config_args='-des -Duseshrplib -Duse64bitint -Duselongdouble -Uusethreads -Uusemultiplicity -Dprefix=/opt/perl-5.20.2 -Dsiteprefix=/opt/perl-5.20.2 -Dvendorprefix=/opt/perl-5.20.2/vendor -Doptimize=-ggdb -O3 -fbranch-target-load-optimize -fgcse-las -fgcse-sm -fipa-pta -floop-block -floop-interchange -floop-strip-mine -fmodulo-sched -fomit-frame-pointer -freorder-blocks-and-partition -fsched-spec-load -fsched-spec-load-dangerous -ftree-loop-distribution -Dcccdlflags=-fPIC -O3 -pipe'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=define
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-ggdb -O3 -fbranch-target-load-optimize -fgcse-las -fgcse-sm -fipa-pta -floop-block -floop-interchange -floop-strip-mine -fmodulo-sched -fomit-frame-pointer -freorder-blocks-and-partition -fsched-spec-load -fsched-spec-load-dangerous -ftree-loop-distribution',
    cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='long double', nvsize=16, Off_t='off_t', lseeksize=8
    alignbytes=16, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.7/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lnsl -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.17.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.17'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/opt/perl-5.20.2/lib/5.20.2/x86_64-linux-ld/CORE'
    cccdlflags='-fPIC -O3 -pipe', lddlflags='-shared -ggdb -O3 -fbranch-target-load-optimize -fgcse-las -fgcse-sm -fipa-pta -floop-block -floop-interchange -floop-strip-mine -fmodulo-sched -fomit-frame-pointer -freorder-blocks-and-partition -fsched-spec-load -fsched-spec-load-dangerous -ftree-loop-distribution -L/usr/local/lib -fstack-protector'



@INC for perl 5.20.2:
    /opt/perl-5.20.2/lib/site_perl/5.20.2/x86_64-linux-ld
    /opt/perl-5.20.2/lib/site_perl/5.20.2
    /opt/perl-5.20.2/vendor/lib/vendor_perl/5.20.2/x86_64-linux-ld
    /opt/perl-5.20.2/vendor/lib/vendor_perl/5.20.2
    /opt/perl-5.20.2/lib/5.20.2/x86_64-linux-ld
    /opt/perl-5.20.2/lib/5.20.2
    .


Environment for perl 5.20.2:
    HOME=/home/zefram
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/zefram/pub/x86_64-unknown-linux-gnu/bin:/home/zefram/pub/common/bin:/usr/bin:/bin:/usr/local/bin:/usr/games:/opt/babelhttpd-2.2.26/bin:/opt/babelhttpd-2.4.7/bin:/opt/geoip/bin:/opt/httpd/bin:/opt/perl/bin
    PERL_BADLANG (unset)
    SHELL=/usr/bin/zsh

From zefram@fysh.org

I suspect that the simple solution to this will be to perform the CvDEPTH
restoration via the save stack. Maybe other aspects of POPSUB() et al
could be done this way too​: popping a scope from the scope stack could
amount to no more than a LEAVE_SCOPE(). The latter would be going beyond
the scope of a pure bugfix, of course.

-zefram

From @cpansprout

This is related to #105916.

--

Father Chrysostomos

The RT System itself - Status changed from 'new' to 'open'

From @cpansprout

On Wed Mar 25 01​:09​:36 2015, zefram@​fysh.org wrote​:

I suspect that the simple solution to this will be to perform the CvDEPTH
restoration via the save stack. Maybe other aspects of POPSUB() et al
could be done this way too​: popping a scope from the scope stack could
amount to no more than a LEAVE_SCOPE(). The latter would be going beyond
the scope of a pure bugfix, of course.

Not only that, but catching longjmps that occur during leave_scope is probably necessary, too, but that would have to wait until after 5.22.

--

Father Chrysostomos

From perl@profvince.com

Le 27/03/2015 17​:40, Father Chrysostomos via RT a écrit :

On Wed Mar 25 01​:09​:36 2015, zefram@​fysh.org wrote​:

I suspect that the simple solution to this will be to perform the CvDEPTH
restoration via the save stack. Maybe other aspects of POPSUB() et al
could be done this way too​: popping a scope from the scope stack could
amount to no more than a LEAVE_SCOPE(). The latter would be going beyond
the scope of a pure bugfix, of course.

Not only that, but catching longjmps that occur during leave_scope is probably necessary, too, but that would have to wait until after 5.22.

By the way, I would like to note that commit 2537512 ("[perl #119311]
Keep CvDEPTH and savestack in sync") from perl 5.19.4 broke
Scope​::Upper's interaction with the debugger. I could work around of the
problems related to the reap() feature, but I don't think I'll be able
to solve another one related to uplevel(). In particular, calling
LEAVE_SCOPE() in dounwind through POPSUB() is really causing me trouble.
Moreover, I don't really understand why these issues only appear under
the debugger, so I'm not so confident that the module actually works
correctly even outside of it.

(The test suite does not cover the interaction with the debugger, so it
is normal that it was not catched by the CPAN smokes.)

Vincent

From @iabyn

On Fri, Mar 27, 2015 at 06​:29​:17PM -0300, Vincent Pit (VPIT) wrote​:

Le 27/03/2015 17​:40, Father Chrysostomos via RT a écrit :

On Wed Mar 25 01​:09​:36 2015, zefram@​fysh.org wrote​:

I suspect that the simple solution to this will be to perform the CvDEPTH
restoration via the save stack. Maybe other aspects of POPSUB() et al
could be done this way too​: popping a scope from the scope stack could
amount to no more than a LEAVE_SCOPE(). The latter would be going beyond
the scope of a pure bugfix, of course.

Not only that, but catching longjmps that occur during leave_scope is probably necessary, too, but that would have to wait until after 5.22.

By the way, I would like to note that commit 2537512 ("[perl #119311] Keep
CvDEPTH and savestack in sync") from perl 5.19.4 broke Scope​::Upper's
interaction with the debugger. I could work around of the problems related
to the reap() feature, but I don't think I'll be able to solve another one
related to uplevel(). In particular, calling LEAVE_SCOPE() in dounwind
through POPSUB() is really causing me trouble. Moreover, I don't really
understand why these issues only appear under the debugger, so I'm not so
confident that the module actually works correctly even outside of it.

(The test suite does not cover the interaction with the debugger, so it is
normal that it was not catched by the CPAN smokes.)

I've pushed the following for smoking​:

commit f45d6b6
Author​: David Mitchell <davem@​iabyn.com>
AuthorDate​: Fri May 8 14​:46​:01 2015 +0100
Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri May 8 15​:08​:24 2015 +0100

  RT #124156​: death during unwinding causes crash
 
  v5.19.3-139-g2537512 changed POPSUB and POPFORMAT so that they also
  unwind the relevant portion of the scope stack. This (sensible) change
  means that during exception handling, contexts and savestack frames are
  popped in lock-step, rather than all the contexts being popped followed by
  all the savestack contents.
 
  However, LEAVE_SCOPE() is now called by POPSUB/FORMAT, which can trigger
  destructors, tied method calls etc, which themselves may croak. The new
  unwinding will see the old sub context still on the context stack and call
  POPSUB on it again, leading to double frees etc.
 
  At this late stage in code freeze, the least invasive change is to
  use an unused bit in cx->blk_u16 to indicate that POPSUB has already
  been called on this context frame.
 
  Sometime later, this whole area of code really needs a thorough overhaul.
  The main issue is that if cxstack_ix-- is done too early, then calling
  destructors etc can overwrite the current context frame while we're still
  using using it; if cxstack_ix-- is done too late, then that stack frame
  can end up getting unwound twice.

--
Monto Blanco... scorchio!

From @iabyn

On Fri, May 08, 2015 at 03​:25​:05PM +0100, Dave Mitchell wrote​:

I've pushed the following for smoking​:

and now an improved version, f053056,
which sets sv to NULL in POPSUB(cx,sv) if it's called for the second time.

--
In my day, we used to edit the inodes by hand. With magnets.

From @iabyn

On Fri, May 08, 2015 at 10​:11​:47PM +0100, Dave Mitchell wrote​:

On Fri, May 08, 2015 at 03​:25​:05PM +0100, Dave Mitchell wrote​:

I've pushed the following for smoking​:

and now an improved version, f053056,
which sets sv to NULL in POPSUB(cx,sv) if it's called for the second time.

and now an even more improved version, currently smoking as
smoke-me/davem/unwind3.

The previous version skipped too much on the second POPSUBbing of a
context. The first POPSUB gets interrupted during LEAVES_SCOPE, and so
fails to do the depth restore, or to return a cv for LEAVESUB to decrement
its ref count.
On the second call to POPSUB on the same context, we should skip the stuff
before the LEAVES_SCOPE, but still do everything after it (unwind2 was
skipping both before and after).

There are still many flaws in the current implementation. For example
I suspect that something like

  sub Foo​::DESTROY { die }
  sub f {
  local *_ = bless [], 'Foo';
  }

will cause Bad Things to happen still. I plan to do a more general fix
post 5.22. In particular, I intend to save tmps_floor and comppad in the
CX struct rather than on the savestack. In addition to being faster and
using less memory, it removes a frame of data on the savestack​: currently
the savestack contains the three items pushed by pp_entersub(), followed
by any saves caused by the execution of the body of the sub. It's not
currently possible to pop the latter without popping the former too. By
removing the former, a LEAVE_SCOPE can be done by dounwind() which won't
mess with the sub context, removing the LEAVE_SCOPE() from the middle of
POPSUB, and reducing the issue of POPSUB being interrupted in the middle.

--
Modern art​:
  "That's easy, I could have done that!"
  "Ah, but you didn't!"

From @rjbs

On Mon May 11 05​:59​:41 2015, davem wrote​:

and now an even more improved version, currently smoking as
smoke-me/davem/unwind3.

Are we good to apply this? The only smoke issues I see are with George's clang 4.2.1, which shows this error on unwind3, which I did not see /lately/ on blead​:

http​://perl5.test-smoke.org/report/34399

Assuming this is unrelated and/or glitchy, I'd like to apply so that we're in shape for a Monday-ish RC1.

--
rjbs

From @khwilliamson

On 05/15/2015 03​:48 PM, Ricardo SIGNES via RT wrote​:

On Mon May 11 05​:59​:41 2015, davem wrote​:

and now an even more improved version, currently smoking as
smoke-me/davem/unwind3.

Are we good to apply this? The only smoke issues I see are with George's clang 4.2.1, which shows this error on unwind3, which I did not see /lately/ on blead​:

http​://perl5.test-smoke.org/report/34399

Assuming this is unrelated and/or glitchy, I'd like to apply so that we're in shape for a Monday-ish RC1.

commit 1956db7
  Author​: David Mitchell <davem@​iabyn.com>
  Date​: Fri May 8 14​:46​:01 2015 +0100

  RT #124156​: death during unwinding causes crash

From @rjbs

* Karl Williamson <public@​khwilliamson.com> [2015-05-15T18​:52​:49]

commit 1956db7
Author​: David Mitchell <davem@​iabyn.com>
Date​: Fri May 8 14​:46​:01 2015 +0100

Whoops, I hadn't pulled in a bit. In my defense, I wasn't *that* out of date​:

  Commit​: David Mitchell <davem@​iabyn.com>
  CommitDate​: Fri May 15 12​:31​:59 2015 +0100

:-)

--
rjbs

From @khwilliamson

On 05/15/2015 08​:07 PM, Ricardo Signes wrote​:

* Karl Williamson <public@​khwilliamson.com> [2015-05-15T18​:52​:49]

commit 1956db7
Author​: David Mitchell <davem@​iabyn.com>
Date​: Fri May 8 14​:46​:01 2015 +0100

Whoops, I hadn't pulled in a bit. In my defense, I wasn't *that* out of date​:

Commit​: David Mitchell <davem@​iabyn.com>
CommitDate​: Fri May 15 12​:31​:59 2015 +0100

:-)

I can understand your confusion, since it stayed on the blocker list;
now removed.

From @iabyn

On Mon, May 11, 2015 at 01​:59​:00PM +0100, Dave Mitchell wrote​:

There are still many flaws in the current implementation. For example
I suspect that something like

sub Foo​::DESTROY \{ die \}
sub f \{
    local \*\_ = bless \[\]\, 'Foo';
\}

will cause Bad Things to happen still. I plan to do a more general fix
post 5.22. In particular, I intend to save tmps_floor and comppad in the
CX struct rather than on the savestack. In addition to being faster and
using less memory, it removes a frame of data on the savestack​: currently
the savestack contains the three items pushed by pp_entersub(), followed
by any saves caused by the execution of the body of the sub. It's not
currently possible to pop the latter without popping the former too. By
removing the former, a LEAVE_SCOPE can be done by dounwind() which won't
mess with the sub context, removing the LEAVE_SCOPE() from the middle of
POPSUB, and reducing the issue of POPSUB being interrupted in the middle.

Now done as part of my "revamp context system" work.

--
A walk of a thousand miles begins with a single step...
then continues for another 1,999,999 or so.

@iabyn - Status changed from 'open' to 'pending release'

From @khwilliamson

Thank you for submitting this report. You have helped make Perl better.
 
With the release of Perl 5.24.0 on May 9, 2016, this and 149 other issues have been resolved.

Perl 5.24.0 may be downloaded via https://metacpan.org/release/RJBS/perl-5.24.0

@khwilliamson - Status changed from 'pending release' to 'resolved'