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

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

g = (function() {
    var Float32ArrayView = Float32Array()
    function f(i0, i1) {
        i1 = 1 | Infinity
        switch (1) {
            case 0:
                (w[0]) = 0
                i1 = -1
                break;
            case 0:
                i1 = x;
                break;
        }
        Float32ArrayView[0];
        i1 = (1 ? i1 : i0)
    }
    return f
})();
(function() {
    for (var j = 0; j < 2; ++j) {
        g()
    }
})()

asserts js debug shell on m-c changeset 06e9a27a6fcc with --ion-eager --ion-offthread-compile=off at Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp

My configure flags are:

MAKE=mozmake AR=ar sh c:\\\\Users\\\\mozillaadmin\\\\trees\\\\mozilla-central\\\\js\\\\src\\\\configure --enable-debug --disable-optimize --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --enable-threadsafe <other NSPR options>

This seems Win-only - I'll see if I can get a regression range. Setting s-s and sec-critical by default because this involves MIR.

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: more debug information

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5e18fd30243f
user:        Nicolas B. Pierron
date:        Thu May 15 22:57:18 2014 -0700
summary:     Bug 1007027 - Replace MPhi::slot by a flag based on ResumePoint indexes. r=h4writer

Actually, bug 1007027 may be related. Setting flags accordingly - seems like only Fx32 and later are affected.

Comment 4

[Nicolas B. Pierron [:nbp]]

I do not see how Bug 1007027 might have change this behavior, except by avoiding optimizations that are currently being done.  I do not see how our type inference might depend on the operating system.

Valgrind does not report any uninitialized value on linux x64 debug build.

Comment 5

[Nicolas B. Pierron [:nbp]]

I was able to reproduce this issue on Linux x86 debug build, with the same test case of comment 0.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: func10-pass04-Apply types-mir.gv.png

This is weird, apparently we produce a MBox during ApplyType out of a Phi node which used to be an Int32, and then becomes a Double during the ApplyType phase.

The result TypeSet of the MBox can be an Int32, but it does not list Double as being in the result type set of the MBox.

Also, there is some weirdness which might need Benjamin expertise, as a "-1" which is strangely encoded as a multiplication of Int32 becomes as -1 encoded as a multiplication of Float32.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Do not convert unused Phi to float32 operations.

The problem of the test case of comment 0 indeed raised with the patch which
is keeping the extra Phi nodes alive, as they are observable.

In the current float32 coercion, if a Phi or any instruction is unused (even
if captured by a resume points), then it can accept any Float32
computation.

The current patch prevents coercing to Float32 any unused operations, which
satisfy the typeSet of the original Phi node and pass the tests added by
emitValueResultChecks, as no Double appears in the typeSet of the MBox
operation.



Bug 1034349 - Do not convert unused Phi to float32 operations. r=

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Another test case that shows the issue:
function f(i) {
    var x = Math.fround(0);
    var z = 0;
    if (i == 42)
        z = 1;
    var w = z + 4;
    assertFloat32(w, false);
    return 1 ? z : w;
}

setJitCompilerOption("ion.usecount.trigger", 20);
for (var i = 70; i--;) {
    f(i)
}

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8451723 [details] [diff] [review]
Do not convert unused Phi to float32 operations.

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

So the issue is that when choosing specialization of Float32 operations, we can still specialize operations into float32, even if none of the operands has a floating point type (double or float32). This is pretty bad and more general that this issue.

There are only 9 trySpecializeFloat32 functions in MIR.cpp [1]. They all seem to use CheckUsesAreFloat32Consistent() [2] or EnsureFloatInputOrConvert [3]. We could check in these two functions that at least one of the operands has a floating point type. Nicolas, could you try that and check that it doesn't inhibit float32 specializations? (in particular, testFloat32.js should Just Work (c)). This looks more general than looking at used phis. Otherwise, we could even add this check in the float32 specialization algorithm, in IonAnalysis.cpp.

[1] http://dxr.mozilla.org/mozilla-central/search?q=trySpecialize&case=true
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#50
[3] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.cpp#844

