IonMonkey: Assertion failure: false (unexpected type), at ion/Lowering.cpp:983 or Crash [@ js::ion::LIRGenerator::visitToDouble]

VERIFIED FIXED in Firefox 18

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: sstangl)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla18
x86_64
Linux
assertion, crash, sec-moderate, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 unaffected, firefox18 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

(Whiteboard: [jsbugmon:update][ion:p1:fx18][adv-main18-])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
The following testcase asserts on mozilla-central revision e4757379b99a (run with --ion-eager):


function pow1(x) {
    return Math.pow(x, 0.5);
}
assertEq(pow1(NaN), NaN);
function iso(d) {
  return new Date( NaN, "typeof Number(new Number(Number.NaN))", typeof pow1(new pow1(pow1.NaN)))
}
function check(s, millis) {
  description = "Date.parse('"+s+"') == '"+iso(millis)+"'";
}
check("2009-07-23T00:00:00.000-07:00", undefined)
(Reporter)

Comment 1

6 years ago
Valgrind shows:

==47874== Invalid read of size 4
==47874==    at 0x68CBBA: js::ion::LIRGenerator::visitToDouble(js::ion::MToDouble*) (Lowering.cpp:955)
==47874==    by 0x6870BF: js::ion::LIRGenerator::visitInstruction(js::ion::MInstruction*) (Lowering.cpp:1887)
==47874==    by 0x6872E9: js::ion::LIRGenerator::visitBlock(js::ion::MBasicBlock*) (Lowering.cpp:1979)
==47874==    by 0x6875BE: js::ion::LIRGenerator::generate() (Lowering.cpp:2049)
==47874==    by 0x64AE21: js::ion::CompileBackEnd(js::ion::IonBuilder*) (Ion.cpp:854)
==47874==    by 0x64B1FF: js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&) (Ion.cpp:913)
==47874==    by 0x64C70D: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:1022)
==47874==    by 0x64C9FC: js::ion::MethodStatus js::ion::Compile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:1154)
==47874==    by 0x64CB74: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:1252)
==47874==    by 0x4965EF: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:297)
==47874==    by 0x496819: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:378)
==47874==    by 0x496E4C: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==47874==  Address 0x1b29f47ac is not stack'd, malloc'd or (recently) free'd
Blocks: 724444
Keywords: crash
Whiteboard: [jsbugmon:update]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
(Assignee)

Comment 2

6 years ago
Not s-s; in opt builds it will just abort compilation.

The problem here is that TI is telling us that an argument to Math.pow() is a double, because it previously saw a double pass through when called from a different context, even though the current argument is an MCreateThis. We follow TI's advice and wind up in a sticky situation.

One solution is to attempt to detect whether the MIRType of each argument is actually convertible to a double, and not inline in the case of a mismatch. Another solution is to just remove the JS_NOT_REACHED() and let the compilation fail. The former sounds better, but it is invasive and must be done for a number of these functions.
(Assignee)

Updated

6 years ago
Group: core-security
(Assignee)

Updated

6 years ago
Group: core-security
(Assignee)

Comment 3

6 years ago
The crash is from JS_NOT_REACHED affecting opt builds. So far, we have been using JS_NOT_REACHED() as if it were JS_ASSERT() with additional documentation, but JS_NOT_REACHED winds up using __builtin_unreachable() to actually communicate unreachability to the compiler.

We occasionally presume graceful fallback to a |return false| in the JS_NOT_REACHED cases -- these should be changed to JS_ASSERT() where relevant for less exploitability.
(Assignee)

Updated

6 years ago
Assignee: general → sstangl
Status: NEW → ASSIGNED
unhiding based on comment 2.
Group: core-security
Keywords: csec-dos
(Reporter)

Comment 5

6 years ago
S-s based on comment 1 and 3, comment 2 is wrong :)
Group: core-security
Keywords: csec-dos
(Assignee)

Comment 6

6 years ago
Created attachment 667229 [details] [diff] [review]
Patch: Prevent segfaults in opt builds.

This is half of a fix to this bug -- it fixes the segfault in opt builds without fixing the tripping assertion in debug builds.

I would prefer to land this patch first and quickly, since it can manifest as extremely-difficult-to-debug opt crashes in a real browser.
Attachment #667229 - Flags: review?(dvander)
Comment on attachment 667229 [details] [diff] [review]
Patch: Prevent segfaults in opt builds.

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

For some definition of easier to debug - I think the false propagation will be safer but since no error is reported, the callstack will just unwind and disappear.
Attachment #667229 - Flags: review?(dvander) → review+
(Assignee)

Comment 8

6 years ago
(In reply to David Anderson [:dvander] from comment #7)
> For some definition of easier to debug - I think the false propagation will
> be safer but since no error is reported, the callstack will just unwind and
> disappear.

Instead of segfaulting in opt builds in mysterious places, it will silently fail and propagate error. Makes such bugs perf problems instead of sec-critical issues.
It will also be a correctness issue, since entire JS callstack will unwind with a failure return. That is we wouldn't just fail Ion compilation, we'd fail to run the method. But seeing as how we need to fix the underlying bug anyway, and we've still got asserts, it's not a big deal.
(Assignee)

Updated

6 years ago
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:update][ion:p1:fx18][no-close]
(Assignee)

Updated

6 years ago
Whiteboard: [jsbugmon:update][ion:p1:fx18][no-close] → [jsbugmon:update][ion:p1:fx18][leave open]
Keywords: sec-moderate
(Reporter)

Comment 12

6 years ago
Comment 9 suggests this might even be critical, but dvander can probably tell more.
(Assignee)

Comment 13

6 years ago
Created attachment 667730 [details] [diff] [review]
fix

Fix bug by forcing bailout.
Attachment #667730 - Flags: review?(dvander)
Attachment #667730 - Flags: review?(dvander) → review+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/e1c62d51d0ba
Whiteboard: [jsbugmon:update][ion:p1:fx18][leave open] → [jsbugmon:update][ion:p1:fx18]
https://hg.mozilla.org/mozilla-central/rev/e1c62d51d0ba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 16

6 years ago
JSBugMon: This bug has been automatically verified fixed.
status-firefox-esr10: --- → unaffected
status-firefox17: --- → unaffected
status-firefox-esr17: --- → unaffected
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:update][ion:p1:fx18][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.