Re: Occasional assertion failure in S_sv_del_backref

[Dave Mitchell <davem@iabyn.com>]

On Sat, Aug 23, 2008 at 12:00:11AM +0100, Dave Mitchell wrote:
> On Fri, Aug 22, 2008 at 03:04:56PM +0100, Dave Mitchell wrote:
> > On Fri, Aug 22, 2008 at 11:26:28AM +0100, Nicholas Clark wrote:
> > > Do you need PERL_DESTRUCT_LEVEL set to 2 in the environment?
> > > 
> > > > What's your perl's build?
> > 
> > Hmm, even with your Configure args and PERL_DESTRUCT_LEVEL set, I can't
> > get it to fail :-( This is at change 34216.
> 
> I've managed to reproduce it now (it needed -MTestInit) and I now know
> what the issue is. It's really a more general problem related to global
> destruction and dangling SV pointers, rather than specifically backrefs.
> 
> During global destruction, after the root set's been freed, we scan the
> SV arenas, and for each SV set SVf_BREAK then artificially decrement the
> refcount by 1 (which may or may not trigger that SV and its dependents
> being freed). This is all well and good, except that some other SV may
> still have a pointer to the just freed SV. Which usually isn't too bad,
> except that the freed SV might get reallocated, then the parent SV now has
> a pointer to a completely unrelated SV, and bad things may happen.
> 
> I have a suggested fix - when we free an SV, if its SVf_BREAK flag is set,
> then we don't add it to the free list, so it can never be reallocated (we
> still decrement PL_sv_count though).
> 
> Any thoughts, folks?

Since no-one commented, I've added it as change #34220.
Seems to fix the S_sv_del_backref assertion failure.

Change 34220 by davem@davem-pigeon on 2008/08/24 12:16:28

        Don't add freed SVF_BREAK scalars to the freed list.
        This may still be referenced, so don't reuse.

Affected files ...

... //depot/perl/sv.c#1554 edit

Differences ...

==== //depot/perl/sv.c#1554 (text) ====

@@ -192,13 +192,23 @@
 #  define POSION_SV_HEAD(sv)
 #endif
 
+/* Mark an SV head as unused, and add to free list.
+ *
+ * If SVf_BREAK is set, skip adding it to the free list, as this SV had
+ * its refcount artificially decremented during global destruction, so
+ * there may be dangling pointers to it. The last thing we want in that
+ * case is for it to be reused. */
+
 #define plant_SV(p) \
     STMT_START {                                       \
+       const U32 old_flags = SvFLAGS(p);                       \
        FREE_SV_DEBUG_FILE(p);                          \
        POSION_SV_HEAD(p);                              \
-       SvARENA_CHAIN(p) = (void *)PL_sv_root;          \
        SvFLAGS(p) = SVTYPEMASK;                        \
-       PL_sv_root = (p);                               \
+       if (!(old_flags & SVf_BREAK)) {         \
+           SvARENA_CHAIN(p) = (void *)PL_sv_root;      \
+           PL_sv_root = (p);                           \
+       }                                               \
        --PL_sv_count;                                  \
     } STMT_END


-- 
All right. I will give you one more chance. This time, I want to hear
no Wubens. No Weginalds. No Wudolf the wed-nosed weindeers.
    -- Life of Brian

---

References to:

Nicholas Clark <nick@ccl4.org>
Dave Mitchell <davem@iabyn.com>
Nicholas Clark <nick@ccl4.org>
Dave Mitchell <davem@iabyn.com>
Dave Mitchell <davem@iabyn.com>

---

[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index][Thread Index][Top&Search][Original]

---