Last Comment Bug 751454 - GC hazard around mNativeScriptableSharedMap when creating compartments
: GC hazard around mNativeScriptableSharedMap when creating compartments
Status: RESOLVED FIXED
[sg:critical][advisory-tracking+]
: regression, sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on: 715761
Blocks: 720580
  Show dependency treegraph
 
Reported: 2012-05-02 20:14 PDT by Brian Hackett (:bhackett)
Modified: 2012-07-11 20:00 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed
fixed
unaffected


Attachments
patch (3.07 KB, patch)
2012-05-07 11:53 PDT, Bill McCloskey (:billm)
bobbyholley: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Brian Hackett (:bhackett) 2012-05-02 20:14:40 PDT
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 (busy) 2012-05-04 00:49:33 PDT
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 (busy) 2012-05-06 13:16:34 PDT
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 (:billm) 2012-05-07 11:53:49 PDT
Created attachment 621681 [details] [diff] [review]
patch

This seems to fix the problem.
Comment 4 Bobby Holley (busy) 2012-05-08 02:28:00 PDT
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 6 Bill McCloskey (:billm) 2012-05-08 18:49:29 PDT
https://hg.mozilla.org/mozilla-central/rev/3f5473a24f93
Comment 7 Daniel Veditz [:dveditz] 2012-05-17 16:58:06 PDT
We should land this on Beta and Aurora so we will never have shipped with this security bug.
Comment 8 Bill McCloskey (:billm) 2012-05-18 11:52:30 PDT
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 2012-05-18 15:43:58 PDT
Comment on attachment 621681 [details] [diff] [review]
patch

low risk, sg:crit - approved for branches.
Comment 10 Bill McCloskey (:billm) 2012-05-21 11:02:35 PDT
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 2012-05-21 15:37:13 PDT
Beta is open again, please go ahead and land.
Comment 12 Bill McCloskey (:billm) 2012-05-21 16:12:20 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/c26bf281454f
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-29 11:29:51 PDT
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] 2012-06-08 17:21:28 PDT
Brian, do you have an answer for QA in comment 13?

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