Closed Bug 1325344 Opened 8 years ago Closed 8 years ago

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


(Core :: JavaScript Engine, defect, P1)




Tracking Status
firefox-esr45 --- wontfix
thunderbird_esr45 --- affected
firefox50 --- wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified


(Reporter: gkw, Assigned: h4writer)



(4 keywords, Whiteboard: [jsbugmon:update][adv-main51+])


(2 files)

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 {
    } catch (e) {}


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)

For detailed crash information, see attachment.

Setting s-s as a start because LIR is involved.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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?
Blocks: 1171945
Flags: needinfo?(hv1989)
Attached patch PatchSplinter Review
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 ;).
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8821194 - Flags: review?(jdemooij)
Hannes: what's the security impact here? Is this something we need to uplift to beta to avoid security problems?
Comment on attachment 8821194 [details] [diff] [review]

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

Makes sense.
Attachment #8821194 - Flags: review?(jdemooij) → review+
Priority: -- → P1
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.
Keywords: sec-low
Comment on attachment 8821194 [details] [diff] [review]

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?]:

[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]: 

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

[Is the change risky?]:

[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]:
Attachment #8821194 - Flags: approval-mozilla-beta?
Attachment #8821194 - Flags: approval-mozilla-aurora?
Comment on attachment 8821194 [details] [diff] [review]

Crash fix, doesn't sound too risky, let's go ahead and uplift this to aurora and beta.
Attachment #8821194 - Flags: approval-mozilla-beta?
Attachment #8821194 - Flags: approval-mozilla-beta+
Attachment #8821194 - Flags: approval-mozilla-aurora?
Attachment #8821194 - Flags: approval-mozilla-aurora+
This landed to inbound at some point and now merged to m-c:
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
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
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main51+]
Group: core-security-release
Keywords: bugmon
You need to log in before you can comment on or make changes to this bug.