SegFault in perl 5.010 -5.14.1

[p5pRT]

Migrated from rt.perl.org#107440 (status was 'resolved')

Searchable as RT107440$

[p5pRT]

From 0body0@rambler.ru

perl crushed with segfault

Summary of my perl5 (revision 5 version 14 subversion 1) configuration​:

  Platform​:
  osname=linux, osvers=2.6.32-32-generic, archname=i686-linux
  uname='linux gtoly-zion 2.6.32-32-generic #62-ubuntu smp wed apr 20
21​:54​:21 utc 2011 i686 gnulinux '
  config_args='-ds -e'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=undef, usemultiplicity=undef
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
  ccversion='', gccversion='4.4.3', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t',
lseeksize=8
  alignbytes=4, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /lib/../lib /usr/lib/../lib /lib /usr/lib
/usr/lib/i486-linux-gnu
  libs=-lnsl -ldl -lm -lcrypt -lutil -lc
  perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
  libc=/lib/libc-2.11.1.so, so=so, useshrplib=false, libperl=libperl.a
  gnulibc_version='2.11.1'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector'

Characteristics of this binary (from libperl)​:
  Compile-time options​: PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP
  PERL_PRESERVE_IVUV USE_LARGE_FILES USE_PERLIO
  USE_PERL_ATOF
  Built under linux
  Compiled at Jun 24 2011 20​:19​:17
  @​INC​:
  /usr/local/lib/perl5/site_perl/5.14.1/i686-linux
  /usr/local/lib/perl5/site_perl/5.14.1
  /usr/local/lib/perl5/5.14.1/i686-linux
  /usr/local/lib/perl5/5.14.1
  .

[p5pRT]

From 0body0@rambler.ru

bug_weaken.pl

[p5pRT]

From @cpansprout

On Tue Jan 03 00​:21​:56 2012, grian wrote​:

perl crushed with segfault

Here is a simpler test case​:

{
  package A;
  sub guard{
  my $sub = shift;
  return bless { 'sub'=>$sub }, 'A';
  }
  sub DESTROY{
  $_[0]->{sub}();
  }
}

my %connections;
$connections{1} = {
  client_id => 1,
  guard => A​::guard sub { delete $connections{1} }
};
% {$connections{1}} = ();

Under a debugging build of blead, it gives​:

Assertion failed​: (array), function Perl_hfree_next_entry, file hv.c,
line 1733.
Abort trap

The hash is being freed while it is being cleared. The hash-clearing
code assumes that nothing is going to free the hash (a big assumption).

The same problem happens with arrays​:

{
  package A;
  sub guard{
  my $sub = shift;
  return bless { 'sub'=>$sub }, 'A';
  }
  sub DESTROY{
  $_[0]->{sub}();
  }
}

my %connections;
$connections{1} = [
  client_id => 1,
  guard => A​::guard sub { delete $connections{1} }
];
@​ {$connections{1}} = ();

Output​:

Attempt to free unreferenced scalar​: SV 0x826e50, Perl interpreter​:
0x800000 at - line 18.
Segmentation fault

The most naïve way to fix this would be to increase the reference count
when clearing an array or hash, if the reference count is above 0 to
begin with (that last bit is not necessary, I think, as freeing no
longer uses av_clear and hv_clear). But is that the best way?

Alternatively, since hv_clear and av_clear could check the reference
count after each element and simply return if it has gone down to zero.

What does Dave Mitchell think?

--

Father Chrysostomos

[p5pRT]

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

[p5pRT]

From @cpansprout

On Tue Jan 03 09​:56​:54 2012, sprout wrote​:

The most na�ve way to fix this

Argh!! RT suffers from the Unicode Bug.

--

Father Chrysostomos

[p5pRT]

From [Unknown Contact. See original ticket]

On Tue Jan 03 09​:56​:54 2012, sprout wrote​:

The most na�ve way to fix this

Argh!! RT suffers from the Unicode Bug.

--

Father Chrysostomos

