Last Comment Bug 767273 - "Assertion failure: Chunk::withinArenasRange(addr)" with cross-compartment __lookupSetter__ on trickster proxy
: "Assertion failure: Chunk::withinArenasRange(addr)" with cross-compartment __...
Status: RESOLVED FIXED
[js:p1][advisory-tracking-] [qa?]
: assertion, crash, sec-critical, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla17
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
: 766430 767966 772215 (view as bug list)
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2012-06-21 23:01 PDT by Jesse Ruderman
Modified: 2012-11-08 04:38 PST (History)
12 users (show)
dveditz: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
+
fixed
+
fixed
unaffected


Attachments
stack trace (18.40 KB, text/plain)
2012-06-21 23:02 PDT, Jesse Ruderman
no flags Details
Use an AutoPropertyDescriptorRooter in obj_lookup{Getter,Setter}. v1 (2.20 KB, patch)
2012-07-10 03:22 PDT, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review
Never declare a stack-allocated PropertyDescriptor within SM, and make API safe. v1 (4.38 KB, patch)
2012-07-10 03:37 PDT, Bobby Holley (PTO through June 13)
luke: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review
make it crashier (1.72 KB, patch)
2012-07-16 06:53 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Review

Description Jesse Ruderman 2012-06-21 23:01:51 PDT
var prox = Proxy.create({
   getPropertyDescriptor: function() { return undefined; },
   has:                   function() { return true; },
});
newGlobal("new-compartment").__lookupSetter__.call(prox, "e");

