Closed Bug 774859 Opened 10 years ago Closed 9 years ago

JM: Missing write barrier on JSOP_INITPROP


(Core :: JavaScript Engine, defect)

Other Branch
Not set



Tracking Status
firefox15 --- unaffected
firefox16 --- fixed
firefox17 --- verified
firefox-esr10 --- unaffected


(Reporter: billm, Assigned: billm)



(4 keywords, Whiteboard: [js:p1][jsbugmon:update][advisory-tracking-])


(2 files)

+++ This bug was initially created as a clone of Bug #773588 +++

The following testcase asserts on ionmonkey revision a29f6c635516 (run with --ion -n -m --ion-eager):

function f() {
    var a = [], i, N = 10;
    for (i = 0; i < N; i++)
        a[i] = {
		m: function() { return 0; }, 
		m: function() { return (false  ); }
        assertEq(f(), 1);
Whiteboard: [jsbugmon:update] → [js:p1][jsbugmon:update]
Is this only a problem in the debug-only verifier code, or a potential security problem for users?
Keywords: sec-high
With the patch in bug 781390, I was able to write a testcase. This is a real security problem, not just a verifier thing.
Keywords: sec-highsec-critical
Attached patch purge patchSplinter Review
This first patch just makes it easier to write reproducible test cases for the write barrier verifier. Currently, when starting verification, we set the allocatedDuringIncremental bit on every arena currently being allocated into. This is pretty conservative, because some of these objects were allocated before verification started. I noticed when trying to write a test case that this makes it much less likely that the verifier will fail.

I've replaced the allocatedDuringIncremental stuff with a patch that forces us to allocate into a fresh page when we start verification. This causes us to leak a little memory until the next GC (the remainder of the arena we were allocating into), but it's just for debugging, so it's not a big deal. And now it's easier to write test cases.
Attachment #650719 - Flags: review?
Attached patch initprop patchSplinter Review
This fixes the actual bug. compileBarriers() is like needsBarrier(), but slightly different. It's being introduced in bug 781390.
Attachment #650720 - Flags: review?(dvander)
Attachment #650719 - Flags: review? → review?(terrence)
Comment on attachment 650719 [details] [diff] [review]
purge patch

Review of attachment 650719 [details] [diff] [review]:

Whoa, we can get rid of prepareForIncrementalGC now.  Nice.
Attachment #650719 - Flags: review?(terrence) → review+
Attachment #650720 - Flags: review?(dvander) → review+
Comment on attachment 650720 [details] [diff] [review]
initprop patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 641025
User impact if declined: Possible crashes, security holes.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk. At worst, there could be performance regressions.
String or UUID changes made by this patch: None.
Attachment #650720 - Flags: approval-mozilla-aurora?
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
JSBugMon: This bug has been automatically verified fixed.
Attachment #650720 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Caused by bug 641025, landed in FF13. FF10 ESR is unaffected.
I assume as an ionmonkey bug, Firefox 15 is completely unaffected, especially since this comes from incremental GC, but I'm confirming here. Anyone?
This is a JaegerMonkey bug, but since it's incremental GC, FF15 is unaffected.
Ah, ok. I thought comment 0 implied ionmonkey but I'll take your word for it!
Whiteboard: [js:p1][jsbugmon:update] → [js:p1][jsbugmon:update][advisory-tracking-]
Depends on: 781390
Keywords: regression
This was initially found on IonMonkey, but then it turned out to affect Jagermonkey, too, in a way that required a different patch.
Group: core-security
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.