[p5pRT]

From zefram@fysh.org

Father Chrysostomos via RT wrote​:

Alternatively, since hv_clear and av_clear could check the reference
count after each element and simply return if it has gone down to zero.

That would be an unwarranted assumption that the address of the freed
HV/AV wouldn't be reused before the check.

-zefram

[p5pRT]

From @nwc10

On Tue, Jan 03, 2012 at 09​:56​:55AM -0800, Father Chrysostomos via RT wrote​:

The most naïve way to fix this would be to increase the reference count
when clearing an array or hash, if the reference count is above 0 to
begin with (that last bit is not necessary, I think, as freeing no
longer uses av_clear and hv_clear). But is that the best way?

Alternatively, since hv_clear and av_clear could check the reference
count after each element and simply return if it has gone down to zero.

What does Dave Mitchell think?

I'm not Dave, nor do I play him on TV*, but neither of these approaches make
me particularly comfortable.

The following suggestion might also make people uncomfortable, but it strikes
me that the "atomic" way to free a hash or array is to swap out HvARRAY()
or AvARRAY() from inside it (and reset the other parts of PVHV or PVAV to be
consistent). That way, the HV or AV [at that address] immediately contains
zero elements.

Then, iterate over the guts of the HV or AV and free up the scalars it used
to contain.

Dangers of this I can see to avoid

1​: The intermediate stage holds references to the (ex) body and the SVs within.
  It's important not to fail in the case of exceptions by leaking these.

2​: Dealing (in some way) with anything that attempts to insert new elements
  into the HV or AV that one thought one had just freed.

Nicholas Clark

* and I'm replying to my e-mail backlog in a very non-deterministic order,
  but it is starting to shrink...

[p5pRT]

From @nwc10

On Tue, Jan 03, 2012 at 09​:58​:02AM -0800, Father Chrysostomos via RT wrote​:

On Tue Jan 03 09​:56​:54 2012, sprout wrote​:

The most na???ve way to fix this

Argh!! RT suffers from the Unicode Bug.

The version of RT that rt.perl.org runs definitely exhibits some bugs.

It's a couple of versions behind the current, but I'm told that upgrading it
is not trivially easy because it has some extensive customisations, that
we need for our day to day use of it.

So I don't know if it's like MySQL (fixed in the next release)
but not totally like MySQL (the "next release" that you seek is already
released)

Nicholas Clark

[p5pRT]

From @cpansprout

On Tue Jan 03 10​:48​:40 2012, nicholas wrote​:

On Tue, Jan 03, 2012 at 09​:56​:55AM -0800, Father Chrysostomos via RT
wrote​:

The most na�ve way to fix this would be to increase the reference
count
when clearing an array or hash, if the reference count is above 0 to
begin with (that last bit is not necessary, I think, as freeing no
longer uses av_clear and hv_clear). But is that the best way?

Alternatively, since hv_clear and av_clear could check the reference
count after each element and simply return if it has gone down to
zero.

What does Dave Mitchell think?

I'm not Dave, nor do I play him on TV*, but neither of these
approaches make
me particularly comfortable.

The following suggestion might also make people uncomfortable, but it
strikes
me that the "atomic" way to free a hash or array is to swap out
HvARRAY()
or AvARRAY() from inside it (and reset the other parts of PVHV or PVAV
to be
consistent). That way, the HV or AV [at that address] immediately
contains
zero elements.

Yes, this has been bothering me for some time now. The fact that
destructors can see the HV or AV being emptied seems like an
implementation detail leaking through.

Then, iterate over the guts of the HV or AV and free up the scalars it
used
to contain.

Dangers of this I can see to avoid

1​: The intermediate stage holds references to the (ex) body and the
SVs within.
It's important not to fail in the case of exceptions by leaking
these.

2​: Dealing (in some way) with anything that attempts to insert new
elements
into the HV or AV that one thought one had just freed.

