Closed Bug 727330 Opened 8 years ago Closed 8 years ago

"Assertion failure: (*dictp)->inDictionary()" when proxy fix returns a large array

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox10 --- affected
firefox11 --- verified
firefox12 --- verified
firefox-esr10 11+ fixed
status1.9.2 --- unaffected

People

(Reporter: jruderman, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical][qa-] js-triage-done)

Attachments

(1 file)

var a = [];
for (var i = 0; i < 200; ++i) a.push({});
var p = Proxy.create({fix: function() { return a; }});
Object.preventExtensions(p);

Assertion failure: (*dictp)->inDictionary(), at js/src/jsscopeinlines.h:359

The first bad revision is:
changeset:   ff51ddfdf5d1
user:        Brian Hackett
date:        Wed Sep 28 15:04:55 2011 -0700
summary:     Remove shape numbers and Shape::slotSpan, factor Shape getter/setter into BaseShape, bug 684505.
Attached patch patchSplinter Review
Object swapping (as is done during proxy fixing) did not preserve the doubly linked structure of shape lists for objects in dictionary mode --- after swapping a link in the last property of one object could point to an internal field of the other object.  This issue predates bug 684505, since bug 684505 we generate a new Shape* when doing generateOwnShape, and failed when inserting that new shape into the broken dictionary.
Assignee: general → bhackett1024
Attachment #597997 - Flags: review?(luke)
Comment on attachment 597997 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Possible crashes, potential security exploit (after a GC pointers to freed memory could be used).
Risk to taking this patch (and alternatives if risky): Low, fixes incorrect state in an obscure corner of JS.
Attachment #597997 - Flags: approval-mozilla-beta?
Attachment #597997 - Flags: approval-mozilla-aurora?
Attachment #597997 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/75eb6956410b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Adding [sg:] to find out the security rating of this bug before deciding on whether or not to uplift to Aurora. Since this is an internally reported bug and we're already on our fourth beta, denying for mozilla-beta for the sake of risk mitigation.
Whiteboard: [sg:]
Attachment #597997 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Brian Hackett (:bhackett) from comment #2)
> [Approval Request Comment]
> User impact if declined: Possible crashes, potential security exploit (after
> a GC pointers to freed memory could be used).

Brian mentions potential security exploit -> assuming worse case [sg:critical]
Whiteboard: [sg:] → [sg:critical] js-triage-done
Alex, talking to Dan, we should probably take this for beta.
Comment on attachment 597997 [details] [diff] [review]
patch

[Triage Comment]
The security team now recommends that we uplift as soon as possible, so we'll take this for both Aurora 12 and Beta 11 based upon the low risk assessment.
Attachment #597997 - Flags: approval-mozilla-beta-
Attachment #597997 - Flags: approval-mozilla-beta+
Attachment #597997 - Flags: approval-mozilla-aurora?
Attachment #597997 - Flags: approval-mozilla-aurora+
Target Milestone: mozilla11 → mozilla13
Need this fix on ESR as well.
Comment on attachment 597997 [details] [diff] [review]
patch

Please land this today in time for tomorrow's go-to-build.  See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details and ping me or email release-mgmt@mozilla.com if there are any concerns about getting this landed in time.
Attachment #597997 - Flags: approval-mozilla-esr10+
Patch was backported to esr10, rebasing modifications were r=bhackett via IRC:

http://hg.mozilla.org/releases/mozilla-esr10/rev/c603b9cdee7f
> Patch was backported to esr10, rebasing modifications were r=bhackett via
> IRC:

Quickly landed to meet the March 2 build deadline, bhackett wasn't at a machine where he could do this today.
(In reply to Gary Kwong [:gkw, :nth10sd] from comment #13)
> > Patch was backported to esr10, rebasing modifications were r=bhackett via
> > IRC:
> 
> Quickly landed to meet the March 2 build deadline, bhackett wasn't at a
> machine where he could do this today.

This was backed out for causing extreme fire.

https://hg.mozilla.org/releases/mozilla-esr10/rev/314eac275aab
Oh, there's a bogus assert in esr10 when calling inDictionaryMode() on non-native objects.  Testing a->isNative() && a->inDictionaryMode() instead should work (and likewise for b).
Can someone give this another go?  Go to build is imminent and blocked on this landing.
> Can someone give this another go?  Go to build is imminent and blocked on
> this landing.

Landed:
http://hg.mozilla.org/releases/mozilla-esr10/rev/6dd1c5192f09
Backed out due to wrong fix:
http://hg.mozilla.org/releases/mozilla-esr10/rev/32cd852c1e6b

Gave it another go based on comment 15, hopefully it sticks on tbpl ( https://tbpl.mozilla.org/?tree=Mozilla-Esr10 ) this time:

http://hg.mozilla.org/releases/mozilla-esr10/rev/cdc9bf49ac24
> Gave it another go based on comment 15, hopefully it sticks on tbpl (
> https://tbpl.mozilla.org/?tree=Mozilla-Esr10 ) this time:
> 
> http://hg.mozilla.org/releases/mozilla-esr10/rev/cdc9bf49ac24

Looks green on tbpl, this stuck! :)
Note to QA: Apparently this will be a little tricky to test before & after landing of the patch on ESR - on ESR 10, the assert doesn't seem to show both before and after the patch.

The testcase doesn't fail before the patch landed because objshrink (which I think landed after Firefox 10) is needed to trigger the issue, but the problem still exists on Fx 10, just that the symptom is not the assert.
Given comment 19, the short timeframe to release, and the fact that we are focusing on multiple releases here, we will skip QA verification for esr10; instead we will verify for Firefox 11 and 12, and assume verified for Firefox 10.0.3esr.

If someone has ideas how we can verify this on Firefox 10.0.3esr, I'm open to suggestions.
Whiteboard: [sg:critical] js-triage-done → [sg:critical][qa+] js-triage-done
Verified fixed in Firefox 11.0b6, latest-12.0a2, and latest-13.0a1.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] js-triage-done → [sg:critical][qa!] js-triage-done
Group: core-security
Whiteboard: [sg:critical][qa!] js-triage-done → [sg:critical][qa-] js-triage-done
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.