"Assertion failure: Chunk::withinArenasRange(addr)" with cross-compartment __lookupSetter__ on trickster proxy

RESOLVED FIXED in Firefox 16

Status

()

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

People

(Reporter: Jesse Ruderman, Assigned: bholley)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla17
x86_64
Mac OS X
assertion, crash, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox13 unaffected, firefox14 unaffected, firefox15 unaffected, firefox16+ fixed, firefox17+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [js:p1][advisory-tracking-] [qa?])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Comment 1

5 years ago
Created attachment 635639 [details]
stack trace
(Reporter)

Comment 2

5 years ago
The equivalent browser testcase crashes... but not always, and in various ways :(
Whiteboard: [js:t]
Whiteboard: [js:t] → [js:p1]
Assignee: general → bobbyholley+bmo
status-firefox13: --- → unaffected
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → affected
tracking-firefox16: --- → +
Created attachment 640555 [details] [diff] [review]
Use an AutoPropertyDescriptorRooter in obj_lookup{Getter,Setter}. v1
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?
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.
Attachment #640555 - Attachment is obsolete: true
Attachment #640556 - Flags: review?(luke)

Comment 6

5 years ago
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

5 years ago
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?
Attachment #640556 - Flags: review?(luke)
(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?
Attachment #640556 - Flags: review?(luke)
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.
Duplicate of this bug: 766430
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.
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.
Duplicate of this bug: 772215
Assignee: bobbyholley+bmo → continuation

Updated

5 years ago
Attachment #640556 - Flags: review?(luke) → review+
bholley is just going to go ahead and land his reviewed patch.  It seems good enough.
Assignee: continuation → bobbyholley+bmo
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/878c00396d62
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/878c00396d62
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: --- → fixed
Resolution: --- → FIXED
If you're happy with this patch please request approval for Aurora.
status-firefox-esr10: --- → unaffected
tracking-firefox17: --- → +
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.
Attachment #640556 - Flags: approval-mozilla-aurora?
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.
Attachment #640556 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/1a79e2cf11ad
status-firefox16: affected → fixed
Keywords: verifyme
Whiteboard: [js:p1] → [js:p1][advisory-tracking-]
Duplicate of this bug: 767966
Group: core-security
Flags: in-testsuite+
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?
Keywords: verifyme
Whiteboard: [js:p1][advisory-tracking-] → [js:p1][advisory-tracking-] [qa?]
You need to log in before you can comment on or make changes to this bug.