Closed
Bug 856384
Opened 12 years ago
Closed 12 years ago
"ASSERTION: PostCreate failed" and crash with Object.seal(win)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | + | fixed |
firefox23 | + | verified |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: jruderman, Assigned: Waldo)
References
Details
(5 keywords, Whiteboard: [fuzzblocker][adv-main22-])
Attachments
(3 files)
392 bytes,
text/html
|
Details | |
51.36 KB,
text/plain
|
Details | |
3.33 KB,
patch
|
bholley
:
review+
till
:
review+
bajaj
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
###!!! 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.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
This can lead to all kinds of GC-related crashes.
Assignee | ||
Updated•12 years ago
|
Assignee: general → jwalden+bmo
Comment 3•12 years ago
|
||
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•12 years ago
|
||
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!
![]() |
||
Updated•12 years ago
|
tracking-firefox22:
--- → ?
Assignee | ||
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
> 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.
Assignee | ||
Comment 7•12 years ago
|
||
ObjectImpl::preventExtensions calls GetPropertyNames(cx, self, JSITER_HIDDEN | JSITER_OWNONLY, &props). And seal has to call that, as its first step.
![]() |
||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
> 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. ;)
Assignee | ||
Comment 11•12 years ago
|
||
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.
Attachment #731955 -
Flags: review?(tschneidereit)
Attachment #731955 -
Flags: review?(bobbyholley+bmo)
Comment 12•12 years ago
|
||
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]
Review of attachment 731955 [details] [diff] [review]:
-----------------------------------------------------------------
Yes
Attachment #731955 -
Flags: review?(tschneidereit) → review+
Comment 13•12 years ago
|
||
Comment on attachment 731955 [details] [diff] [review]
Make nsOuterWindowProxy say always [[Extensible]]
Review of attachment 731955 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #731955 -
Flags: review?(bobbyholley+bmo) → review+
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #731955 -
Flags: sec-approval?
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #731955 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 15•12 years ago
|
||
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.
Attachment #731955 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
status-firefox23:
--- → affected
tracking-firefox23:
--- → +
Comment 16•12 years ago
|
||
This patch looks extremely safe, but is there any reason to approve before this lands to m-c per normal process?
Assignee | ||
Comment 17•12 years ago
|
||
I thought normal process was landing closer to simultaneously everywhere, for security bug things. Am I mistaken?
Updated•12 years ago
|
status-b2g18:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Comment 18•12 years ago
|
||
(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.
Updated•12 years ago
|
Keywords: checkin-needed
Comment 19•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 20•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Attachment #731955 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•12 years ago
|
||
Updated•12 years ago
|
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main22-]
Updated•12 years ago
|
Group: core-security
Comment 22•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•