Closed Bug 1132265 Opened 5 years ago Closed 5 years ago

Assertion failure: entry.isIon() || entry.isBaseline() || entry.isIonCache(), at jit/JitFrames.cpp

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- unaffected
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- wontfix
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: djvj)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

// Randomly chosen test: js/src/jit-test/tests/basic/bug908915.js
(function() {
    function callFromJIT(f) {
        f();
    }

    const fs = [
        function() {
            /* warm up the caller */
        },
        function() {
            enableSPSProfiling();
            enableSingleStepProfiling();
        },
    ];

    for each (let f in fs) {
        callFromJIT(f);
    }
})();

asserts js debug shell on m-c changeset ee093ca70666 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: entry.isIon() || entry.isBaseline() || entry.isIonCache(), at jit/JitFrames.cpp.

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-debug --enable-profiling --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/fuzzing/js/compileShell.py -b "--32 --enable-debug --enable-profiling --enable-nspr-build --enable-more-deterministic --enable-arm-simulator -R ~/trees/mozilla-central" -r ee093ca70666

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/70a8168c7d24
user:        Kannan Vijayan
date:        Thu Jan 15 20:11:21 2015 -0500
summary:     Bug 1057082 - 3/7 - Modify jits to use lastProfilingFrame and lastProfilingCallSite fields. r=jandem

Setting s-s by default because this involves the profiler, see bug 1124036 comment 4. Big thanks to Jesse for helping reduce this testcase.

Kannan, is bug 1057082 a likely regressor?
Flags: needinfo?(kvijayan)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x302164, 0x00503761 js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`js::jit::JitProfilingFrameIterator::tryInitWithTable(this=<unavailable>, table=<unavailable>, pc=<unavailable>, rt=<unavailable>, forLastCallSite=<unavailable>) + 401 at JitFrames.cpp:2892, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00503761 js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`js::jit::JitProfilingFrameIterator::tryInitWithTable(this=<unavailable>, table=<unavailable>, pc=<unavailable>, rt=<unavailable>, forLastCallSite=<unavailable>) + 401 at JitFrames.cpp:2892
    frame #1: 0x0050329c js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`js::jit::JitProfilingFrameIterator::JitProfilingFrameIterator(this=0xbfffe244, rt=0x02041400, state=<unavailable>) + 252 at JitFrames.cpp:2796
    frame #2: 0x00291f6e js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`JS::ProfilingFrameIterator::iteratorConstruct(this=0xbfffe244, state=<unavailable>) + 78 at Stack.cpp:1796
    frame #3: 0x00291e37 js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`JS::ProfilingFrameIterator::ProfilingFrameIterator(this=0xbfffe238, rt=0x02041400, state=<unavailable>) + 103 at Stack.cpp:1737
    frame #4: 0x00016e06 js-dbg-32-prof-dm-nsprBuild-armSim-darwin-ee093ca70666`SingleStepCallback(arg=0xbfffe238, sim=0x0204de00, pc=<unavailable>) + 166 at js.cpp:4174
(lldb)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Flags: needinfo?(kvijayan)
QA Contact: kvijayan
Assignee: nobody → kvijayan
QA Contact: kvijayan
We need to handle dummy frames in tryInitWithTable in ProfilingFrameIterator.
Attachment #8576237 - Flags: review?(shu)
Comment on attachment 8576237 [details] [diff] [review]
fix-bug-1132265.patch

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

Seems okay to me. But why does this happen? Turning on profiling with on-stack frames?
Attachment #8576237 - Flags: review?(shu) → review+
Yeah, specifically triggered by the single-stepper.  Profiling is turned on for a frame.  Typically with Ion, this will cause invalidation of the frame (so we'll bail immediately after we return to the ion frame, and there isn't a high chance of the sampler catching it within that).  With the single stepper turned on, we do profiling at every instruction boundary, so we definitely hit the frame before it gets bailed out, and we see the frameptr with a dummy mapping.
https://hg.mozilla.org/mozilla-central/rev/d8d51e983a8b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Flags: in-testsuite?
It's a security issue that's definitely present, but it's also extremely hard to trigger and only presents when the profiler is enabled.

On the other hand, it's a very low-risk patch that adds a few lines to a single file.  I'd recommend including it in esr38.  No need to expose ourselves when it's easy enough to close off a vector.  However, if we are otherwise pressed for time and need to leave it out, it shouldn't be too big of a deal.

What are the criteria for inclusion in esr38 is what i'm asking, I guess..
Flags: needinfo?(kvijayan) → needinfo?(ryanvm)
sec-moderates aren't slam-dunk approvals for ESR releases, but I've seen them taken before if the patch author feels strongly enough about it. Personally, I'd nominate it and state your case. Can't hurt to ask.
Flags: needinfo?(ryanvm)
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main39+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.