1325344 - Hit MOZ_CRASH(ToInt32 invalid input type) at js/src/jit/Lowering.cpp:2159

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

The following testcase crashes on mozilla-central revision 7083c0d30e75 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --ion-gvn=off):

function f(x, y) {
    y(1 != x || 1 == -x);
}
for (var i = 0; i < 2; i++) {
    try {
        f();
    } catch (e) {}
}


Backtrace:

0   js-dbg-64-dm-clang-darwin-7083c0d30e75	0x0000000101865863 js::jit::LIRGenerator::visitToInt32(js::jit::MToInt32*) + 1619 (Lowering.cpp:2159)
1   js-dbg-64-dm-clang-darwin-7083c0d30e75	0x0000000101893b48 js::jit::LIRGenerator::visitInstruction(js::jit::MInstruction*) + 56 (Lowering.cpp:4764)
2   js-dbg-64-dm-clang-darwin-7083c0d30e75	0x000000010189430f js::jit::LIRGenerator::visitBlock(js::jit::MBasicBlock*) + 463 (Lowering.cpp:4842)
3   js-dbg-64-dm-clang-darwin-7083c0d30e75	0x0000000101894c2b js::jit::LIRGenerator::generate() + 155 (Lowering.cpp:4907)
4   js-dbg-64-dm-clang-darwin-7083c0d30e75	0x0000000101795a71 js::jit::GenerateLIR(js::jit::MIRGenerator*) + 529 (Ion.cpp:1931)
/snip

For detailed crash information, see attachment.

Setting s-s as a start because LIR is involved.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

[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/d791ba00bf06
user:        Hannes Verschore
date:        Fri Aug 14 12:45:47 2015 +0200
summary:     Bug 1171945: IonMonkey: Use tryXXX structure for jsop_binary in IonBuilder, r=jandem

Hannes, is bug 1171945 a likely regressor?

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch

We add Beta nodes after compares to contain the information of what types can go through. Here we were comparing if (1 != x) and in the false branch we added a beta that made the range of "x" is [1:1]. Which would have been correct if x was actually a number type. Which in this testcase it isn't. It is "undefined". Which is not a number type and we shouldn't make assumptions on that, because that can lead to bugs ;).

Comment 4

[Daniel Veditz [:dveditz]]

Hannes: what's the security impact here? Is this something we need to uplift to beta to avoid security problems?

Comment 5

[Jan de Mooij [:jandem]]

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

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

Makes sense.

Comment 6

[Hannes Verschore [:h4writer]]

Quite an old bug. Hard to pinpoint the actual date, but could be 2 years already.

(In reply to Daniel Veditz [:dveditz] from comment #4)
> Hannes: what's the security impact here? Is this something we need to uplift
> to beta to avoid security problems?

This particular case calls MOZ_CRASH nicely. Which would mean no security issues.
I looked at the crash-stats and we have this particular crash not happen in the last month.

Not all erroneous paths will have this nice MOZ_CRASH there.
Though it took fuzzers a non-default configurations to find this issue and also 10+ releases.
(Note this bug is also present in default configuration but even harder to hit).

As a result I wouldn't be worried about having this bug in the tree for a bit longer if needed.
Though this is a easy/quick and well understood fix. If we can we should just take it.

Comment 7

[Hannes Verschore [:h4writer]]

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

Approval Request Comment
[Feature/Bug causing the regression]:
Unknown. Long time ago.

[User impact if declined]:
Maybe some more crashes. Though not encountered in the wild yet.

[Is this code covered by automated tests?]:
yes

[Has the fix been verified in Nightly?]:
Verified on the automated tests and the testcase added here.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
nothing

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Disables some optimizations for special cases. They now take the regular code path which has been tested extensively. Patch is small and well understood.

[String changes made/needed]:
/

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Crash fix, doesn't sound too risky, let's go ahead and uplift this to aurora and beta.

Comment 9

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

This landed to inbound at some point and now merged to m-c:
https://hg.mozilla.org/mozilla-central/rev/05f02bc144b1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ec81934225f

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/08e289b7046a9fc826e75455d533cbb3f250df27

Comment 12

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx51
JSBugMon: This bug has been automatically verified fixed on Fx52