Last Comment Bug 690825 - "Assertion failure: !wrapper->unwrap(NULL)->isProxy()"
: "Assertion failure: !wrapper->unwrap(NULL)->isProxy()"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla10
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 714110
Blocks: 326633
  Show dependency treegraph
 
Reported: 2011-09-30 10:38 PDT by Jesse Ruderman
Modified: 2012-01-03 10:32 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (263 bytes, text/html)
2011-09-30 10:38 PDT, Jesse Ruderman
no flags Details
stack trace (11.15 KB, text/plain)
2011-09-30 10:39 PDT, Jesse Ruderman
no flags Details
fix HandleNonGenericMethodClassMismatch and nativeCall assert (46.59 KB, patch)
2011-10-03 10:24 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Review
add js::SecurityWrapper (13.32 KB, patch)
2011-10-03 11:33 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper (15.22 KB, patch)
2011-10-10 10:54 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Review

Description Jesse Ruderman 2011-09-30 10:38:14 PDT
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
Comment 1 Jesse Ruderman 2011-09-30 10:39:06 PDT
Created attachment 563770 [details]
stack trace
Comment 2 Jesse Ruderman 2011-09-30 10:39:45 PDT
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?)
Comment 3 Luke Wagner [:luke] 2011-09-30 18:01:37 PDT
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.
Comment 4 Luke Wagner [:luke] 2011-10-03 09:51:40 PDT
Jesse: here's the shell test-case:

var wrappedProxy = newGlobal('new-compartment').eval('Proxy.create({}, {}.prototype)');
String.prototype.toString.call(wrapedProxy);
Comment 5 Luke Wagner [:luke] 2011-10-03 10:24:28 PDT
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 ;-)
Comment 6 Luke Wagner [:luke] 2011-10-03 11:33:36 PDT
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.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-03 17:38:42 PDT
Comment on attachment 564227 [details] [diff] [review]
fix HandleNonGenericMethodClassMismatch and nativeCall assert

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

A little kludgy, but meh.
Comment 9 Marco Bonardo [::mak] 2011-10-05 05:08:00 PDT
https://hg.mozilla.org/mozilla-central/rev/6029755897c3
Comment 10 Luke Wagner [:luke] 2011-10-07 11:59:48 PDT
Still waiting on review from mrbkap and/or gal on the SecurityWrapper patch.
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-10-07 16:15:38 PDT
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.
Comment 12 Luke Wagner [:luke] 2011-10-10 10:54:13 PDT
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.
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-10-10 14:09:42 PDT
Comment on attachment 565976 [details] [diff] [review]
add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper

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

Thanks.
Comment 15 Ed Morley [:emorley] 2011-10-11 16:31:28 PDT
https://hg.mozilla.org/mozilla-central/rev/949c2cf4c772

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