crash in mozilla::dom::InstanceClassHasProtoAtDepth

RESOLVED FIXED in Firefox 21

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mats, Assigned: bzbarsky)

Tracking

(4 keywords)

Trunk
mozilla22
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox20- wontfix, firefox21+ fixed, firefox22+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main21+], crash signature, )

Attachments

(2 attachments)

This bug was filed from the Socorro interface and is 
report bp-d2b6ee93-7d37-4078-a801-a34c02130322 .
============================================================= 

STR: I tried to open the links in bug 853772 comment 3


mozilla::dom::InstanceClassHasProtoAtDepth	dom/bindings/BindingUtils.cpp:619
TestShouldDOMCall	js/src/ion/IonBuilder.cpp:4215
mozilla::ErrorResult::ReportJSException	dom/bindings/BindingUtils.cpp:154
PR_GetCurrentThread	nsprpub/pr/src/pthreads/ptthread.c:621
js::ion::IonBuilder::makeCallHelper	js/src/ion/IonBuilder.cpp:4341
js::TempAllocPolicy::malloc_	obj-firefox/dist/include/js/Utility.h:151
js::ion::IonBuilder::makeCallBarrier	js/src/ion/IonBuilder.cpp:4380
js::ion::IonBuilder::jsop_call	js/src/ion/IonBuilder.cpp:4160
js::ion::IonBuilder::getPropTryConstant	js/src/ion/IonBuilder.cpp:6563
js::ion::MUnbox::New	js/src/ion/MIR.h:1872
PR_GetCurrentThread	nsprpub/pr/src/pthreads/ptthread.c:621
js::ion::MPassArg::New	js/src/ion/MIR.h:2104
js::ion::IonBuilder::inspectOpcode	js/src/ion/IonBuilder.cpp:932
js::ion::MBasicBlock::NewPopN	js/src/ion/MIRGraph.cpp:127
PR_GetCurrentThread	nsprpub/pr/src/pthreads/ptthread.c:621
PR_GetThreadPrivate	nsprpub/pr/src/threads/prtpd.c:200
js::ion::IonBuilder::processCondSwitchBody	js/src/ion/IonBuilder.cpp:2503
js::ion::IonBuilder::processCondSwitchCase	js/src/ion/IonBuilder.cpp:2474
js::ion::IonBuilder::processCondSwitchBody	js/src/ion/IonBuilder.cpp:2503
js::ion::IonBuilder::snoopControlFlow	js/src/ion/CompileInfo-inl.h:56
js::ion::IonBuilder::traverseBytecode	js/src/ion/IonBuilder.cpp:684
js::ion::IonBuilder::build	js/src/ion/IonBuilder.cpp:346
js::VectorImpl<js::types::CompilerOutput, 0ul, js::TempAllocPolicy, false>::grow	obj-firefox/dist/include/js/Utility.h:168
js::ion::SequentialCompileContext::compile	js/src/ion/Ion.cpp:1293
js::ion::IonBuilder::IonBuilder	js/src/ion/IonBuilder.cpp:48
js::ion::Compile<js::ion::SequentialCompileContext>	js/src/ion/Ion.cpp:1248
icon-theme.cache@0x135e000	
js::CallObject::createForFunction	js/src/gc/Barrier-inl.h:74
js::CallObject::createForFunction	js/src/gc/Barrier-inl.h:74
js::ion::CanEnterAtBranch	js/src/ion/Ion.cpp:1514
js::Interpret	js/src/jsinterp.cpp:1591
[...]
A null-deref on the domClass?  That's ... very odd.

Mats, can you get this in a debugger?  I'd very much like to interrogate the objects involved, but I can't reproduce the crash.  :(
I should have noted that the crash was with a non-default profile, sorry.
I've reduced the prefs.js that causes the crash to this:
user_pref("javascript.options.methodjit.content", false);

Boris, can you reproduce it with that?
Yes, indeed.

In fact, in a debug build I get a fatal assert on the toPrivate() bit of this line:

   js::GetReservedSlot(protoObject, DOM_PROTO_INSTANCE_CLASS_SLOT).toPrivate());

in mozilla::dom::InstanceClassHasProtoAtDepth.

The passed-in protoObject is "Worker".  Not "WorkerPrototype", but "Worker".  In particular, its getClass is Worker::sClass.