Well, if we make it appear atomic, then logically the freeing of the
inner scalars comes *after* the clearing of the outer aggregate, so new
elements added can just be left there.

For arrays, the solution might be as simple as​:

  ENTER;
  SAVETMPS;
  ... memcpy the AvARRAY to the tmps stack ...
  LEAVE;

--

Father Chrysostomos

[p5pRT]

From @cpansprout

On Tue Jan 03 12​:39​:54 2012, sprout wrote​:

On Tue Jan 03 10​:48​:40 2012, nicholas wrote​:

On Tue, Jan 03, 2012 at 09​:56​:55AM -0800, Father Chrysostomos via RT
wrote​:

The most na�ve way to fix this would be to increase the reference
count
when clearing an array or hash, if the reference count is above 0 to
begin with (that last bit is not necessary, I think, as freeing no
longer uses av_clear and hv_clear). But is that the best way?

Alternatively, since hv_clear and av_clear could check the reference
count after each element and simply return if it has gone down to
zero.

What does Dave Mitchell think?

I'm not Dave, nor do I play him on TV*, but neither of these
approaches make
me particularly comfortable.

The following suggestion might also make people uncomfortable, but it
strikes
me that the "atomic" way to free a hash or array is to swap out
HvARRAY()
or AvARRAY() from inside it (and reset the other parts of PVHV or PVAV
to be
consistent). That way, the HV or AV [at that address] immediately
contains
zero elements.

Yes, this has been bothering me for some time now. The fact that
destructors can see the HV or AV being emptied seems like an
implementation detail leaking through.

Then, iterate over the guts of the HV or AV and free up the scalars it
used
to contain.

Dangers of this I can see to avoid

1​: The intermediate stage holds references to the (ex) body and the
SVs within.
It's important not to fail in the case of exceptions by leaking
these.

2​: Dealing (in some way) with anything that attempts to insert new
elements
into the HV or AV that one thought one had just freed.

Well, if we make it appear atomic, then logically the freeing of the
inner scalars comes *after* the clearing of the outer aggregate, so new
elements added can just be left there.

For arrays, the solution might be as simple as​:

ENTER;
SAVETMPS;
\.\.\. memcpy the AvARRAY to the tmps stack \.\.\.
LEAVE;

(and free AvARRAY of course)

--

Father Chrysostomos

[p5pRT]

From @iabyn

On Tue, Jan 03, 2012 at 12​:42​:51PM -0800, Father Chrysostomos via RT wrote​:
[various discussions about AV/HV clearing ]

Just for those playing along at home, the example can be reduced to

  sub A​::DESTROY{ $ra = 0 }
  $ra = [ bless [], 'A' ];
  undef @​$ra;

and it concerns the code that empties an array or hash (av_undef,
av_clear, hv_undef, hv_clear), rather than the code that frees an array or
hash (now in sv_clear).

The main options seem to be​:

1 to temporarily increase the refcnt of the array/hash being emptied;
2 to replace the AvARRAY/HvARRAY with a new empty one then free the stuff
  in the old one;
3. to push all the elements onto the tmps stack

I like (1). It's simple to implement, and its only cost is that
the various clear/undef fns will need an ENTER/SAVEFREESV/LEAVE or
similar, to avoid leak on premature scope exit.

(2) I don't like, in that we used to have something similar for HVs
(admittedly for freeing rather than clearing) and it soon got complicated
and messy and buggy. Most of that was due to the HvAUX struct being part
of the HvARRAY alloc, and the difficulties with new elements being added
by destructors. That last one shouldn't bother us this time, as we're not
freeing. And we have the complicartions of making sure that the old
[AH]vARRAY and contents don't leak on premature scope exit.

(3) I don't like, as it means to clear a large array we have to
unnecessarily grow a huge tmps stack. Also, I think it will be messy
trying to do something similar with hashes.

--
Nothing ventured, nothing lost.

[p5pRT]

From @cpansprout

On Wed Jan 04 09​:15​:14 2012, davem wrote​:

