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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

// 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.

Comment 1

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

Attachment: 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)

Comment 2

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

Attachment: 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)

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 4

[Shu-yu Guo [:shu]]

Sorry, this isn't something I have expertise to debug.

Comment 5

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

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).

Comment 6

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #5)
> involving SavedFrame stack stuff not

-not

Comment 7

[Shu-yu Guo [:shu]]

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.

Comment 8

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

Not sure if we have an available ARM JS folk now, so needinfo? from Jan as well as a fallback.

Comment 9

[Jan de Mooij [:jandem]]

(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.

Comment 10

[Sean Stangl [:sstangl]]

Attachment: 0001-Fix-treating-saved-frame-bit-as-part-of-the-frame-ty.patch

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.

Comment 11

[Sean Stangl [:sstangl]]

OK to include test code in commit?

Bug is long-standing, but probably requires Bug 1028418 to expose bad behavior.

Comment 12

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

I'd defer to sec-approval folks for this - setting needinfo? from Al.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 14

[Sean Stangl [:sstangl]]

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.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Calling this a sec-moderate. This means you don't need sec-approval+ and can just check in.

Comment 16

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab040e254a0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ab040e254a0

Please nominate this for Aurora approval when you get a chance.

Comment 18

[Sean Stangl [:sstangl]]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

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!

Comment 20

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3fec00fc2bd

The form did not appear in the textarea when I selected the flag.