"Assertion failure: !wrapper->unwrap(NULL)->isProxy()"

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: luke)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla10
x86_64
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 563769 [details]
testcase (asserts fatally when loaded)

This browser testcase triggers:

Assertion failure: !wrapper->unwrap(NULL)->isProxy(), at js/src/jswrapper.cpp:749
(Reporter)

Comment 1

6 years ago
Created attachment 563770 [details]
stack trace
(Reporter)

Comment 2

6 years ago
My guess at a JS shell testcase did NOT reproduce the assertion:

n = newGlobal('new-compartment')
(new WeakMap).has.call(n);

Instead, it said "function has() {[native code]} is not a function". (O rly?)
(Assignee)

Comment 3

6 years ago
The key piece of this test-cast is that 'this' is a wrapped wrapper (the first wrapper is cross-compartment, the second is the outer window, which is implemented as a wrapper, but not a cross-compartment wrapper) which the assert didn't think was possible.  So the simple fix is to change the assert to !isCrossCompartmentWrapper().

But then it hits a different assert (and null-deref) when HandleNonGenericMethodClassMismatch assumes that the callee is a function (in fact it is a wrapped function).  This is also trivially fixed by having callers explicitly pass the native.

Stepping through this I got the chills to see us enter the compartment of a different-domain (to do nativeCall).  This is safe because (by the NonGenericMethodGuard contract), this will immediately return an error.  However, I'd like to back off this ledge and just have the security wrappers override nativeCall to fail.  But that is gross and leads me back to a previous design I pursued, then backed off of, which was to make a new TransparentCrossCompartmentWrapper that was not derived by security wrappers.  That way, the *default* would be for nativeCall to give an errror.
Group: core-security
(Assignee)

Comment 4

6 years ago
Jesse: here's the shell test-case:

var wrappedProxy = newGlobal('new-compartment').eval('Proxy.create({}, {}.prototype)');
String.prototype.toString.call(wrapedProxy);
(Assignee)

Comment 5

6 years ago
Created attachment 564227 [details] [diff] [review]
fix HandleNonGenericMethodClassMismatch and nativeCall assert

Arg, I was so close to having had this caught by testCrossCompartmentTransparency.js.  Waldo, I blame you ;-)
Assignee: general → luke
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Attachment #564227 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

6 years ago
Created attachment 564274 [details] [diff] [review]
add js::SecurityWrapper

I started to add a TransparentCrossCompartmentWrapper (and make CrossCompartmentWrapper opaque by default), but this was going against the grain.  The natural solution I think is to add a js::SecurityWrapper that derives CrossCompartmentWrapper which has the meaning: "a cross compartment wrapper which does not share by default" (unlike Wrapper and CCW which are transparent by default).

Now, in this patch, I only added overrides for nativeCall and objectClassIs (and thus the testcase in comment 0 never enters the target compartment to call WeakMap_has) because I was scared of breaking things.  But I think, with SecurityWrapper in place, we may want to add more default-to-no overrides in the future.
Attachment #564274 - Flags: review?(mrbkap)
Attachment #564274 - Flags: feedback?(gal)
Comment on attachment 564227 [details] [diff] [review]
fix HandleNonGenericMethodClassMismatch and nativeCall assert

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

A little kludgy, but meh.
Attachment #564227 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6029755897c3
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/6029755897c3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

6 years ago
Still waiting on review from mrbkap and/or gal on the SecurityWrapper patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 564274 [details] [diff] [review]
add js::SecurityWrapper

I talked with Luke on IRC. There are a couple of same compartment wrappers that are security wrappers which necessitates making SecurityWrapper a template in its own right.
Attachment #564274 - Flags: review?(mrbkap)
(Assignee)

Updated

6 years ago
Attachment #564274 - Flags: feedback?(gal)
(Assignee)

Comment 12

6 years ago
Created attachment 565976 [details] [diff] [review]
add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper

The SecurityWrapper<> template was verbose so I added two slightly less but still verbose typedefs.
Attachment #564274 - Attachment is obsolete: true
Attachment #565976 - Flags: review?(mrbkap)
Comment on attachment 565976 [details] [diff] [review]
add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper

r=me with the fix to WrapperFactory::WrapSOWObject.

Thanks.
Attachment #565976 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 14

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/949c2cf4c772
https://hg.mozilla.org/mozilla-central/rev/949c2cf4c772
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 714110
You need to log in before you can comment on or make changes to this bug.