On Tue, Jan 03, 2012 at 12​:42​:51PM -0800, Father Chrysostomos via RT
wrote​:
[various discussions about AV/HV clearing ]

Just for those playing along at home, the example can be reduced to

sub A​::DESTROY\{ $ra = 0 \}
$ra = \[ bless \[\]\, 'A' \];
undef @​$ra;

and it concerns the code that empties an array or hash (av_undef,
av_clear, hv_undef, hv_clear), rather than the code that frees an array or
hash (now in sv_clear).

The main options seem to be​:

1 to temporarily increase the refcnt of the array/hash being emptied;
2 to replace the AvARRAY/HvARRAY with a new empty one then free the stuff
in the old one;
3. to push all the elements onto the tmps stack

I like (1). It's simple to implement, and its only cost is that
the various clear/undef fns will need an ENTER/SAVEFREESV/LEAVE or
similar, to avoid leak on premature scope exit.

For hashes, yes. For undef @​$ra, yes. For @​$ra = (), itâ��s a little
more complicated. pp_aassign contains this​:

  case SVt_PVAV​:
  ary = MUTABLE_AV(sv);
  magic = SvMAGICAL(ary) != 0;
  av_clear(ary);
  av_extend(ary, lastrelem - relem);
  i = 0;

So, if av_clear has an ENTER/LEAVE pair, the array will be freed by the
time av_extend happens. For @​$ra = () itâ��s probably as simple as
skipping the extend since lastrelem-relem == 0.

But what should @​$ra = ('a'..'z') do in that case?

What about %$ra = ('a'..'z'), which crashes even with my local fix for
%$ra=()?

sub A​::DESTROY { $​::ra = 0 }
$​::ra = {a=>bless [], 'A'};
%$​::ra = ('a'..'z');
__END__
Assertion failed​: (SvTYPE(hv) == SVt_PVHV), function Perl_hv_common,
file hv.c, line 353.
Abort trap

How about making the stash refcounted? :-) If we did that, it would fix
the OP�s bug.

Is adding ENTER/LEAVE worthwhile, considering it is just a variation of
a larger bug, that would require a reference-counted stack to fix? This
is starting to feel a bit like plugging a tiny whole for little gain
(and overall slowdown).

This is the twenty-third bug report in this category.

Attached is my unfinished patch, since this is a convenient place to put
what I�ve done so far. It certainly cannot be applied as it is.

--

Father Chrysostomos

[p5pRT]

From @cpansprout

Inline Patch

