Closed Bug 999358 Opened 6 years ago Closed 6 years ago

MLambdaArrow should initialize the unused extended slot

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 --- fixed
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

(Keywords: csectype-uninitialized, regression, sec-high, Whiteboard: [qa-])

Attachments

(1 file)

Not initializing the second slot probably caused bug 991755 comment 13.

I think we're also missing a post barrier atm, this patch fixes that too. Terrence, please double check this.
Attached patch PatchSplinter Review
Forgot to attach the patch; see comment 0.
Attachment #8410154 - Flags: review?(terrence)
Comment on attachment 8410154 [details] [diff] [review]
Patch

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

Nice! r=me

::: js/src/jit/IonBuilder.cpp
@@ +9442,5 @@
>      current->add(ins);
>      current->push(ins);
>  
> +    if (NeedsPostBarrier(info(), thisDef))
> +        current->add(MPostWriteBarrier::New(alloc(), ins, thisDef));

Actually, we shouldn't ever need a barrier here: LambdaArrow only generates a nursery allocation, so will either nursery allocate or take the ool path. If the lambda is only ever in the nursery then the |this| assignment can only create nursery->tenured edges, which we don't care about. The OOL path may allocate tenured, but it rejoins after initialization -- it will be barriered by the interpreter's normal mechanisms.
Attachment #8410154 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #2)
> Actually, we shouldn't ever need a barrier here: LambdaArrow only generates
> a nursery allocation, so will either nursery allocate or take the ool path.

Ah you're right. masm.newGCThing will only use the free list if initialHeap == gc::TenuredHeap, I was thinking about the nursery-disabled case but that doesn't affect this.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e4dac08b6e

(No sec-approval, does not affect branches.)
Blocks: 988993
https://hg.mozilla.org/mozilla-central/rev/b9e4dac08b6e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Group: javascript-core-security → core-security
Marking [qa-] due to lack of test and/or STR. Feel free to add that if you'd like QA verification, thanks.
Whiteboard: [qa-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.