Better fix for perl #107440

> > 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.

This commit takes care of the first part; it changes pp_aassign to use
ENTER/SAVEFREESV/LEAVE and adds the same to h_freeentries (called both
by hv_undef and hv_clear), av_undef and av_clear.

It effectively reverts the C code part of 9f71cfe6ef2.

diff --git a/av.c b/av.c
index 1671f16..472600b 100644 (file)
--- a/av.c
+++ b/av.c
@@ -437,6 +437,7 @@ Perl_av_clear(pTHX_ register AV *av)
 {
     dVAR;
     I32 extra;
+    bool real;
 
     PERL_ARGS_ASSERT_AV_CLEAR;
     assert(SvTYPE(av) == SVt_PVAV);
@@ -462,9 +463,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
@@ -479,7 +482,7 @@ Perl_av_clear(pTHX_ register AV *av)
        AvARRAY(av) = AvALLOC(av);
     }
     AvFILLp(av) = -1;
-
+    if (real) LEAVE;
 }
 
 /*
@@ -493,6 +496,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);
 
@@ -500,8 +505,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]);
     }
@@ -512,6 +519,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 af41de8..2cfe25b 100644 (file)
--- 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/pp.c b/pp.c
index 5910e86..eaf6a85 100644 (file)
--- a/pp.c
+++ b/pp.c
@@ -970,10 +970,10 @@ PP(pp_undef)
     case SVt_NULL:
        break;
     case SVt_PVAV:
-       av_undef(MUTABLE_AV(sv_2mortal(SvREFCNT_inc_simple_NN(sv))));
+       av_undef(MUTABLE_AV(sv));
        break;
     case SVt_PVHV:
-       hv_undef(MUTABLE_HV(sv_2mortal(SvREFCNT_inc_simple_NN(sv))));
+       hv_undef(MUTABLE_HV(sv));
        break;
     case SVt_PVCV:
        if (cv_const_sv((const CV *)sv))

diff --git a/pp_hot.c b/pp_hot.c
index add9400..ff834a9 100644 (file)
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -993,8 +993,10 @@ PP(pp_aassign)
        sv = *lelem++;
        switch (SvTYPE(sv)) {
        case SVt_PVAV:
-           ary = MUTABLE_AV(sv_2mortal(SvREFCNT_inc_simple_NN(sv)));
+           ary = MUTABLE_AV(sv);
            magic = SvMAGICAL(ary) != 0;
+           ENTER;
+           SAVEFREESV(SvREFCNT_inc_simple_NN(sv));
            av_clear(ary);
            av_extend(ary, lastrelem - relem);
            i = 0;
@@ -1015,13 +1017,16 @@ PP(pp_aassign)
            }
            if (PL_delaymagic & DM_ARRAY_ISA)
                SvSETMAGIC(MUTABLE_SV(ary));
+           LEAVE;
            break;
        case SVt_PVHV: {                                /* normal hash */
                SV *tmpstr;
                SV** topelem = relem;
 
-               hash = MUTABLE_HV(sv_2mortal(SvREFCNT_inc_simple_NN(sv)));
+               hash = MUTABLE_HV(sv);
                magic = SvMAGICAL(hash) != 0;
+               ENTER;
+               SAVEFREESV(SvREFCNT_inc_simple_NN(sv));
                hv_clear(hash);
                firsthashrelem = relem;
 
@@ -1058,6 +1063,7 @@ PP(pp_aassign)
                    do_oddball(hash, relem, firstrelem);
                    relem++;
                }
+               LEAVE;
            }
            break;
        default: