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

VERIFIED FIXED in Firefox 51

Status

()

defect
P1
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: h4writer)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla53
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 wontfix, thunderbird_esr45 affected, firefox50 wontfix, firefox51+ verified, firefox52+ verified, firefox53+ verified)

Details

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

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 2

2 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/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?
Blocks: 1171945
Flags: needinfo?(hv1989)
(Assignee)

Comment 3

2 years ago
Posted 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]
Patch

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

Makes sense.
Attachment #8821194 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 6

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

Updated

2 years ago
Keywords: sec-low
(Assignee)

Comment 7

2 years ago
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]:
/
Attachment #8821194 - Flags: approval-mozilla-beta?
Attachment #8821194 - Flags: approval-mozilla-aurora?
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.
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:
https://hg.mozilla.org/mozilla-central/rev/05f02bc144b1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release

Comment 12

2 years ago
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
You need to log in before you can comment on or make changes to this bug.