make POP_SAVEARRAY() safe

POP_SAVEARRAY() frees the current @_ and restores it to the callers value.
As currently defined, it frees the old AV then updates GvAV(PL_defgv). If
the free old the old AV triggers a destructor for example, then in
theory something bad could happen.

I couldn't actually find a simple failure case, but I suspect that
something like

    sub f { *_ = bless [], 'Foo' }

where Foo::DESTROY is an XS sub that messes with GvAV(PL_defgv), could do
it.

Anyway to play safe, this commit updates the GvAV() slot and *then* frees
the old AV. This our normal modus operandi in other places anyway.

The equivalent code in pp_goto already does this the safe way. This commit
also updates pp_goto to use the (now equivalent) POP_SAVEARRAY() macro.

diff --git a/cop.h b/cop.h
index 12ff11e..04e4d76 100644 (file)
--- a/cop.h
+++ b/cop.h
@@ -624,10 +624,12 @@ struct block_format {
        CvDEPTH(cv)++;                                                  \
        SvREFCNT_inc_void(cx->blk_format.dfoutgv)
 
+/* Restore old @_ */
 #define POP_SAVEARRAY()                                                \
     STMT_START {                                                       \
-       SvREFCNT_dec(GvAV(PL_defgv));                                   \
+        AV *av = GvAV(PL_defgv);                                        \
        GvAV(PL_defgv) = cx->blk_sub.savearray;                         \
+        SvREFCNT_dec(av);                                              \
     } STMT_END
 
 /* junk in @_ spells trouble when cloning CVs and in pp_caller(), so don't

diff --git a/pp_ctl.c b/pp_ctl.c
index f43824e..9a8cc39 100644 (file)
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -2822,9 +2822,7 @@ PP(pp_goto)
                SvREFCNT_dec(arg);
                if (CxTYPE(cx) == CXt_SUB && CxHASARGS(cx)) {
                    /* Restore old @_ */
-                   arg = GvAV(PL_defgv);
-                   GvAV(PL_defgv) = cx->blk_sub.savearray;
-                   SvREFCNT_dec(arg);
+                    POP_SAVEARRAY();
                }
 
                retop = cx->blk_sub.retop;