1404714 - Crash [@ js::jit::JSJitFrameIter::operator++] with asm.js and inIon

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 41286177c59c (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

g = (function(t, foreign) {
    "use asm";
    var ff = foreign.ff;
    function f() {
        ff()
    }
    return f
})(this, {
    ff: -0 || (this) ? inIon : a &= () => test()
})
function m(f) {
    while (true) {
        f();
    }
}
m(g);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::jit::JSJitFrameIter::operator++ (this=this@entry=0x7fffffffb6d0) at js/src/jit/JSJitFrameIter.cpp:158
#0  js::jit::JSJitFrameIter::operator++ (this=this@entry=0x7fffffffb6d0) at js/src/jit/JSJitFrameIter.cpp:158
#1  0x00000000008943ab in testingFunc_inIon (cx=0x7ffff6948000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:2479
#2  0x000000000055d6c1 in js::CallJSNative (cx=0x7ffff6948000, native=0x894180 <testingFunc_inIon(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#3  0x0000000000551adf in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6948000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:495
#4  0x0000000000551f7d in InternalCall (cx=0x7ffff6948000, args=...) at js/src/vm/Interpreter.cpp:540
#5  0x00000000005520e0 in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:559
#6  0x0000000000dc7e82 in js::wasm::Instance::callImport (this=this@entry=0x7ffff42a5b00, cx=0x7ffff6948000, funcImportIndex=funcImportIndex@entry=0, argc=argc@entry=0, argv=argv@entry=0x7fffffffbfc0, rval=..., rval@entry=...) at js/src/wasm/WasmInstance.cpp:178
#7  0x0000000000dc88aa in js::wasm::Instance::callImport_void (instance=0x7ffff42a5b00, funcImportIndex=0, argc=0, argv=0x7fffffffbfc0) at js/src/wasm/WasmInstance.cpp:265
[...]
#14 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff6948000	140737330315264
rcx	0x0	0
rdx	0x0	0
rsi	0x7ffff6948000	140737330315264
rdi	0x7fffffffb6d0	140737488336592
rbp	0x7fffffffbb60	140737488337760
rsp	0x7fffffffb6b8	140737488336568
r8	0x1	1
r9	0x7ffff435e9df	140737290562015
r10	0x0	0
r11	0x1	1
r12	0x7fffffffb700	140737488336640
r13	0x7fffffffbee0	140737488338656
r14	0x1	1
r15	0x7fffffffb6d0	140737488336592
rip	0x753479 <js::jit::JSJitFrameIter::operator++()+9>
=> 0x753479 <js::jit::JSJitFrameIter::operator++()+9>:	mov    0x8(%rdx),%rax
   0x75347d <js::jit::JSJitFrameIter::operator++()+13>:	movq   $0x0,0x20(%rdi)


Probably shell-only due to interaction with the "inIon" testing function.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2688b23e8e9a
user:        Benjamin Bouvier
date:        Fri Jun 02 19:34:05 2017 +0200
summary:     Bug 1364520: Remove the jitTop optimization; r=jandem

This iteration took 270.092 seconds to run.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Still reproduces after bug 1360211 has landed.

The code in testingFunc_inIon is probably incorrect, see what it does:

    ScriptFrameIter iter(cx);
    if (!iter.done() && iter.isIon()) {
        // Reset the counter of the IonScript's script.
        jit::JSJitFrameIter jitIter(cx);
        ++jitIter;
        jitIter.script()->resetWarmUpResetCounter();
    }

Here what happens is that:
- in ScriptFrameIter ctor, we call FrameIter, which sees the wasm JitActivation
- then ScriptFrameIter::settle() unwinds the wasm JitActivation because it has no scripts
- then the next JitActivation (the call to wasm) has been jitted
- but the call to jit::JSJitFrameIter ctor uses the cx->activation(), which is the wasm JitActivation!

I think it should instead use the latest activation that ScriptFrameIter has seen.

The same thing happens a few lines below, where the JSScript* is read from cx->currentScript() and might be incorrect. We have hasScript()/script() on the FrameIter, so let's use this instead.

Not sure the test has much worth, since it's a testing only problem. But I could make one (using inIon() in f, heh).

This passes all the jit tests containing a call to `inIon`.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> The code in testingFunc_inIon is probably incorrect, see what it does:
> 
>     ScriptFrameIter iter(cx);
>     if (!iter.done() && iter.isIon()) {
>         // Reset the counter of the IonScript's script.
>         jit::JSJitFrameIter jitIter(cx);
>         ++jitIter;
>         jitIter.script()->resetWarmUpResetCounter();
>     }
> 
> Here what happens is that:
> - in ScriptFrameIter ctor, we call FrameIter, which sees the wasm
> JitActivation
> - then ScriptFrameIter::settle() unwinds the wasm JitActivation because it
> has no scripts
> - then the next JitActivation (the call to wasm) has been jitted
> - but the call to jit::JSJitFrameIter ctor uses the cx->activation(), which
> is the wasm JitActivation!
> 
> I think it should instead use the latest activation that ScriptFrameIter has
> seen.

No, the behaviour of inIon is to report if the latest frame is in Ion-generated code.

In this case the latest frame is in Wasm, so I guess we could go either way by generating either false or true if we are in a Wasm activation.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8914769 [details] [diff] [review]
fix.patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ +2479,3 @@
>          ++jitIter;
>          jitIter.script()->resetWarmUpResetCounter();
> +    } else if (iter.hasScript() && iter.script()->getWarmUpResetCount() >= 20) {

iter.hasScript() will assert if iter.done() returns true.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix.patch

Alright! I think wasm !== ion, so inIon() called from wasm should return false, unless we've taken the jit exit in wasm (wasm could be considered inJit() though, to some extent).

If we've exited wasm through C++ (i.e. exitFP is marked as wasm), let's just return false. If we've exited wasm through Ion (fast wasm->jit exit that's just been implemented), exitFP isn't set at all (so it's not marked as wasm) and there's going to be a jit frame at the top of the frames stack.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6956f4fcbb79
inIon() should return false when in wasm; r=nbp

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6956f4fcbb79

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5f57ad0a0fb

Comment 9

[Ionuț Goldan [:igoldan]]

This brought some improvements on Beta:

== Change summary for alert #9917 (as of October 05 2017 15:54 UTC) ==

Improvements:

 14%  Strings PerfIsUTF8Fifteen macosx64-nightly opt      2,031.58 -> 1,748.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9917