965893 - Use-after-free by SnowWhiteKiller

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Splitting this off from bug 964462.  In summary, if a snow-white object gets resurrected via a weak reference, then killed off again, it can end up in the snow-white buffer twice, so we end up calling the destructor on a dead object.  Only the destructor is run, and no user code is probably going to run in between (only destructors), so it isn't as bad as it could be.

This appears to have been happening intermittently in bug 789919 since the initial landing of snow white!

Comment 1

[Andrew McCreight [:mccr8]]

I'd guess this is causing at least some of these other SnowWhiteKiller crashes.

Comment 2

[Andrew McCreight [:mccr8]]

Regression from bug 789919, so this affects 25+.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And note, we tried a version the patch for this to fix bug 964462 and it didn't help.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch (v4)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8368348 [details] [diff] [review]
patch (v4)

Gregor, I doubt this helps with your crash, but please test.

Comment 6

[Andrew McCreight [:mccr8]]

Sorry, I thought I understood what was supposed to be happening here, but now I'm not so sure.

If running the destructor of snow-white object A causes another snow-white object B to get touched, doesn't it end up purple, and thus the |(!o.mRefCnt->get() && !o.mRefCnt->IsInPurpleBuffer())| check will keep us from destroying B?  What is the bad sequence that results in something getting destroyed twice by the snow white killer?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

So one idea was that during nsPurpleBuffer::RemoveSkippable we go through pb.
We find something sw and put it to the sw-list and remove from pb. Then while going through
pb, we end up adding the same thing back to pb (because of some addref + release during canSkip or so)
and that is how we have the same object twice in sw.

Stabilizing sw right after removing from pb makes sure that addref/release during
pb cleanup doesn't affect to the results.

To me this is rather theoretical issue still, but could happen.

This bug is not about weak refs or such, I'd say.
(bug 964462 is still full mystery to me.)

Comment 8

[Andrew McCreight [:mccr8]]

It isn't clear this actually happens, so I'm going to lower this to moderate.  Feel free to upgrade the rating again if you disagree.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yeah, moderate is fine. So far we haven't seen any cases where this issue would have caused crashes.
It is just that if we add more skippability optimizations, this might become rather bad issue.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8368348 [details] [diff] [review]
patch (v4)

This won't probably fix the crasher. The patch in Bug 964462 may.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8368348 [details] [diff] [review]
patch (v4)

Review of attachment 8368348 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +402,5 @@
>  void
>  nsXPCWrappedJS::Destroy()
>  {
> +    MOZ_ASSERT(mRefCnt.IsInPurpleBuffer() ||
> +               1 == int32_t(mRefCnt), "should be stabilized for deletion");

Is the second part of this ever true?  Isn't it always going to be 0 here?  Maybe the check should be && int32_t(mRefCnt) == 0?

::: xpcom/base/nsCycleCollector.cpp
@@ +2314,5 @@
> +              NS_CycleCollectorSuspect3(o.mPointer, o.mParticipant, o.mRefCnt,
> +                                        nullptr);
> +            } else {
> +              // refcnt has been increased, but we aren't purple and aren't
> +              // really in the purple buffer, just just clear the bits.

nit: "just just" -> "so just" or something

::: xpcom/glue/nsISupportsImpl.h
@@ +113,5 @@
>    }
>  
>    MOZ_ALWAYS_INLINE void stabilizeForDeletion()
>    {
> +    // Mark us to be in the purple buffer,

I think it would be good if you also said that you are setting refcnt to 0.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> > +    MOZ_ASSERT(mRefCnt.IsInPurpleBuffer() ||
> > +               1 == int32_t(mRefCnt), "should be stabilized for deletion");
> 
> Is the second part of this ever true? 
Yes, IIRC. I'll look at it before landing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Should we INCOMPLETE this at this point?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I think so. This was one idea for Bug 964462, but that ended up being a bug in OfflineCacheUpdateChild,  and I think we found that this bug isn't really possible.

Andrew, feel free to disagree.