953114 - Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp:1223

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision cd3e9359fd64 (run with --fuzzing-safe --ion-eager):


function test() {
    var arr = new Uint32Array(100);
    arr[50] = 0xffffee00;
    for (var i=0; i<200; i++)
      res = arr[i || this];
}
test();

Comment 1

[Jan de Mooij [:jandem]]

Good catch. This is another TI problem with the Ion GetElement typed array stub (see also bug 929261). The result typeset is int32-or-undefined, but then we do this:

    if (output.hasValue())
        masm.loadFromTypedArray(arrayType, source, output.valueReg(), true,
                                elementReg, &popAndFail);

Passing true for allowDouble, so we'll happily read the double value (0xffffee00) out of the Uint32Array. The IC should probably have an allowDouble flag or something.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1b91cf5c8407
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 1.054 seconds to run.

Comment 3

[Andrew McCreight [:mccr8]]

This doesn't sound great, so I'm setting it to sec-high.  Feel free to adjust as needed.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Set an allowDoubleResult flag on the IC if it's okay to return a double and use that for Uint32Array.

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8355508 [details] [diff] [review]
Patch

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

OK, r=me with question below answered.

::: js/src/jit/MIR.cpp
@@ +2663,5 @@
>      return typedArray_->viewData();
>  }
>  
> +bool
> +MGetElementCache::allowDoubleResult() const

OK, this will work.

I was briefly considering a scheme in which we checked the state of TI at stub generation time, rather than cache creation time, and then generated sutbs based on that information, but this is simpler, if less responsive to change over time.

The only difference that I can see is that in this scheme, if we choose to go to the interpreter from the fallback path, and monitor a double result, we can then use that information to generate better stubs in the future for other typed arrays.

Is there some specific reason, other than simplicity, that we chose to check here, rather than in canAttachTypedArrayElement()?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8355508 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, but it may be possible for somebody with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Firefox 22+.

> If not all supported branches, which bug introduced the flaw?
Bug 840696.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Shouldn't be hard.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 7

[Jan de Mooij [:jandem]]

(As far as we know, the fuzzers never found a testcase that breaks/crashes due to this bug. That means it may not be exploitable, but in general breaking TI invariants (producing Values with unexpected types) is scary and should be considered sec-high/sec-critical just to be safe, I think.)

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8355508 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get Aurora, Beta, and ESR24 patches made and nominated once it is in.

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a585e70f28

(In reply to Eric Faust [:efaust] from comment #5)
> The only difference that I can see is that in this scheme, if we choose to
> go to the interpreter from the fallback path, and monitor a double result,
> we can then use that information to generate better stubs in the future for
> other typed arrays.
> 
> Is there some specific reason, other than simplicity, that we chose to check
> here, rather than in canAttachTypedArrayElement()?

I replied on IRC, but for posterity: stack/monitored TypeSets are frozen, so when we see a double the script will be invalidated anyway. The next time we compile it, we'll be able to attach the stub.

Setting needinfo for approval requests.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch for Aurora and Beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 840696.
User impact if declined: Security bugs.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR24

[Approval Request Comment]
User impact if declined: Security issues.
Fix Landed on Version: 29, will be backported to 27 and 28.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 12

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93fe56269382).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This was merged:
https://hg.mozilla.org/mozilla-central/rev/91a585e70f28

And uplifted:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b0157bcff32c
https://hg.mozilla.org/releases/mozilla-beta/rev/d63b720379c4
https://hg.mozilla.org/releases/mozilla-esr24/rev/ef97beb65d7f

Jan, can you please make a patch for b2g26 as well? Thanks :)

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Patch for b2g26

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/21fd14e1c3d4