Comment 10

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

Is this going to go in soon? Please don't forget to use sec-approval? for this since it is a sec-critical affecting multiple releases.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Al Billings [:abillings] from comment #10)
> Is this going to go in soon?

I intend to continue on it tomorrow, and submit another patch for comparison.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Skip Float32 specialization for int32 operations.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8461495 [details] [diff] [review]
Skip Float32 specialization for int32 operations.

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

So, as I understand it:
- MSqrt and MMathFunction don't need this as they return a floating point type anyways.
- MCompare doesn't need it because its trySpecializeFloat32 tests for compareType_ to be compare_Double.
- MNot doesn't need it because it doesn't specialize at the MIR level for Float32, it just converts a float32 back to a double if it was unsafe to take a float32 as an input.
- MFloor / MRound / MCeil don't need it (see my comment below).

So only MBinaryArithInstruction and MAbs need it, as a matter of fact. If one of these sounds wrong to you, let's discuss this more.

::: js/src/jit/MIR.cpp
@@ +859,5 @@
>      }
> +
> +    // Do not use Float32 if we can use int32.
> +    if (input->type() == MIRType_Int32)
> +        return false;

Actually this one isn't needed: EnsureFloatInputOrConvert is called only by MCeil, MRound, MFloor, which are created in MCallOptimize if the input has a floating point type (i.e. float32 or double), so no way to have a Int32 here. Can you JS_ASSERT(input->type() != MIRType_Int32) instead? (and check tests pass)

@@ +1519,5 @@
>          return;
>      }
>  
> +    // Do not use Float32 if we can use int32.
> +    if (specialization_ == MIRType_Int32)

This check is less costful than the check in the previous if branch. Can you put it at the top of this function?

@@ +1543,5 @@
>      }
>  
> +    // Do not use Float32 if we can use int32.
> +    if (input()->type() == MIRType_Int32)
> +        return;

Same here.

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Skip Float32 specialization for int32 operations.

Comment 15

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8462671 [details] [diff] [review]
Skip Float32 specialization for int32 operations.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily, the patch do hint at float32, but it does not relate to the wrong type caused by the eager float32 conversion.

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

No, the test case is not being included in the patch for security reasons.

> Which older supported branches are affected by this flaw?

Since Gecko 32.

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

Based on comment 3 and comment 7, this is cause by a combination of patches which made it in Firefox 32, including Bug 1007027.

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

The same patch should work on all branches.

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

This patch is working fine with both comment 0 and comment 8 tests cases and the test suite, when running it locally.  It should be architecture independent.

In the worse case scenario, this might disable some optimizations for float 32, and fallback on an explicit ToFloat32 conversion.

Comment 16

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

Comment on attachment 8462671 [details] [diff] [review]
Skip Float32 specialization for int32 operations.

sec-approval+ for trunk. Please nominate the patch for Aurora and Beta since fixing there will avoid us shipping this issue.

Comment 17

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcdc879af12f

Comment 18

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8462671 [details] [diff] [review]
Skip Float32 specialization for int32 operations.

Approval Request Comment
[Feature/regressing bug #]: Bug 1007027, …

[User impact if declined]: Potential pointer injection (MIR instruction returned value with unexpected type)

[Describe test coverage new/current, TBPL]: running on inbound, tested on try https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4ea1ba9c038

[Risks and why]: The risk is slow, as this code prevents eager optimizations which might be de-optimized to an explicit coercion to Float32.

[String/UUID change made/needed]: None.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcdc879af12f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/295491664949
https://hg.mozilla.org/releases/mozilla-beta/rev/4fc9c8d8a4ae

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4fc9c8d8a4ae

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Not having a Windows build environment set up at the moment, I tried this against a Linux x86 build, as comment 5 suggested. I'm able to reproduce and verify for the script in comment 8, but not original test in comment 0.

Gary, can you verify this as fixed on Fx32 and/or Fx33/Fx34 on your config?

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm able to verify as fixed on Fx34 nightly.