Closed Bug 897403 Opened 7 years ago Closed 6 years ago

"Assertion failure: !((attrs ^ shape->attrs) & 0x40) || !(attrs & 0x40)" with bound function proxy

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox22 --- affected
firefox23 - affected
firefox24 - affected
firefox25 - affected
b2g18 ? ---

People

(Reporter: jruderman, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

Attached file stack
var boundFun = (function(){}).bind({});
var prox = new Proxy(boundFun, {});
Object.defineProperty(prox, "caller", {get: function(){}});

Assertion failure: !((attrs ^ shape->attrs) & 0x40) || !(attrs & 0x40), at js/src/vm/Shape.cpp:767

This testcase causes trouble all the way back to the landing of direct proxies in bug 703537.  Bug 788172 made it easier for one of my fuzzers to find it.
Bug 703537 landed in Firefox 18, setting tracking flags as necessary.
If this is present all the way back to FF18 we have no reason to track this but a low risk uplift nomination can be considered when a fix is available.
The assertion is this:

    /* Allow only shared (slotless) => unshared (slotful) transition. */
    JS_ASSERT(!((attrs ^ shape->attrs) & JSPROP_SHARED) ||
              !(attrs & JSPROP_SHARED));

I have no idea what that means, so I'm going to assume it is bad.  Feel free to adjust.
Keywords: sec-high
jandem, you fixed a similar bug 867082 as well, possible to take a look?
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> jandem, you fixed a similar bug 867082 as well, possible to take a look?

Unfortunately I've no idea what's causing this. Somebody more familiar with proxies should take a look (maybe Eddy or Jason).
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> > jandem, you fixed a similar bug 867082 as well, possible to take a look?
> 
> Unfortunately I've no idea what's causing this. Somebody more familiar with
> proxies should take a look (maybe Eddy or Jason).

Jason, thoughts?
Flags: needinfo?(jorendorff)
(In reply to Andrew McCreight [:mccr8] from comment #3)
> The assertion is this:
> 
>     /* Allow only shared (slotless) => unshared (slotful) transition. */
>     JS_ASSERT(!((attrs ^ shape->attrs) & JSPROP_SHARED) ||
>               !(attrs & JSPROP_SHARED));
> 
> I have no idea what that means, so I'm going to assume it is bad.  Feel free
> to adjust.

I don't think this assertion is security-important. Changing a property from slotful to slotless leaks a slot, I think.

I have a shallow fix for the symptom here: just make the property slotless to begin with. fun_getProperty doesn't use the slot for anything.

Of course the real fix would be to make it so JS_DefineProperty doesn't flunk an assertion in this sort of case; I think it should fail with an exception instead. Let's save that for a followup.
Flags: needinfo?(jorendorff)
Attached patch v1Splinter Review
Assignee: general → jorendorff
Attachment #803100 - Flags: review?(jwalden+bmo)
Group: core-security
Clearing sec-flag per comment 8.
Keywords: sec-high
Attachment #803100 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/3cb16a4bf227
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.