856384 - "ASSERTION: PostCreate failed" and crash with Object.seal(win)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: PostCreate failed! This is known to cause inconsistent state for some class types and may even cause a crash in combination with a JS GC. Fix the failing PostCreate ASAP!: 'Error', file js/xpconnect/src/XPCWrappedNative.cpp, line 713

###!!! ASSERTION: tearoff not empty in dtor: '!(GetInterface()||GetNative()||GetJSObjectPreserveColor())', file js/xpconnect/src/XPCInlines.h, line 552

Plus some errors in chrome JS.

I suspect this is a regression from bug 789897.

Comment 1

[Jesse Ruderman]

Attachment: stacks (gdb)

Comment 2

[Jesse Ruderman]

This can lead to all kinds of GC-related crashes.

Comment 3

[Bobby Holley (:bholley)]

I bet the window isn't used to being sealed because it's always been wrapped via the outer window proxy, which, despite being transparent, never would have transmitted preventExtensions until now.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, we can't possibly support sealing of objects with a resolve hook.. at least not without first calling the resolve hook for all relevant property names!

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Yeah, we can't possibly support sealing of objects with a resolve hook.. at
> least not without first calling the resolve hook for all relevant property
> names!

Sure we can.  The resolve/enumerate contract is that anything that can be resolved, the enumerate hook has to define onto the object.  (At least, when called with JSITER_ALL, at least.  I can't remember if everything must be resolved onto the object when that's not there.)  When the stuff to enumerate, and the stuff resolved, is inconstant...um, yeah, need to think more.

In the longer WebIDL run the window object becomes a normal object with GSP being the cracktastic proto-chain-member here, so in theory you could maybe make the window object preventExtensions-able.  Maybe.  In the short run maybe we should just say "no".

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The resolve/enumerate contract is that anything that can be resolved, the enumerate hook
> has to define onto the object.

Does seal() call enumerate?

> In the longer WebIDL run the window object becomes a normal object with GSP being the
> cracktastic proto-chain-member

We're already there, afaik.  The only weirdness on the Window is the resolve hook that randomly resolves interface objects, standard classes, etc.

Comment 7

[Jeff Walden [:Waldo]]

ObjectImpl::preventExtensions calls GetPropertyNames(cx, self, JSITER_HIDDEN | JSITER_OWNONLY, &props).  And seal has to call that, as its first step.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, ok.

So I looked at the testcase, and the problem is it's sealing a WindowProxy, not a Window.  I don't see a sane way a WindowProxy can be sealable.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> Ah, ok.
> 
> So I looked at the testcase, and the problem is it's sealing a WindowProxy,
> not a Window.  I don't see a sane way a WindowProxy can be sealable.

Well, we could just forward it to the inner, right? What does the spec say to do here?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Well, we could just forward it to the inner, right?

We could, yes.  However note that the inner has, per spec, indexed properties that come and go.

> What does the spec say to do here?

Funny man.  ;)

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Make nsOuterWindowProxy say always [[Extensible]]

Test fails without patch:

367 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/base/test/test_window_extensible.html | preventExtensions(window) should throw a TypeError, instead got: didn't throw - got false, expected true
368 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/base/test/test_window_extensible.html | Windows are extensible after an extensibility "change" attempt - got false, expected true
JavaScript error: http://mochi.test:8888/tests/SimpleTest/TestRunner.js, line 264: TypeError: $(...).contentWindow.location is not extensible

and passes with.  Ideally the test would continue in an unfixed build, but I'm not sure I see how things would go wrong quite well enough to do that.  I'm not sure it's worth the time to figure out what exactly is going wrong and how, to do that.

I very much dislike the proxy handler trap-defining setup.  It seems very ad-hoc how you know what traps to override, in which classes, and if you miss one, you just don't know unless a fuzzer hits it.  Which so far has been easy, but I wouldn't count on it in all cases.

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

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

Yes

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

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

r=me

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

The regressing changes landed just early enough to be before the imminent uplift, and this patch would land now after the uplift.  So, sec-approval dance.  Blargh.  :-\  If I'd been thinking at all I'd have held off a few more days til after uplift; sorry for not thinking here.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Moderate to hard.  There's not an obvious straight-line path from issue to exploit, but it's probably not difficult to find *a* path with a little work.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Given this issue's going to exist for all of a few days, I didn't think it worth trying to obscure the issue at all in the patch/test.

Which older supported branches are affected by this flaw?
Only Aurora, assuming uplift gets the regressor but won't get this patch.

If not all supported branches, which bug introduced the flaw?
Bug 789897

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes, this patch.

How likely is this patch to cause regressions; how much testing does it need?
It's clear and obvious enough of a fix -- shouldn't regress anything.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789897
User impact if declined: Various internal invariants violated, consequences unknown for them, quite possibly you can make bad stuff happen (see the assert noted here).
Testing completed (on m-c, etc.): Hasn't landed on m-c yet, because of stupid mis-timing on my part and having the regressor and followups split around a merge date.  :-\
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Comment 16

[Alex Keybl [:akeybl]]

This patch looks extremely safe, but is there any reason to approve before this lands to m-c per normal process?

Comment 17

[Jeff Walden [:Waldo]]

I thought normal process was landing closer to simultaneously everywhere, for security bug things.  Am I mistaken?

Comment 18

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> I thought normal process was landing closer to simultaneously everywhere,
> for security bug things.  Am I mistaken?

We don't require that. Normally, things land on m-c and are then nominated for branches.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bab8308f561

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bab8308f561

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb0e98e308c

Comment 22

[Tiziana Sellitto [:tiziana]]

Verified fixed with the latest beta debug build, on a Mac OSX 10.8.4 machine. No assertion failure is now present.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130716 Firefox/23.0
Build ID: 20130716071311