Closed Bug 1195590 Opened 5 years ago Closed 5 years ago

Crash [@ js::jit::Simulator::decodeType01] or Assertion failure: Invalid caller frame type when exiting from Ion frame., at jit/MacroAssembler.cpp

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- unaffected
firefox42 --- fixed
firefox43 --- fixed
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: sstangl)

References

Details

(5 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-])

Crash Data

Attachments

(3 files)

// Randomly chosen test: jit-test/tests/saved-stacks/bug-1012646-strlen-crasher.js
function f(x) {
    try {
        eval(x);
    } catch (e) {}
};
f("enableSPSProfilingWithSlowAssertions();");
f("enableTrackAllocations(); throw Error();");

asserts js debug ARM-simulator shell on m-c changeset 9673c75864be with --fuzzing-safe --no-threads --ion-eager at Assertion failure: Invalid caller frame type when exiting from Ion frame., at jit/MacroAssembler.cpp and crashes js opt ARM-simulator builds at js::jit::Simulator::decodeType01

Debug configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-simulator=arm --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build --32 --enable-simulator=arm" -r 9673c75864be

Opt configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-simulator=arm --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build --32 --enable-simulator=arm" -r 9673c75864be

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2729e432104c
user:        Nick Fitzgerald
date:        Tue Jul 28 13:04:56 2015 -0700
summary:     Bug 1028418 - Part 5: Minimize stack walking when capturing SavedFrame stacks with a cache; r=shu

Nick, is bug 1028418 a likely regressor?

This also manifests in native ARM builds, as SIGTRAP crashes.
Flags: needinfo?(nfitzgerald)
Attached file opt stack
(lldb) bt
* thread #1: tid = 0xbcb0d, 0x0047cdcc js-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::decodeType01(this=<unavailable>, instr=<unavailable>) + 4028 at Simulator-arm.cpp:2735, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0047cdcc js-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::decodeType01(this=<unavailable>, instr=<unavailable>) + 4028 at Simulator-arm.cpp:2735
    frame #1: 0x00000036
(lldb)
Attached file debug stack
(lldb) bt 5
* thread #1: tid = 0xbd141, 0x00793bbc js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::decodeType01(this=<unavailable>, instr=<unavailable>) + 5964 at Simulator-arm.cpp:2735, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00793bbc js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::decodeType01(this=<unavailable>, instr=<unavailable>) + 5964 at Simulator-arm.cpp:2735
    frame #1: 0x007904f2 js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::instructionDecode(this=0x0079247e, instr=0x03b000f0) + 802 at Simulator-arm.cpp:4171
    frame #2: 0x008237b6 js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`void js::jit::Simulator::execute<false>(this=0x01e93000) + 134 at Simulator-arm.cpp:4244
    frame #3: 0x00798973 js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::callInternal(this=0x01e93000, entry=0x03b00940) + 227 at Simulator-arm.cpp:4332
    frame #4: 0x00799091 js-dbg-32-dm-nsprBuild-armSim-darwin-9673c75864be`js::jit::Simulator::call(this=0x01e93000, entry=<unavailable>, argument_count=<unavailable>) + 161 at Simulator-arm.cpp:4415
(lldb)
It seems like maybe my patch may have caused some bad interactions with the JIT, but the JIT is largely outside my area of expertise, unfortunately.

Shu, do you have any idea what might be going on here?
Flags: needinfo?(nfitzgerald) → needinfo?(shu)
Sorry, this isn't something I have expertise to debug.
Flags: needinfo?(shu)
If it's SPS profiler, it might be Kannan's world.

Also, I'm setting s-s because it seems it's not just involving SavedFrame stack stuff not, as per the potential regressor in comment 0 (which might not have been correct either).
Group: core-security, javascript-core-security
Flags: needinfo?(kvijayan)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> involving SavedFrame stack stuff not

-not
Well, it looks like SPS toggling causing malformed ARM instructions, probably during toggling stuff in Baseline code. Should get someone who knows our ARM codegen to look.
Not sure if we have an available ARM JS folk now, so needinfo? from Jan as well as a fallback.
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)
> Not sure if we have an available ARM JS folk now, so needinfo? from Jan as
> well as a fallback.

NI Sean, though this does look like a profiler issue.
Flags: needinfo?(jdemooij) → needinfo?(sstangl)
The FrameType mask in ARM profiler code included one extra bit erroneously. This patch used that bit as the "saved frame bit", which caused the read FrameType to be gibberish, from which point we ran off into impossible code.
Flags: needinfo?(sstangl)
Flags: needinfo?(kvijayan)
Attachment #8651902 - Flags: review?(kvijayan)
Attachment #8651902 - Flags: review?(kvijayan) → review+
Assignee: nobody → sstangl
OK to include test code in commit?

Bug is long-standing, but probably requires Bug 1028418 to expose bad behavior.
Flags: needinfo?(gary)
I'd defer to sec-approval folks for this - setting needinfo? from Al.
Flags: needinfo?(gary) → needinfo?(abillings)
Don't ni? me. Ask for sec-approval? on a patch and fill out the form. :-)

That said, this needs a security rating before asking for sec-approval.
Flags: needinfo?(abillings)
Comment on attachment 8651902 [details] [diff] [review]
0001-Fix-treating-saved-frame-bit-as-part-of-the-frame-ty.patch

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?

Very easily, but although the problem existed in-tree for a long time, it probably requires Bug 1028418 to make the bad behavior observable. Without the patchset in that bug, the extra bit that the instruction reads should always be zero, which we assumed it would be.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes, but again it probably is not a security problem on non-

>Which older supported branches are affected by this flaw?

All contain the bug, just on the ARM platform. But they don't contain the code for Bug 1028418, so they probably don't need to be patched.

>Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Yeah, it's the same one-line patch.

>How likely is this patch to cause regressions; how much testing does it need?

Extremely unlikely; none.
Attachment #8651902 - Flags: sec-approval?
Calling this a sec-moderate. This means you don't need sec-approval+ and can just check in.
Keywords: sec-moderate
Attachment #8651902 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/3ab040e254a0

Please nominate this for Aurora approval when you get a chance.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(sstangl)
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Group: javascript-core-security
Group: core-security → core-security-release
Comment on attachment 8651902 [details] [diff] [review]
0001-Fix-treating-saved-frame-bit-as-part-of-the-frame-ty.patch

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

Nominating for Aurora per Comment 17.
Attachment #8651902 - Flags: approval-mozilla-aurora?
Flags: needinfo?(sstangl)
Comment on attachment 8651902 [details] [diff] [review]
0001-Fix-treating-saved-frame-bit-as-part-of-the-frame-ty.patch

Next time, please fill the form even if a sheriff asked you too. We still need the information. Thanks!
Attachment #8651902 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3fec00fc2bd

The form did not appear in the textarea when I selected the flag.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Group: core-security-release
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.