Assertion failure: Chunk::withinArenasRange(addr), at js/src/gc/Heap.h:826

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1bdd81c4d926
user:        Bobby Holley
date:        Tue Jun 12 15:44:14 2012 +0200
summary:     Bug 762432 - Handle proxies on __lookupGetter__ and __lookupSetter__. r=jorendorff
Comment 1 Jesse Ruderman 2012-06-21 23:02:11 PDT
Created attachment 635639 [details]
stack trace
Comment 2 Jesse Ruderman 2012-06-21 23:03:16 PDT
The equivalent browser testcase crashes... but not always, and in various ways :(
Comment 3 Bobby Holley (PTO through June 13) 2012-07-10 03:22:11 PDT
Created attachment 640555 [details] [diff] [review]
Use an AutoPropertyDescriptorRooter in obj_lookup{Getter,Setter}. v1
Comment 4 Bobby Holley (PTO through June 13) 2012-07-10 03:25:18 PDT
So, the attached patch here fixes the bug, but it feels like whack-a-mole to me. Basically, JSPropertyDescriptor is a raw struct with no constructor, so the fields end up garbage if they're not initialized. So if the descriptor ever crosses compartment boundaries, we try to wrap garbage and crash.

This happens in other places too, even in the API with JS_GetPropertyDescriptorById, which I imagine is the source of bug 772215.

I'm not an API guru here, but it seems like we should have some sort of consistent convention to apply here. Perhaps we should zero out anything that comes across the API, and only declare JSAutoPropertyDescriptorRooters internally?
Comment 5 Bobby Holley (PTO through June 13) 2012-07-10 03:37:32 PDT
Created attachment 640556 [details] [diff] [review]
Never declare a stack-allocated PropertyDescriptor within SM, and make API safe. v1

Here's a second, more complete stab at this. Not sure if this is the right approach here, but flagging luke for review.
Comment 6 Luke Wagner [:luke] 2012-07-10 08:52:34 PDT
You're right, the raw struct in the signature is just asking for trouble.

Waldo: does it make sense to just put the AutoPropertyDescriptorRooter in the signature?
Comment 7 Luke Wagner [:luke] 2012-07-12 16:05:35 PDT
Comment on attachment 640556 [details] [diff] [review]
Never declare a stack-allocated PropertyDescriptor within SM, and make API safe. v1

Could you try putting the AutoPropertyDescriptorRooter in the signature?
Comment 8 Bobby Holley (PTO through June 13) 2012-07-13 01:09:37 PDT
(In reply to Luke Wagner [:luke] from comment #7)
> Could you try putting the AutoPropertyDescriptorRooter in the signature?

This requires:

1 - Moving the definition of AutoPropertyDescriptorRooter into jsapi.h, placing it appropriately after the definition of JSPropertyDescriptor (away from the other rooter definitions), and in its own batch of c++ #ifdef gunk.

2 - Making JS_GetPropertyDescriptorById a C++-only API.

3 - Updating dozens of callers in gecko.

Can I please skip that?
Comment 9 Andrew McCreight [:mccr8] 2012-07-15 15:07:03 PDT
Oh, fun, I came across this exact same problem in bug 766430.  I already did Step 1 in comment 8 in a patch.  There are a number of other places that need to be fixed up, as can be seen in my patch in that bug.  I marked a few things as not really needing rooting, but we should probably do it anyways in service of having sane local invariants.

What I was thinking we could do is treat JSPropertyDescriptor like a Handle<> (eg somebody somewhere else has to have rooted it), and never use PropertyDescriptor outside of arguments (so it behaves like Root<>).  Instead, you need to use the rooter thing everywhere you declare a property descriptor.

I guess I misidentified the problem as being related to rooting rather than initialization. I was a little surprised that the GC could manage to run where it did, so initialization being messed up would make a little more sense.  I'll have to ponder some more.
Comment 10 Andrew McCreight [:mccr8] 2012-07-16 06:50:33 PDT
*** Bug 766430 has been marked as a duplicate of this bug. ***
Comment 11 Andrew McCreight [:mccr8] 2012-07-16 06:53:14 PDT
Created attachment 642569 [details] [diff] [review]
make it crashier

This patch demonstrates the underlying problem, by initializing the struct with 0xDEADBEEF instead of relying on the vagaries of stack memory to provide the lethal brew.  They should make the various browser tests in bug 766430 more reproducible.
Comment 12 Andrew McCreight [:mccr8] 2012-07-16 07:10:55 PDT
Oddly enough, I just had a crash on Nightly that looks a lot like this problem:
https://crash-stats.mozilla.com/report/index/339d438f-becd-4915-a251-83fe92120716
crash in JSCompartment::wrap inside a call to JS_GetPropertyDescriptorById from some list DOM binding stuff.
Comment 13 Andrew McCreight [:mccr8] 2012-07-16 07:29:10 PDT
*** Bug 772215 has been marked as a duplicate of this bug. ***
Comment 14 Andrew McCreight [:mccr8] 2012-07-16 14:27:03 PDT
bholley is just going to go ahead and land his reviewed patch.  It seems good enough.
Comment 15 Bobby Holley (PTO through June 13) 2012-07-17 03:12:14 PDT
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/878c00396d62
Comment 16 Ed Morley [:emorley] 2012-07-18 06:14:07 PDT
https://hg.mozilla.org/mozilla-central/rev/878c00396d62
Comment 17 Daniel Veditz [:dveditz] 2012-07-19 16:59:17 PDT
If you're happy with this patch please request approval for Aurora.
Comment 18 Bobby Holley (PTO through June 13) 2012-07-20 00:27:06 PDT
Comment on attachment 640556 [details] [diff] [review]
Never declare a stack-allocated PropertyDescriptor within SM, and make API safe. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 762432 sort of, but this stuff was always uninitialized.
User impact if declined: Potential crashes and security bugs.
Testing completed (on m-c, etc.): baking on m-c.
Risk to taking this patch (and alternatives if risky): Not risky - just roots and null-initializes some stuff.
String or UUID changes made by this patch: None.
Comment 19 Alex Keybl [:akeybl] 2012-07-23 10:42:47 PDT
Comment on attachment 640556 [details] [diff] [review]
Never declare a stack-allocated PropertyDescriptor within SM, and make API safe. v1

[Triage Comment]
Low risk sg:crit fix, approved for Aurora 16.
Comment 20 Bobby Holley (PTO through June 13) 2012-07-24 02:31:24 PDT
Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/1a79e2cf11ad
Comment 21 Andrew McCreight [:mccr8] 2012-11-05 17:45:42 PST
*** Bug 767966 has been marked as a duplicate of this bug. ***
Comment 22 Mihaela Velimiroviciu (:mihaelav) 2012-11-08 04:38:42 PST
I am unable to reproduce this on the rev mentioned in bug description. I built Spidermonkey and ran the provided code, but no assertions occured. I have also tried running that code on a debug build (id 20120614022225), but still couldn't reproduce. Any suggestions on how I can reproduce the assertion on the affected builds?

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