Its class flags are 0x330.

I assume this is part of the worker "use instance classes for prototypes but still use DOMJSClass for instances" insanity, but I can't find where workers are setting up their proto objects right now.  That spot needs to be fixed to not violate invariants.  The current setup is ... a problem.
Group: core-security
Component: DOM → DOM: Workers
It's possible that workers create the proto via the mess that is js::InitClassWithReserved...
This is all totally busted.  So.

1) Workers use the same JSClass for instances and prototypes.
2) The trace hook in this class tries to UnwrapDOMObject to WorkerPrivate and it it
   returns non-null (it'll return null for the proto!) traces it.
3) The DOM object is stored in slot 0.
4) The instance class is stored on normal WebIDL prototypes in slot 0.

So the only possible solution I can see here is to stop the insanity and stop using the same JSClass for both instances and protos for workers.  Ben, do we do that for anything other than Worker, ChromeWorker, and DedicatedWorkerGlobalScope?  On the other hand, if we do that, how do we make the objects have the right proto?  Right now it seems like it finds them via the insane jsclass-to-proto hashtable thing, right?

I guess another possible solution is to completely disable the ion DOM optimizations because of this worker insanity.  :(
And to be clear, this "reuse the same class" thing was supposed to be temporary when it was written over a year ago.  Since then, it's caused at least two security bugs that I know of.  We really need someone to actually fix it.  :(
Flags: needinfo?(bent.mozilla)
This almost certainly affects every single branch we have that has Ion...
We have no info around when this regressed or the level of criticality here and are about to build our final FF20 beta & rc candidate so minusing for tracking on FF20 and we can make a call on tracking for other versions once we get more info.
> We have no info around when this regressed

This regressed with the ionmonkey landing in 18.

> or the level of criticality here 

That's a harder call.   If the private on these is always JS::UndefinedValue, then it's possible that this is always a safe crash.  I just can't tell obviously from code inspection....

Certainly this is a trivial "crash the browser" kind of thing, but that's not that critical in some ways.
I talked this over with mrbkap yesterday and I think splitting the worker classes into a instance and prototype classes is fine if it makes life easier.

There's nothing really "insane" going on here, though... js::InitClass with a null private and a constructor function is just the way the JS engine does classes. It was the way we approximated DOM constructors back when this was written years ago. As we continue to aggressively optimize JS/DOM interaction we're bound to run into cases where this old approximation causes problems.
Flags: needinfo?(bent.mozilla)
> There's nothing really "insane" going on here, though...

There is from the point of view of WebIDL: prototypes and instance objects are totally different beasts in WebIDL, with different behavior, different [[Class]], etc...

The other "insane" part is manually doing things that sort of look like WebIDL objects but violate invariants WebIDL objects depend on, of course.   :(

In any case, if we plan to split this up, who's doing the work?  You, me, or Kyle seem like the obvious options...
(In reply to Boris Zbarsky (:bz) from comment #11)

Right, I'm in total agreement. I guess my point was just that this is old code written before WebIDL was a real thing and we're trying to retrofit it, basically. Incompatibilities that require code changes are inevitable.

> In any case, if we plan to split this up, who's doing the work?

I'm really hoping Kyle can tackle this. I'm utterly swamped for the next few weeks at least.
Assignee: nobody → bzbarsky
I'm just going to mark this as sec-high, because of unknown badness.  Feel free to adjust as needed.

Marking esr17 and b2g18 as unaffected, as they don't have Ionmonkey.
Comment on attachment 730123 [details] [diff] [review]
part 1.  Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects.

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

::: dom/workers/Worker.cpp
@@ +481,5 @@
> +    // js::InitClassWithReserved this JSClass and then call
> +    // JS_NewObject with our sClass and have it find the right
> +    // prototype.
> +    "ChromeWorker",
> +    JSCLASS_IS_DOMIFACEANDPROTOJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(2),

This only needs one reserved slot, right?

::: dom/workers/WorkerScope.cpp
@@ +895,5 @@
> +    // so that we can JS_InitClass this JSClass and then
> +    // call JS_NewObject with our sClass and have it find the right
> +    // prototype.
> +    "DedicatedWorkerGlobalScope",
> +    JSCLASS_IS_DOMIFACEANDPROTOJSCLASS | JSCLASS_HAS_RESERVED_SLOTS(2),

Same here.
Attachment #730123 - Flags: review?(bent.mozilla) → review+
Comment on attachment 730124 [details] [diff] [review]
part 2.  Remove a null check in bindings code that is no longer needed because workers no longer use a DOMJSClass for prototype objects.

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

This looks fine to me, but I bet you want peterv to look over this.
Attachment #730124 - Flags: review?(bent.mozilla) → review+
> This only needs one reserved slot, right?

No.  The second one is used by Xrays.
Comment on attachment 730123 [details] [diff] [review]
part 1.  Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects.

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

::: dom/workers/Worker.cpp
@@ +316,5 @@
> +  {
> +    // XXXbz we use "Worker" here to match sClass so that we can
> +    // js::InitClassWithReserved this JSClass and then call
> +    // JS_NewObject with our sClass and have it find the right
> +    // prototype.

Bah :-(. These should just be in the DOM proto array :-(.

@@ +336,5 @@
> +    JSCLASS_NO_INTERNAL_MEMBERS
> +  },
> +  eInterfacePrototype,
> +  &sWorkerNativePropertyHooks,
> +  "[object Worker]",

"[object WorkerPrototype]" I think.

@@ +499,5 @@
> +    JSCLASS_NO_INTERNAL_MEMBERS
> +  },
> +  eInterfacePrototype,
> +  &sWorkerNativePropertyHooks,
> +  "[object ChromeWorker]",

"[object ChromeWorkerPrototype]"

::: dom/workers/WorkerScope.cpp
@@ +913,5 @@
> +    JSCLASS_NO_INTERNAL_MEMBERS
> +  },
> +  eInterfacePrototype,
> +  &sWorkerNativePropertyHooks,
> +  "[object DedicatedWorkerGlobalScope]",

"[object DedicatedWorkerGlobalScopePrototype]"
Attachment #730123 - Flags: review?(peterv) → review+
Attachment #730124 - Flags: review?(peterv) → review+
> Bah :-(. These should just be in the DOM proto array :-(.

I agree, but that's a larger change that I'm not comfortable backporting to branches.  :(

> "[object WorkerPrototype]" I think.

Again, I was trying to preserve existing behavior as much as possible.  Are you OK with me not making that change?
Flags: needinfo?(peterv)
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3e18a5b5257d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/db2babefda12
Flags: needinfo?(peterv) → in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla22
Comment on attachment 730123 [details] [diff] [review]
part 1.  Give workers that are pretending to be on WebIDL bindings separate JSClasses for instance objects and prototype objects.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Ionmonkey landing
User impact if declined: Possible crashes in ion code that involves workers, may
   be exploitable.
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Risk is low to medium. 
   The other option is disabling the ion optimizations for WebIDL bindings, which
   I suspect is riskier, not to mention a performance hit.
String or IDL/UUID changes made by this patch: None.
Attachment #730123 - Flags: approval-mozilla-aurora?
(In reply to Boris Zbarsky (:bz) from comment #23)
> Comment on attachment 730123 [details] [diff] [review]
> part 1.  Give workers that are pretending to be on WebIDL bindings separate
> JSClasses for instance objects and prototype objects.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Ionmonkey landing
> User impact if declined: Possible crashes in ion code that involves workers,
> may
>    be exploitable.
> Testing completed (on m-c, etc.): Passes tests
> Risk to taking this patch (and alternatives if risky): Risk is low to
> medium. 
>    The other option is disabling the ion optimizations for WebIDL bindings,
> which
>    I suspect is riskier, not to mention a performance hit.

Based on the risk evaluation here and keeping in mind the alternative is riskier along with perf impact approving the patch here. But a quick check on what sort of impact this may have in terms of new regressions? Are we talking about a possible stability issue ?

> String or IDL/UUID changes made by this patch: None.
Attachment #730123 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
> But a quick check on what sort of impact this may have in terms of new regressions?

The most likely fallout would be either correctness or stability issues with workers.  Impact would be limited to pages that actually use workers.  Given that we do pass our worker tests, and we have sane test coverage here, the most likely regression is actually another worker-related security-critical crash bug of some sort.... :(
Blocks: 742193
Whiteboard: [adv-main21+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.