This is a live mirror of the Perl 5 development currently hosted at https://github.com/perl/perl5
Fix CvOUTSIDE assert/refcnt bugs with sub redefinition
authorFather Chrysostomos <sprout@cpan.org>
Fri, 27 Jul 2012 17:30:58 +0000 (10:30 -0700)
committerFather Chrysostomos <sprout@cpan.org>
Fri, 27 Jul 2012 17:30:58 +0000 (10:30 -0700)
commite52de15a297bcbe6fac509aef6b35095923fa7d0
tree83dab6e756d27ede1ccc63c1c231efe137253958
parentf6894bc8d44272e8edc3e1c3719989f1b171de3f
Fix CvOUTSIDE assert/refcnt bugs with sub redefinition

my $sub = sub { 4 };
*foo = $sub;
*bar = *foo;
undef &$sub;
eval "sub bar { 3 }";
undef *foo;
undef *bar;

As of 5.8.4, this script produces:

Attempt to free unreferenced scalar: SV 0x8002c4.

As of 5.14.0:

panic: del_backref.

Or, undef debugging builds:

Assertion failed: (!CvWEAKOUTSIDE(cv)), function Perl_newATTRSUB_flags, file op.c, line 7045.

Commit 5c41a5fa918 (backported to 5.8.4 in commit 7a565e5d) caused the
first bug:

commit 5c41a5fa918d32924e1ac2f02418d5d7f465ef26
Author: Dave Mitchell <davem@fdisolutions.com>
Date:   Sun Jan 25 02:04:23 2004 +0000

    Remove small memory leak in newATTRSUB that manifested as a
    leaking scalar after the interpeter was cloned

    p4raw-id: //depot/perl@22209

diff --git a/op.c b/op.c
index b902fed..5fd21bf 100644
--- a/op.c
+++ b/op.c
@@ -4165,6 +4165,8 @@ Perl_newATTRSUB(pTHX_ I32 floor, OP *o, OP *proto, OP *attrs, OP *block)
  /* transfer PL_compcv to cv */
  cv_undef(cv);
  CvFLAGS(cv) = CvFLAGS(PL_compcv);
+ if (!CvWEAKOUTSIDE(cv))
+     SvREFCNT_dec(CvOUTSIDE(cv));
  CvOUTSIDE(cv) = CvOUTSIDE(PL_compcv);
  CvOUTSIDE_SEQ(cv) = CvOUTSIDE_SEQ(PL_compcv);
  CvOUTSIDE(PL_compcv) = 0;

Checking the flags right after clobbering them can’t be a good idea.

Commit 437388a93 caused the panics and assertion failures.  See com-
mit f6894bc for detail.

Commit f6894bc fixed the panics and assertion failures involving CvGV.

One remaining assertion (!CvWEAKOUTSIDE) added by 437388a93 is still
incorrect.  It’s not true that CvWEAKOUTSIDE is never set on a re-
used stub.

In both cases (5c41a5fa’s code and 437388a93’s code), the weakness
of CvOUTSIDE is ignored and the outside sub (the eval) is freed
prematurely.

It could be that this type of redefinition should be disallowed (des-
pite its usefulness), but that is a separate issue.  This used to
work.  And pure-Perl code should not be triggering assertion failures
or freeing scalars twice.
op.c
t/op/sub.t