diff --git a/av.c b/av.c
index 733bbd4..88f796a 100644
--- a/av.c
+++ b/av.c
@@ -431,6 +431,7 @@ Perl_av_clear(pTHX_ register AV *av)
 {
     dVAR;
     I32 extra;
+    bool real;
 
     PERL_ARGS_ASSERT_AV_CLEAR;
     assert(SvTYPE(av) == SVt_PVAV);
@@ -456,9 +457,11 @@ Perl_av_clear(pTHX_ register AV *av)
     if (AvMAX(av) < 0)
 	return;
 
-    if (AvREAL(av)) {
+    if ((real = !!AvREAL(av))) {
 	SV** const ary = AvARRAY(av);
 	I32 index = AvFILLp(av) + 1;
+	ENTER;
+	SAVEFREESV(SvREFCNT_inc_simple_NN(av));
 	while (index) {
 	    SV * const sv = ary[--index];
 	    /* undef the slot before freeing the value, because a
@@ -473,7 +476,7 @@ Perl_av_clear(pTHX_ register AV *av)
 	AvARRAY(av) = AvALLOC(av);
     }
     AvFILLp(av) = -1;
-
+    if (real) LEAVE;
 }
 
 /*
@@ -487,6 +490,8 @@ Undefines the array.  Frees the memory used by the array itself.
 void
 Perl_av_undef(pTHX_ register AV *av)
 {
+    bool real;
+
     PERL_ARGS_ASSERT_AV_UNDEF;
     assert(SvTYPE(av) == SVt_PVAV);
 
@@ -494,8 +499,10 @@ Perl_av_undef(pTHX_ register AV *av)
     if (SvTIED_mg((const SV *)av, PERL_MAGIC_tied)) 
 	av_fill(av, -1);
 
-    if (AvREAL(av)) {
+    if ((real = !!AvREAL(av))) {
 	register I32 key = AvFILLp(av) + 1;
+	ENTER;
+	SAVEFREESV(SvREFCNT_inc_simple_NN(av));
 	while (key)
 	    SvREFCNT_dec(AvARRAY(av)[--key]);
     }
@@ -506,6 +513,7 @@ Perl_av_undef(pTHX_ register AV *av)
     AvMAX(av) = AvFILLp(av) = -1;
 
     if(SvRMAGICAL(av)) mg_clear(MUTABLE_SV(av));
+    if(real) LEAVE;
 }
 
 /*
diff --git a/hv.c b/hv.c
index 9c5670b..011a23c 100644
--- a/hv.c
+++ b/hv.c
@@ -1681,12 +1681,20 @@ S_hfreeentries(pTHX_ HV *hv)
     STRLEN index = 0;
     XPVHV * const xhv = (XPVHV*)SvANY(hv);
     SV *sv;
+    const bool save = !!SvREFCNT(hv);
 
     PERL_ARGS_ASSERT_HFREEENTRIES;
 
+    if (save) {
+	ENTER;
+	SAVEFREESV(SvREFCNT_inc_simple_NN(hv));
+    }
+
     while ((sv = Perl_hfree_next_entry(aTHX_ hv, &index))||xhv->xhv_keys) {
 	SvREFCNT_dec(sv);
     }
+
+    if (save) LEAVE;
 }
 
 
diff --git a/t/op/array.t b/t/op/array.t
index b53da80..1ae2b2a 100644
--- a/t/op/array.t
+++ b/t/op/array.t
@@ -3,11 +3,10 @@
 BEGIN {
     chdir 't' if -d 't';
     @INC = ('.', '../lib');
+    require 'test.pl';
 }
 
-require 'test.pl';
-
-plan (123);
+plan (125);
 
 #
 # @foo, @bar, and @ary are also used from tie-stdarray after tie-ing them
@@ -441,4 +440,13 @@ sub test_arylen {
 *trit = *scile;  $trit[0];
 ok(1, 'aelem_fast on a nonexistent array does not crash');
 
+# [perl #107440]
+sub A::DESTROY { $::ra = 0 }
+$::ra = [ bless [], 'A' ];
+undef @$::ra;
+pass 'no crash when freeing array that is being undeffed';
+$::ra = [ bless [], 'A' ];
+@$::ra = ();
+pass 'no crash when freeing array that is being cleared';
+
 "We're included by lib/Tie/Array/std.t so we need to return something true";
diff --git a/t/op/hash.t b/t/op/hash.t
index 1e8c6b2..3d4c5e7 100644
--- a/t/op/hash.t
+++ b/t/op/hash.t
@@ -8,7 +8,7 @@ BEGIN {
 
 use strict;
 
-plan tests => 13;
+plan tests => 15;
 
 my %h;
 
@@ -208,3 +208,12 @@ SKIP: {
     }
     is $ref, undef, 'weak refs to pad hashes go stale on scope exit';
 }
+
+# [perl #107440]
+sub A::DESTROY { $::ra = 0 }
+$::ra = {a=>bless [], 'A'};
+undef %$::ra;
+pass 'no crash when freeing hash that is being undeffed';
+$::ra = {a=>bless [], 'A'};
+%$::ra = ();
+pass 'no crash when freeing hash that is being exonerated, ahem, cleared';

[p5pRT]

From 0body0@rambler.ru

A little question about first solution​:
ENTER/LEAVE is really needed in a sequence

ENTER/SAVEFREESV/LEAVE,

may be SAVEFREESV is enough?

04.01.2012 21​:15, Dave Mitchell via RT пиÑ�еÑ�​:

On Tue, Jan 03, 2012 at 12​:42​:51PM -0800, Father Chrysostomos via RT wrote​:
[various discussions about AV/HV clearing ]

Just for those playing along at home, the example can be reduced to

 sub A​::DESTROY\{ $ra = 0 \}
 $ra = \[ bless \[\]\, 'A' \];
 undef @​$ra;

and it concerns the code that empties an array or hash (av_undef,
av_clear, hv_undef, hv_clear), rather than the code that frees an array or
hash (now in sv_clear).

The main options seem to be​:

1 to temporarily increase the refcnt of the array/hash being emptied;
2 to replace the AvARRAY/HvARRAY with a new empty one then free the stuff
in the old one;
3. to push all the elements onto the tmps stack

I like (1). It's simple to implement, and its only cost is that
the various clear/undef fns will need an ENTER/SAVEFREESV/LEAVE or
similar, to avoid leak on premature scope exit.

(2) I don't like, in that we used to have something similar for HVs
(admittedly for freeing rather than clearing) and it soon got complicated
and messy and buggy. Most of that was due to the HvAUX struct being part
of the HvARRAY alloc, and the difficulties with new elements being added
by destructors. That last one shouldn't bother us this time, as we're not
freeing. And we have the complicartions of making sure that the old
[AH]vARRAY and contents don't leak on premature scope exit.

(3) I don't like, as it means to clear a large array we have to
unnecessarily grow a huge tmps stack. Also, I think it will be messy
trying to do something similar with hashes.

[p5pRT]

From 0body0@rambler.ru

You all are interesting people ... The problem was huge in my eyes.

I think first case (1) is the best of all.

It give Perl a little time to work with SV, before destroying it.

Anatoliy.

04.01.2012 21​:15, Dave Mitchell via RT пиÑ�еÑ�​:

On Tue, Jan 03, 2012 at 12​:42​:51PM -0800, Father Chrysostomos via RT wrote​:
[various discussions about AV/HV clearing ]

Just for those playing along at home, the example can be reduced to

 sub A​::DESTROY\{ $ra = 0 \}
 $ra = \[ bless \[\]\, 'A' \];
 undef @​$ra;

and it concerns the code that empties an array or hash (av_undef,
av_clear, hv_undef, hv_clear), rather than the code that frees an array or
hash (now in sv_clear).

The main options seem to be​:

1 to temporarily increase the refcnt of the array/hash being emptied;
2 to replace the AvARRAY/HvARRAY with a new empty one then free the stuff
in the old one;
3. to push all the elements onto the tmps stack

I like (1). It's simple to implement, and its only cost is that
the various clear/undef fns will need an ENTER/SAVEFREESV/LEAVE or
similar, to avoid leak on premature scope exit.

(2) I don't like, in that we used to have something similar for HVs
(admittedly for freeing rather than clearing) and it soon got complicated
and messy and buggy. Most of that was due to the HvAUX struct being part
of the HvARRAY alloc, and the difficulties with new elements being added
by destructors. That last one shouldn't bother us this time, as we're not
freeing. And we have the complicartions of making sure that the old
[AH]vARRAY and contents don't leak on premature scope exit.

(3) I don't like, as it means to clear a large array we have to
unnecessarily grow a huge tmps stack. Also, I think it will be messy
trying to do something similar with hashes.

[p5pRT]

From 0body0@rambler.ru

05.01.2012 10​:30, Father Chrysostomos via RT пиÑ�еÑ�​:

On Wed Jan 04 09​:15​:14 2012, davem wrote​:

On Tue, Jan 03, 2012 at 12​:42​:51PM -0800, Father Chrysostomos via RT
wrote​:
[various discussions about AV/HV clearing ]

Just for those playing along at home, the example can be reduced to

 sub A​::DESTROY\{ $ra = 0 \}
 $ra = \[ bless \[\]\, 'A' \];
 undef @​$ra;

and it concerns the code that empties an array or hash (av_undef,
av_clear, hv_undef, hv_clear), rather than the code that frees an array or
hash (now in sv_clear).

The main options seem to be​:

1 to temporarily increase the refcnt of the array/hash being emptied;
2 to replace the AvARRAY/HvARRAY with a new empty one then free the stuff
in the old one;
3. to push all the elements onto the tmps stack

I like (1). It's simple to implement, and its only cost is that
the various clear/undef fns will need an ENTER/SAVEFREESV/LEAVE or
similar, to avoid leak on premature scope exit.
For hashes, yes. For undef @​$ra, yes. For @​$ra = (), itâ��s a little
more complicated. pp_aassign contains this​:

case SVt\_PVAV​:
    ary = MUTABLE\_AV\(sv\);
    magic = SvMAGICAL\(ary\) \!= 0;
    av\_clear\(ary\);
    av\_extend\(ary\, lastrelem \- relem\);
    i = 0;

So, if av_clear has an ENTER/LEAVE pair, the array will be freed by the
time av_extend happens. For @​$ra = () itâ��s probably as simple as
skipping the extend since lastrelem-relem == 0.

But what should @​$ra = ('a'..'z') do in that case?

What about %$ra = ('a'..'z'), which crashes even with my local fix for
%$ra=()?

sub A​::DESTROY { $​::ra = 0 }
$​::ra = {a=>bless [], 'A'};
%$​::ra = ('a'..'z');
__END__
Assertion failed​: (SvTYPE(hv) == SVt_PVHV), function Perl_hv_common,
file hv.c, line 353.
Abort trap

How about making the stash refcounted? :-) If we did that, it would fix
the OP�s bug.

Is adding ENTER/LEAVE worthwhile, considering it is just a variation of
a larger bug, that would require a reference-counted stack to fix? This
is starting to feel a bit like plugging a tiny whole for little gain
(and overall slowdown).

This is the twenty-third bug report in this category.

Attached is my unfinished patch, since this is a convenient place to put
what I�ve done so far. It certainly cannot be applied as it is.

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

[p5pRT]

From @cpansprout

On Thu Jan 05 01​:19​:36 2012, grian wrote​:

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

av_clear, etc., are not necessarily called directly from a perl
run-loop. So the AV or HV could end up lasting much longer than intended.

That would be bad if it happens with %^H.

I�m wondering now whether we need ENTER/SAVEFREESV/LEAVE inside
pp_aassign as well.

--

Father Chrysostomos

[p5pRT]

From @cpansprout

On Thu Jan 05 06​:21​:04 2012, sprout wrote​:

On Thu Jan 05 01​:19​:36 2012, grian wrote​:

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

av_clear, etc., are not necessarily called directly from a perl
run-loop. So the AV or HV could end up lasting much longer than intended.

That would be bad if it happens with %^H.

I�m wondering now whether we need ENTER/SAVEFREESV/LEAVE inside
pp_aassign as well.

Actually, the simplest solution seem to be to put the av or hv on the
mortals stack in pp_aassign and pp_undef, rather than in [ah]v_undef/clear.

In those cases we are directly inside a runloop, so it will be freed soon.

My concern with %^H had to do with arbitrary code elsewhere making %^H
last for an entire compilation unit, instead of being freed when the
current compilation scope is exited.

I�ve made this change with commit 9f71cfe.

--

Father Chrysostomos

[p5pRT]

@cpansprout - Status changed from 'open' to 'resolved'

[p5pRT]

From johngrahamalvord@gmail.com

On Friday, January 6, 2012, Father Chrysostomos via RT <
perlbug-followup@​perl.org> wrote​:

On Thu Jan 05 06​:21​:04 2012, sprout wrote​:

On Thu Jan 05 01​:19​:36 2012, grian wrote​:

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

av_clear, etc., are not necessarily called directly from a perl
run-loop. So the AV or HV could end up lasting much longer than
intended.

That would be bad if it happens with %^H.

I�m wondering now whether we need ENTER/SAVEFREESV/LEAVE inside
pp_aassign as well.

Actually, the simplest solution seem to be to put the av or hv on the
mortals stack in pp_aassign and pp_undef, rather than in
[ah]v_undef/clear.

In those cases we are directly inside a runloop, so it will be freed soon.

My concern with %^H had to do with arbitrary code elsewhere making %^H
last for an entire compilation unit, instead of being freed when the
current compilation scope is exited.

I�ve made this change with commit 9f71cfe.

--

Father Chrysostomos

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=107440

[p5pRT]

From @iabyn

On Fri, Jan 06, 2012 at 05​:22​:31PM -0800, Father Chrysostomos via RT wrote​:

On Thu Jan 05 06​:21​:04 2012, sprout wrote​:

On Thu Jan 05 01​:19​:36 2012, grian wrote​:

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

av_clear, etc., are not necessarily called directly from a perl
run-loop. So the AV or HV could end up lasting much longer than intended.

That would be bad if it happens with %^H.

I�m wondering now whether we need ENTER/SAVEFREESV/LEAVE inside
pp_aassign as well.

Actually, the simplest solution seem to be to put the av or hv on the
mortals stack in pp_aassign and pp_undef, rather than in [ah]v_undef/clear.

This makes me nervous. The tmps stack is typically cleared only on
statement boundaries, so we run the risks of

  * user-visible delaying of freeing elements;
  * large tmps stack growth might be possible with
  certain types of loop that repeatedly assign to an array without
  freeing tmps (eg map? I think I fixed most map/grep tmps leakage a
  while back, but there may still be some edge cases).

Surely an ENTER/SAVEFREESV/LEAVE inside pp_aassign is just as efficient,
without any attendant risks?

Also, although pp_aassign and pp_undef are now fixed, the
[ah]v_undef/clear functions aren't, and they're part of the public API
that can be called independently of pp_aassign etc. Ideally they should
be fixed (so they don't crash in mid-loop), and their documentation
updated to point out that on return, their AV/HV arg may have been freed.

--
No matter how many dust sheets you use, you will get paint on the carpet.

[p5pRT]

From @cpansprout

On Sun Jan 08 14​:40​:49 2012, davem wrote​:

On Fri, Jan 06, 2012 at 05​:22​:31PM -0800, Father Chrysostomos via RT
wrote​:

On Thu Jan 05 06​:21​:04 2012, sprout wrote​:

On Thu Jan 05 01​:19​:36 2012, grian wrote​:

What if we simply delay freeing AV/HV until OP(nextstate) ?
if will prevent many bad things?

av_clear, etc., are not necessarily called directly from a perl
run-loop. So the AV or HV could end up lasting much longer than
intended.

That would be bad if it happens with %^H.

I�m wondering now whether we need ENTER/SAVEFREESV/LEAVE inside
pp_aassign as well.

Actually, the simplest solution seem to be to put the av or hv on
the
mortals stack in pp_aassign and pp_undef, rather than in
[ah]v_undef/clear.

This makes me nervous. The tmps stack is typically cleared only on
statement boundaries, so we run the risks of

\* user\-visible delaying of freeing elements;
\* large tmps stack growth might be possible with
  certain types of loop that repeatedly assign to an array without
  freeing tmps \(eg map? I think I fixed most map/grep tmps leakage

a
while back, but there may still be some edge cases).

Surely an ENTER/SAVEFREESV/LEAVE inside pp_aassign is just as
efficient,
without any attendant risks?

Also, although pp_aassign and pp_undef are now fixed, the
[ah]v_undef/clear functions aren't, and they're part of the public API
that can be called independently of pp_aassign etc. Ideally they
should
be fixed (so they don't crash in mid-loop), and their documentation
updated to point out that on return, their AV/HV arg may have been
freed.

I�ve made the changes with commits 60edcf0 and 8b9a115.

--

Father Chrysostomos