IonMonkey: SetElementIC dense element stub ignores double arrays

RESOLVED FIXED in Firefox 27

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

({sec-low})

unspecified
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox-esr24 wontfix, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [adv-main27-])

Attachments

(2 attachments)

Assignee

Description

6 years ago
Posted file Testcase
The attached testcase crashes on OS X 32-bit, inbound rev 961256d21b8e:

Assertion failure: JSVAL_IS_DOUBLE_IMPL(l), at ../dist/include/js/Value.h:404

With an opt build there's a correctness problem:

test.js:14:1 Error: Assertion failed: got 44, expected 26350
Assignee

Comment 1

6 years ago
In case it's not clear from the title, the problem is that the SetElementIC stub writes an int32 value to an array with the ObjectElements::CONVERT_DOUBLE_ELEMENTS flag. Ion code then expects a double and finds an int32 value.
Assignee

Updated

6 years ago
Flags: needinfo?(jdemooij)
What's the failure case here? We just interpret the number wrong? Or do we scoop up an extra byte to make the double and then misinterpret everything from that point on?
int-float confusion doesn't sound too bad
Keywords: sec-low
Assignee

Updated

6 years ago
Component: JavaScript Engine → JavaScript Engine: JIT
Flags: needinfo?(jdemooij)
Assignee

Updated

6 years ago
Flags: needinfo?(jdemooij)
Assignee

Comment 4

6 years ago
Posted patch PatchSplinter Review
This fell off my radar but, finally, here's a fix.

It's a bit unfortunate that we have to handle all these ConstantOrRegister cases, but this is the most efficient way to do it I think and pretty straight-forward.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8359296 - Flags: review?(shu)
Flags: needinfo?(jdemooij)

Comment 5

6 years ago
Comment on attachment 8359296 [details] [diff] [review]
Patch

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

I'm cancelling review until I'm convinced of the reg.type() assertion. You're probably right, but I'd like to understand why.

::: js/src/jit/IonCaches.cpp
@@ +3543,5 @@
> +        if (v.isInt32()) {
> +            Label dontConvert;
> +            masm.branchTest32(Assembler::Zero, elementsFlags,
> +                              Imm32(ObjectElements::CONVERT_DOUBLE_ELEMENTS),
> +                              &dontConvert);

I'm not clear on the mutual exclusion of the members of ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about NONWRITABLE_ARRAY_LENGTH?

If so, this needs to be and'd here.

@@ +3562,5 @@
> +
> +    Label convert, storeValue, done;
> +    masm.branchTest32(Assembler::NonZero, elementsFlags,
> +                      Imm32(ObjectElements::CONVERT_DOUBLE_ELEMENTS),
> +                      &convert);

Ditto.

@@ +3573,5 @@
> +        masm.branchTestInt32(Assembler::NotEqual, reg.valueReg(), &storeValue);
> +        masm.int32ValueToDouble(reg.valueReg(), ScratchFloatReg);
> +        masm.storeDouble(ScratchFloatReg, target);
> +    } else {
> +        JS_ASSERT(reg.type() == MIRType_Int32);

Why should this assert hold? What if we lowered into a LSetElementCacheT on a non-int32 type and tried to emit this dense stub?
Attachment #8359296 - Flags: review?(shu)
Assignee

Comment 6

6 years ago
(In reply to Shu-yu Guo [:shu] from comment #5)
> I'm not clear on the mutual exclusion of the members of
> ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can
> coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about
> NONWRITABLE_ARRAY_LENGTH?
> 
> If so, this needs to be and'd here.

branchTest32 *does* a bitwise-and. We're testing if the flag is set or not, it will work if there are other flags.

> Why should this assert hold? What if we lowered into a LSetElementCacheT on
> a non-int32 type and tried to emit this dense stub?

There's a "if (reg.hasTyped() && reg.type() != MIRType_Int32)" check before this. In that case we just do the store and return early.
Assignee

Updated

6 years ago
Flags: needinfo?(shu)

Comment 7

6 years ago
(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Shu-yu Guo [:shu] from comment #5)
> > I'm not clear on the mutual exclusion of the members of
> > ObjectElements::Flags. CONVERT_DOUBLE_ELEMENTS doesn't seem like it can
> > coexist with ASMJS_ARRAY_BUFFER or NEUTERED_BUFFER, but what about
> > NONWRITABLE_ARRAY_LENGTH?
> > 
> > If so, this needs to be and'd here.
> 
> branchTest32 *does* a bitwise-and. We're testing if the flag is set or not,
> it will work if there are other flags.

Ah silly me, it's a test, not a cmp.

> 
> > Why should this assert hold? What if we lowered into a LSetElementCacheT on
> > a non-int32 type and tried to emit this dense stub?
> 
> There's a "if (reg.hasTyped() && reg.type() != MIRType_Int32)" check before
> this. In that case we just do the store and return early.

I missed that part also. Sloppy read through.
Flags: needinfo?(shu)

Updated

6 years ago
Attachment #8359296 - Flags: review+
Group: javascript-core-security
landed on https://hg.mozilla.org/mozilla-central/rev/d3f32887012e

i guess jan landed that before on inbound but that was checkin comment was missed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Assignee

Comment 9

6 years ago
Comment on attachment 8359296 [details] [diff] [review]
Patch

Apparently this fixes some problems with pythonfiddle.com, see bug 959655 and bug 961124, so I think we should backport this.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 774006.
User impact if declined: Broken websites.
Testing completed: On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Fixes some problems with real-world websites.
Fix Landed on Version: 29, will likely be backported.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8359296 - Flags: approval-mozilla-esr24?
Attachment #8359296 - Flags: approval-mozilla-beta?
Attachment #8359296 - Flags: approval-mozilla-b2g26?
Attachment #8359296 - Flags: approval-mozilla-aurora?
Attachment #8359296 - Flags: approval-mozilla-beta?
Attachment #8359296 - Flags: approval-mozilla-beta+
Attachment #8359296 - Flags: approval-mozilla-aurora?
Attachment #8359296 - Flags: approval-mozilla-aurora+
Assignee

Comment 11

6 years ago
Setting checkin-needed for aurora.
Keywords: checkin-needed
Whiteboard: [adv-main27-]
Group: javascript-core-security
Comment on attachment 8359296 [details] [diff] [review]
Patch

Minus on approval‑mozilla‑b2g26. This is a low sec issue.
Attachment #8359296 - Flags: approval-mozilla-b2g26? → approval-mozilla-b2g26-
Comment on attachment 8359296 [details] [diff] [review]
Patch

minus for esr 24, this doesn't meet criteria and can ride the trains.
Attachment #8359296 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24-
Group: core-security
You need to log in before you can comment on or make changes to this bug.