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

VERIFIED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
JavaScript Engine: JIT
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla34
x86
All
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox30 unaffected, firefox31 unaffected, firefox32+ fixed, firefox33+ fixed, firefox34+ verified, firefox-esr24 unaffected, firefox-esr31 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [jsbugmon:])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Flags: needinfo?(jdemooij)
(Reporter)

Updated

3 years ago
status-firefox33: --- → affected
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
(Reporter)

Comment 2

3 years ago
Created attachment 8450688 [details]
more debug information
(Reporter)

Comment 3

3 years ago
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.
Blocks: 1007027
status-firefox30: --- → unaffected
status-firefox31: --- → unaffected
status-firefox32: --- → affected
status-firefox-esr24: --- → unaffected
tracking-firefox32: --- → ?
tracking-firefox33: --- → ?
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 4

3 years ago
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.
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Comment 5

3 years ago
I was able to reproduce this issue on Linux x86 debug build, with the same test case of comment 0.
tracking-firefox32: ? → +
tracking-firefox33: ? → +
(Assignee)

Updated

3 years ago
OS: Windows 7 → All
(Assignee)

Comment 6

3 years ago
Created attachment 8451639 [details]
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8451723 [details] [diff] [review]
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=
Attachment #8451723 - Flags: review?(benj)
(Assignee)

Updated

3 years ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
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 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
Attachment #8451723 - Flags: review?(benj)
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.
status-firefox34: --- → affected
tracking-firefox34: --- → +
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
Created attachment 8461495 [details] [diff] [review]
Skip Float32 specialization for int32 operations.
Attachment #8461495 - Flags: review?(benj)
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.
Attachment #8461495 - Flags: review?(benj) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8462671 [details] [diff] [review]
Skip Float32 specialization for int32 operations.
Attachment #8451723 - Attachment is obsolete: true
Attachment #8461495 - Attachment is obsolete: true
Attachment #8462671 - Flags: review+
(Assignee)

Comment 15

3 years ago
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.
Attachment #8462671 - Flags: sec-approval?
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.
Attachment #8462671 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 17

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcdc879af12f
(Assignee)

Comment 18

3 years ago
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.
Attachment #8462671 - Flags: approval-mozilla-beta?
Attachment #8462671 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bcdc879af12f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox34: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attachment #8462671 - Flags: approval-mozilla-beta?
Attachment #8462671 - Flags: approval-mozilla-beta+
Attachment #8462671 - Flags: approval-mozilla-aurora?
Attachment #8462671 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/295491664949
https://hg.mozilla.org/releases/mozilla-beta/rev/4fc9c8d8a4ae
status-b2g-v1.3: --- → unaffected
status-b2g-v1.3T: --- → unaffected
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → fixed
status-firefox32: affected → fixed
status-firefox33: affected → fixed
status-firefox-esr31: --- → unaffected
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4fc9c8d8a4ae
status-b2g-v2.0: affected → fixed
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?
Flags: needinfo?(gary)
(Reporter)

Comment 23

3 years ago
I'm able to verify as fixed on Fx34 nightly.
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
Flags: needinfo?(gary)
Group: javascript-core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.