The default bug view has changed. See this FAQ.

Fix use-after-free problem with write barriers and XPConnect

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
Attachment #575523 - Flags: review?(bhackett1024)
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?
Attachment #575523 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Pushed with the unregister fix and the asserts and poisoning.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a77b629da06a
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/a77b629da06a
Assignee: general → wmccloskey
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.