751454 - GC hazard around mNativeScriptableSharedMap when creating compartments

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Brian Hackett [Laid off!]]

Noticed this while building a browser with gczeal stuff baked in.  The entries in the mNativeScriptableSharedMap map in XPCJSRuntime are weakly held, and are cleared if there are no objects referring back to them.  These entries entrain JS classes, however, and if a GC is triggered between the point where the scriptable info is constructed and added to the map and the point where some JS thing is created which refers back to that map, then the JS class pointer becomes garbage.

A stack snippet is below for how the GC is triggered by construction of the JS-internal BaseShape which holds onto the class pointer, and that GC wipes out the class pointer for the BaseShape being constructed (presumably a GC at several other points under JS_NewGlobalObject could trigger the same thing, but I can't tell under which conditions the XPCNativeScriptableShared will actually be marked).

XPCWrappedNative::WrapNewGlobal should ensure that the XPCNativeScriptableInfo it constructs lives at least until the xpc_CreateGlobalObject call it makes finishes.

#0  JSClassSweeper (table=0x14b970, hdr=0x14b9f0, number=0, arg=0x0) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCJSRuntime.cpp:207
#1  0x03017307 in JS_DHashTableEnumerate (table=0x14b970, etor=0x20975db <JSClassSweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*)>, arg=0x0) at /Users/brianhackett/mozilla-inbound2/js/src/jsdhash.cpp:745
#2  0x0209b937 in XPCNativeScriptableSharedMap::Enumerate (this=0x14b960, f=0x20975db <JSClassSweeper(JSDHashTable*, JSDHashEntryHdr*, unsigned int, void*)>, arg=0x0) at XPCMaps.h:590
#3  0x0209649d in XPCJSRuntime::FinalizeCallback (fop=0xbfffd920, status=JSFINALIZE_END) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCJSRuntime.cpp:819
#4  0x0303a5b8 in SweepPhase (rt=0x9d7200, gckind=js::GC_NORMAL, startBackgroundSweep=0xbfffd9df) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3245
#5  0x0303a9f5 in GCCycle (rt=0x9d7200, incremental=false, budget=0, gckind=js::GC_NORMAL) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3626
#6  0x0303ad83 in Collect () at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3715
#7  0x0303afc9 in js::GC (rt=0x9d7200, gckind=js::GC_NORMAL, reason=js::gcreason::DEBUG_GC) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3736
#8  0x0303b289 in js::gc::RunLastDitchGC (cx=0x15d200, reason=js::gcreason::DEBUG_GC) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:1659
#9  0x0303b2e1 in js::gc::RunDebugGC (cx=0x15d200) at /Users/brianhackett/mozilla-inbound2/js/src/jsgc.cpp:3955
#10 0x031324f5 in js::gc::NewGCThing<js::BaseShape> (cx=0x15d200, kind=js::gc::FINALIZE_BASE_SHAPE, thingSize=32) at jsgcinlines.h:447
#11 0x031325e6 in js_NewGCBaseShape (cx=0x15d200) at jsgcinlines.h:537
#12 0x03128c00 in js::BaseShape::getUnowned (cx=0x15d200, base=@0xbfffdc4c) at /Users/brianhackett/mozilla-inbound2/js/src/jsscope.cpp:1206
#13 0x03128f20 in js::EmptyShape::getInitialShape (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0, kind=js::gc::FINALIZE_OBJECT16, objectFlags=0) at /Users/brianhackett/mozilla-inbound2/js/src/jsscope.cpp:1331
#14 0x030bf502 in NewObject (cx=0x15d200, clasp=0x15d704, type=0x19000020, parent=0x0, kind=js::gc::FINALIZE_OBJECT16) at /Users/brianhackett/mozilla-inbound2/js/src/jsobj.cpp:2754
#15 0x030c0874 in js::NewObjectWithGivenProto (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0, kind=js::gc::FINALIZE_OBJECT16) at /Users/brianhackett/mozilla-inbound2/js/src/jsobj.cpp:2806
#16 0x031786c2 in js::NewObjectWithGivenProto (cx=0x15d200, clasp=0x15d704, proto=0x0, parent=0x0) at jsobjinlines.h:1459
#17 0x031d7842 in js::GlobalObject::create (cx=0x15d200, clasp=0x15d704) at /Users/brianhackett/mozilla-inbound2/js/src/vm/GlobalObject.cpp:274
#18 0x02fb2010 in JS_NewGlobalObject (cx=0x15d200, clasp=0x15d704) at /Users/brianhackett/mozilla-inbound2/js/src/jsapi.cpp:3235
#19 0x02fb20f2 in JS_NewCompartmentAndGlobalObject (cx=0x15d200, clasp=0x15d704, principals=0x14f064) at /Users/brianhackett/mozilla-inbound2/js/src/jsapi.cpp:3268
#20 0x0205f565 in CreateNewCompartment (cx=0x15d200, clasp=0x15d704, principal=0x14f060, priv=0x15d800, global=0xbfffe094, compartment=0xbfffe090) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/nsXPConnect.cpp:1153
#21 0x020606fd in xpc_CreateGlobalObject (cx=0x15d200, clasp=0x15d704, principal=0x14f060, ptr=0x0, wantXrays=false, global=0xbfffe094, compartment=0xbfffe090) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/nsXPConnect.cpp:1255
#22 0x020bd5d7 in XPCWrappedNative::WrapNewGlobal (ccx=@0xbfffe11c, nativeHelper=@0xbfffe1d8, principal=0x14f060, initStandardClasses=false, wrappedGlobal=0xbfffe208) at /Users/brianhackett/mozilla-inbound2/js/xpconnect/src/XPCWrappedNative.cpp:377

Comment 1

[Bobby Holley (:bholley)]

Yeah, this is probably a regression from bug 720580, where I added WrapNewGlobal. I'll try to get a fix soon.

Do we generally consider timing-related GC bugs security bugs? Or are they too difficult to exploit?

Comment 2

[Bobby Holley (:bholley)]

I think we need to add an AutoMarkingPtr for XPCNativeScriptableInfo. Bill has some patches to refactor that stuff over in bug 715761, and I don't want to bitrot them. Let me see if I can cajole him into landing them.

Comment 3

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

Attachment: patch

This seems to fix the problem.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 621681 [details] [diff] [review]
patch


> typedef TypedAutoMarkingPtr<XPCNativeInterface> AutoMarkingNativeInterfacePtr;
> typedef TypedAutoMarkingPtr<XPCNativeSet> AutoMarkingNativeSetPtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNative> AutoMarkingWrappedNativePtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNativeTearOff> AutoMarkingWrappedNativeTearOffPtr;
> typedef TypedAutoMarkingPtr<XPCWrappedNativeProto> AutoMarkingWrappedNativeProtoPtr;
> typedef TypedAutoMarkingPtr<XPCMarkableJSVal> AutoMarkingJSVal;
>+typedef TypedAutoMarkingPtr<XPCNativeScriptableInfo> AutoMarkingNativeScriptableInfoPtr;
> 
> template<class T>
> class ArrayAutoMarkingPtr : public AutoMarkingPtr
> {
>   public:
>     ArrayAutoMarkingPtr(XPCCallContext& ccx)
>       : AutoMarkingPtr(ccx), mPtr(nsnull), mCount(0) {}
>     ArrayAutoMarkingPtr(XPCCallContext& ccx, T** ptr, PRUint32 count, bool clear)

Just below here, there's a comment explaining why there's no AutoMarkingNativeScriptableInfoPtr. Please remove it. :-)

r=bholley. Thanks for taking care of this one. :-)

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5473a24f93

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/3f5473a24f93

Comment 7

[Daniel Veditz [:dveditz]]

We should land this on Beta and Aurora so we will never have shipped with this security bug.

Comment 8

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

Comment on attachment 621681 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720580
User impact if declined: Possible crashes, but they're unlikely.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Very low risk.
String or UUID changes made by this patch: None

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 621681 [details] [diff] [review]
patch

low risk, sg:crit - approved for branches.

Comment 10

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

Landed on Aurora. Beta is closed for some reason.

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4121274b385

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Beta is open again, please go ahead and land.

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/c26bf281454f

Comment 13

[u279076]

Is there something QA can do to verify this fix? If not, @bhackett, can you help us with verifying this is fixed?

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Brian, do you have an answer for QA in comment 13?