make mg_clear() et al behave when RC==0

The functions S_save_magic() and S_restore_magic(), which are called by
mg_get(), mg_set(), mg_length(), mg_size() and mg_clear(),
are not robust when called with an SV whose reference count is zero.
Basically, one of the actions of S_save_magic() is to temporarily
increase the refcount of the SV, and then for S_restore_magic() to reduce
it again at the end, so that if any of the magic functions called
inbetween decrease the count, it won't be prematurely freed.
However, if the count starts at zero, then bumping it up and bringing it
back down to zero, triggers a spurious second freeing.

So, if its zero, just skip the whole bumping thing.

Now, we shouldn't really be calling these functions will a zero-refcount
SV, but these things happen, and its best to be robust.

In particular, this fixes RT #87860, which was ultimately triggered by a
bug in Set::Object 1.28 that managed to create an HV with null SvMAGIC
field, but with the RMG flag set.

When freeing that HV, sv_clear() skips doing mg_free() because SvMAGIC is
null, whereas later it calls Perl_hv_undef_flags, which calls mg_clear()
because it uses the test SvRMAGICAL(hv) (which is true).

1 files changed, 30 insertions, 16 deletions

diff --git a/mg.c b/mg.c
index 1ac7e31aba..e821415b63 100644
--- a/mg.c
+++ b/mg.c
@@ -84,6 +84,7 @@ struct magic_state {
     I32 mgs_ss_ix;
     U32 mgs_magical;
     bool mgs_readonly;
+    bool mgs_bumped;
 };
 /* MGS is typedef'ed to struct magic_state in perl.h */
 
@@ -92,12 +93,20 @@ S_save_magic(pTHX_ I32 mgs_ix, SV *sv)
 {
     dVAR;
     MGS* mgs;
+    bool bumped = FALSE;
 
     PERL_ARGS_ASSERT_SAVE_MAGIC;
 
-    /* guard against sv having being freed midway by holding a private
-       reference. */
-    SvREFCNT_inc_simple_void_NN(sv);
+    /* we shouldn't really be called here with RC==0, but it can sometimes
+     * happen via mg_clear() (which also shouldn't be called when RC==0,
+     * but it can happen). Handle this case gracefully(ish) by not RC++
+     * and thus avoiding the resultant double free */
+    if (SvREFCNT(sv) > 0) {
+    /* guard against sv getting freed midway through the mg clearing,
+     * by holding a private reference for the duration. */
+	SvREFCNT_inc_simple_void_NN(sv);
+	bumped = TRUE;
+    }
 
     assert(SvMAGICAL(sv));
     /* Turning READONLY off for a copy-on-write scalar (including shared
@@ -112,6 +121,7 @@ S_save_magic(pTHX_ I32 mgs_ix, SV *sv)
     mgs->mgs_magical = SvMAGICAL(sv);
     mgs->mgs_readonly = SvREADONLY(sv) != 0;
     mgs->mgs_ss_ix = PL_savestack_ix;   /* points after the saved destructor */
+    mgs->mgs_bumped = bumped;
 
     SvMAGICAL_off(sv);
     SvREADONLY_off(sv);
@@ -3147,6 +3157,7 @@ S_restore_magic(pTHX_ const void *p)
     dVAR;
     MGS* const mgs = SSPTR(PTR2IV(p), MGS*);
     SV* const sv = mgs->mgs_sv;
+    bool bumped;
 
     if (!sv)
         return;
@@ -3178,6 +3189,7 @@ S_restore_magic(pTHX_ const void *p)
 	}
     }
 
+    bumped = mgs->mgs_bumped;
     mgs->mgs_sv = NULL;  /* mark the MGS structure as restored */
 
     /* If we're still on top of the stack, pop us off.  (That condition
@@ -3196,21 +3208,23 @@ S_restore_magic(pTHX_ const void *p)
         assert((popval & SAVE_MASK) == SAVEt_ALLOC);
         PL_savestack_ix -= popval >> SAVE_TIGHT_SHIFT;
     }
-    if (SvREFCNT(sv) == 1) {
-	/* We hold the last reference to this SV, which implies that the
-	   SV was deleted as a side effect of the routines we called.
-	   So artificially keep it alive a bit longer.
-	   We avoid turning on the TEMP flag, which can cause the SV's
-	   buffer to get stolen (and maybe other stuff). */
-	int was_temp = SvTEMP(sv);
-	sv_2mortal(sv);
-	if (!was_temp) {
-	    SvTEMP_off(sv);
+    if (bumped) {
+	if (SvREFCNT(sv) == 1) {
+	    /* We hold the last reference to this SV, which implies that the
+	       SV was deleted as a side effect of the routines we called.
+	       So artificially keep it alive a bit longer.
+	       We avoid turning on the TEMP flag, which can cause the SV's
+	       buffer to get stolen (and maybe other stuff). */
+	    int was_temp = SvTEMP(sv);
+	    sv_2mortal(sv);
+	    if (!was_temp) {
+		SvTEMP_off(sv);
+	    }
+	    SvOK_off(sv);
 	}
-	SvOK_off(sv);
+	else
+	    SvREFCNT_dec(sv); /* undo the inc in S_save_magic() */
     }
-    else
-	SvREFCNT_dec(sv); /* undo the inc in S_save_magic() */
 }
 
 /* clean up the mess created by Perl_sighandler().