Closed Bug 1313810 Opened 3 years ago Closed 3 years ago

Compartment mismatch or Assertion failure: cx->isInsideCurrentCompartment(proto.toObject()), at js/src/vm/ObjectGroup.cpp:470

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: gkw, Assigned: till)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 1561c917ee27 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

// jsfunfuzz-generated
toString = (function z() {
    return function() {
        Promise = evalcx('lazy').Promise;
    };
})();
[].join(this, toString);
// Adapted from randomly chosen test: js/src/tests/ecma_6/Promise/promise-subclassing.js
class SubPromise extends Promise {}
SubPromise.all([new Promise(function(){})]);


Backtrace:

0   js-dbg-64-dm-clang-darwin-1561c917ee27	0x000000010254cf0a js::ObjectGroup::defaultNewGroup(js::ExclusiveContext*, js::Class const*, js::TaggedProto, JSObject*) + 3226 (ObjectGroup.cpp:470)
1   js-dbg-64-dm-clang-darwin-1561c917ee27	0x0000000102341533 js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) + 243 (RootingAPI.h:802)
2   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d1b49 CreatePromiseObjectInternal(JSContext*, JS::Handle<JSObject*>, bool) + 217 (jsobjinlines.h:744)
3   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d3222 js::PromiseObject::create(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) + 210 (RootingAPI.h:802)
4   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001026d4a91 js::PromiseConstructor(JSContext*, unsigned int, JS::Value*) + 817 (RootingAPI.h:802)
5   js-dbg-64-dm-clang-darwin-1561c917ee27	0x000000010251236d js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 93 (jscntxtinlines.h:240)
/snip

For detailed crash information, see attachment.

Setting s-s because on opt builds, the following message shows:

*** Compartment mismatch 0x7fa0f1d48ac0 vs. 0x7fa0f1d3bce0
Segmentation fault: 11
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160716060553" and the hash "a80fdfc128b0c29dc34dd3fc28868252111a5d52".
The "bad" changeset has the timestamp "20160716064053" and the hash "a97ebab940c6ba0f19348c38d17a2193ee75c9ab".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a80fdfc128b0c29dc34dd3fc28868252111a5d52&tochange=a97ebab940c6ba0f19348c38d17a2193ee75c9ab

Till, is bug 1242662 a likely regressor?
Blocks: 1242662
Flags: needinfo?(till)
> Till, is bug 1242662 a likely regressor?

No, this is caused by bug 911216. I'm not sure why the regression range is off. Here's a simpler test case:

class SubPromise extends newGlobal().Promise {}
new SubPromise(()=>{});

Patch coming up.
Flags: needinfo?(till)
Turns out the way xray wrapper handling was set up for Promises didn't work for normal CCWs. I don't think this is s-s: it crashes release builds in a predictable, if unfortunate, way, but shouldn't be exploitable.
Assignee: nobody → till
Status: NEW → ASSIGNED
Attachment #8805768 - Flags: review?(arai.unmht)
Comment on attachment 8805768 [details] [diff] [review]
Properly handle Promise-subclassing with a cross-compartment Promise superclass

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

looks good.

as talked in IRC, we could remove wrap and unwrap for needsWrapping==true case, as it's used only in that case, and wrapped value isn't used.
but it's about API design (what should be wrapped and not), we could do in followup bug if necessary.

::: js/src/jit-test/tests/promise/promise-cross-compartment-subclassing.js
@@ +1,2 @@
> +global = newGlobal();
> +OtherPromise = global.Promise;

I'd prefer having var or let there.
Attachment #8805768 - Flags: review?(arai.unmht) → review+
(In reply to Till Schneidereit [:till] from comment #4)
> I don't think this is s-s: it crashes release builds in a
> predictable, if unfortunate, way, but shouldn't be exploitable.

Opening up.
Group: javascript-core-security
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc058bf7a5b
Properly handle Promise-subclassing with a cross-compartment Promise superclass. r=arai
https://hg.mozilla.org/mozilla-central/rev/0cc058bf7a5b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.