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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

The equivalent browser testcase crashes... but not always, and in various ways :(

Comment 3

[Bobby Holley (:bholley)]

Attachment: Use an AutoPropertyDescriptorRooter in obj_lookup{Getter,Setter}. v1

Comment 4

[Bobby Holley (:bholley)]

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 (:bholley)]

Attachment: 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]]

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]]

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 (:bholley)]

(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]]

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 11

[Andrew McCreight [:mccr8]]

Attachment: 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]]

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 14

[Andrew McCreight [:mccr8]]

bholley is just going to go ahead and land his reviewed patch.  It seems good enough.

Comment 15

[Bobby Holley (:bholley)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/878c00396d62

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/878c00396d62

Comment 17

[Daniel Veditz [:dveditz]]

If you're happy with this patch please request approval for Aurora.

Comment 18

[Bobby Holley (:bholley)]

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]]

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 (:bholley)]

Pushed to m-a:
http://hg.mozilla.org/releases/mozilla-aurora/rev/1a79e2cf11ad

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

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?