699594 - UnmarkGrayChildren is unsafe in presence of the background finalization thread

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Igor Bukanov]

UnmarkGrayChildren, http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#609 , recursively calls  JS_TraceChildren to unmark the gray objects. However, this is not safe if the background finalization thread is running.

The problem comes from the usage of the conservative marking calls like MarkStackRangeConservatively, http://mxr.mozilla.org/mozilla-central/source/js/src/jsiter.cpp#1139 , when navigating the object graph under JS_TraceChildren. As that call checks an address against the GC chunk table and other GC data structures to verify for live GC things it is important that the those data structures are not freed.

However, with the background finalization thread running this is violated. For example, it is possible that the background thread manages to finalize and release the GC chunk for GC thing that MarkStackRangeConservatively just finds and tries to mark. After that the memory range corresponding to the chunk can be allocated and filled from other threads. Then the GC graph traversal may call a function through a pointer stored in that filled memory.

Comment 1

[Igor Bukanov]

A similar problem can also happen if another thread allocates objects. Then the conservative scanner under UnmarkGrayChildren may observe partially objects when they still contains data from older allocations. But if we do not allow/support running JS on non-main thread that uses the main JS runtime this may not be applicable to the browser.

Comment 2

[Daniel Veditz [:dveditz]]

In Fx10 we think all JS threads run on separate runtimes which would make this a non-issue, but in Fx9 we know at least PSM ran multiple JS threads on the same runtime. Can we let this ride until Jan 31 (when Fx10 is scheduled to ship). We don't even have a testcase showing it's possible to line up the necessary conditions to trigger the bug.

Comment 3

[Johnny Stenback (:jst)]

Not sure if this is applicable in the browser on trunk any more since I do not think we're ever running JS on different threads, but this could be exploitable in Firefox 8 and 9. Assigning to Andrew in case there's any work that's already happening that we could backport, or if there's an easy branch patch that we could take in the older releases.

Comment 4

[Andrew McCreight [:mccr8]]

The situation Igor describes in comment #0 comes up even with single-threaded JS, because we still do finalization on another thread.

It looks like the only place JS_TraceChildren may call MarkStackRangeConservatively is in when we're tracing a generator? [1]  The other call sites look like various stack tracing things, which I wouldn't think would be affected.  Can we use a lock or a flag or something to indicate to the finalization thread that we're currently looking at a particular chunk?


[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jsiter.cpp#1136

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I think we should just remove conservative stack scanning from jsiter.cpp. And we should also take it out of the JS stack scanning for good measure.

Brian, how difficult is this? David said he's pretty sure that since TI we sync the JS stack at all times. I guess that doesn't help for FF8, though.

Comment 6

[Brian Hackett [Laid off!]]

With TI we ensure that there is a fully synced interpreter stack when calling into the VM, except that local variables which are dead at the call site may have a torn value (there are a couple other places that want to see handling for this, though).

If the frame being traced is a generator frame, is it even possible that the frame could have been running in JM?  JM does not compile scripts containing JSOP_GENERATOR.  If a frame has only run in the interpreter, its contents should definitely be synced up to the current stack position, in any version of Firefox.

Comment 7

[Luke Wagner [:luke]]

(In reply to Brian Hackett from comment #6)
> If a frame has only run in the interpreter, its contents
> should definitely be synced up to the current stack position, in any version
> of Firefox.

There is still the situation where jit code calls a generator with overflow args and the non-canonical args slots are left in an undefined state.  That's what http://hg.mozilla.org/mozilla-central/file/c60535115ea1/js/src/vm/Stack.cpp#l139 is trying to catch.  If you changed the Debug_SetValueRangeToCrashOnTouch to MakeValueRangeGCSafe I think you could remove the conservative marking in generator_trace.

Comment 8

[Igor Bukanov]

(In reply to Luke Wagner [:luke] from comment #7)
> That's what
> http://hg.mozilla.org/mozilla-central/file/c60535115ea1/js/src/vm/Stack.
> cpp#l139 is trying to catch.  If you changed the
> Debug_SetValueRangeToCrashOnTouch to MakeValueRangeGCSafe I think you could
> remove the conservative marking in generator_trace.

It would be nice if we use the conservative scanner only when checking the native stack roots. Besides fixing this bug it would also simplify a couple of other my patches :)

Comment 9

[Andrew McCreight [:mccr8]]

I'm going to unassign myself from this bug, as it sounds like the path forward is to remove the uses of MarkStackRangeConservatively from JS_TraceChildren, which occur entirely within MarkGenerator in jsiter.cpp.

Comment 10

[Andrew McCreight [:mccr8]]

Bill, sounds like bug 714109 will fix this, by removing the use of conservative scanning in JS_TraceChildren?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Bill, sounds like bug 714109 will fix this, by removing the use of
> conservative scanning in JS_TraceChildren?

Yes, I forgot about this. I agree.

Comment 12

[Johnny Stenback (:jst)]

Andrew, Bill, now that bug 714109 is fixed, is there anything else left to do here? Or can we mark this fixed?

Comment 13

[Andrew McCreight [:mccr8]]

MarkStackRangeConservatively doesn't exist any more, so I think this is fixed.

Comment 14

[u279076]

Is there something QA can do to verify this fix?