Last Comment Bug 703699 - Fix use-after-free problem with write barriers and XPConnect
: Fix use-after-free problem with write barriers and XPConnect
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Bill McCloskey (:billm)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-18 12:13 PST by Bill McCloskey (:billm)
Modified: 2012-02-01 13:59 PST (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (9.10 KB, patch)
2011-11-18 12:13 PST, Bill McCloskey (:billm)
bhackett1024: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-11-18 12:13:46 PST
Created attachment 575523 [details] [diff] [review]
patch

Brian found this issue a few days ago. XPConnect will sometimes destroy its object during a GC after sweeping has taken place. This can cause a write barrier to trigger during the GC on an object that has been swept, which is bad.

This patch fixes the problem in a way that should guarantee that it doesn't happen again. The tricky part is knowing whether we're currently in a GC, since sometimes XPConnect destructors are called outside of a GC. The patch adds a .finalize(rt) method so that we get the runtime.
Comment 1 Brian Hackett (:bhackett) 2011-11-18 17:49:17 PST
Comment on attachment 575523 [details] [diff] [review]
patch

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

::: js/src/jsapi.h
@@ +2919,5 @@
> +     * It is unsafe to call the destructor during a GC unless finalize() has
> +     * already been called.
> +     */
> +
> +    ~HeapPtrObject() { JS_ASSERT(!value); JS_UnregisterReference((void **) &value); }

Why is the JS_UnregisterReference call necessary if value == NULL?

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +903,5 @@
> +     * The only time GetRuntime() will be NULL is if Destroy is called a second
> +     * time on a wrapped native. Since we already unregistered the pointer the
> +     * first time, there's no need to unregister again. Unregistration is safe
> +     * the first time because mWrapperWord isn't used afterwards.
> +     */

Are these constraints asserted anywhere?
Comment 2 Bill McCloskey (:billm) 2011-11-18 17:55:34 PST
(In reply to Brian Hackett from comment #1)
> Why is the JS_UnregisterReference call necessary if value == NULL?

I'll fix that.

> ::: js/xpconnect/src/XPCWrappedNative.cpp
> @@ +903,5 @@
> > +     * The only time GetRuntime() will be NULL is if Destroy is called a second
> > +     * time on a wrapped native. Since we already unregistered the pointer the
> > +     * first time, there's no need to unregister again. Unregistration is safe
> > +     * the first time because mWrapperWord isn't used afterwards.
> > +     */
> 
> Are these constraints asserted anywhere?

I can put in an assert that if GetRuntime() is null then mWrapperWord is also null. That would be useful. To check that mWrapperWord isn't used afterwards, I guess I could poison it.
Comment 3 Bill McCloskey (:billm) 2011-11-18 22:36:44 PST
Pushed with the unregister fix and the asserts and poisoning.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a77b629da06a
Comment 4 Ed Morley [:emorley] 2011-11-19 05:09:35 PST
https://hg.mozilla.org/mozilla-central/rev/a77b629da06a

Note You need to log in before you can comment on or make changes to this bug.