1114587 - Crash [@ js::AbstractFramePtr::asRematerializedFrame] or [@ js::Debugger::detachAllDebuggersFromGlobal] with heap-use-after-free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision b915a50bc6be (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --target=i686-pc-linux-gnu --without-intl-api --enable-posix-nspr-emulation --disable-debug, run with --fuzzing-safe --thread-count=2 --ion-eager --ion-offthread-compile=off):


var lfcode = new Array();
lfcode.push("");
lfcode.push("");
lfcode.push("\
var g = newGlobal();\
g.debuggeeGlobal = this;\
g.eval('(' + function () {\
        dbg = new Debugger(debuggeeGlobal);\
        dbg.onExceptionUnwind = function (frame, exc) {\
            var s = '!';\
            for (var f = frame; f; f = f.older)\
                    s += f.callee.name;\
        };\
    } + ')();');\
Debugger(17) = this;\
");
while (true) {
  var file = lfcode.shift(); if (file == undefined) { break; }
  loadFile(file)
}
function loadFile(lfVarx) {
  if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
    function newFunc(x) { new Function(x)(); }; newFunc(lfVarx); 
  }
}



Backtrace:

==17519==ERROR: AddressSanitizer: heap-use-after-free on address 0xf4e032a4 at pc 0x943ea44 bp 0xffc37518 sp 0xffc3750c
READ of size 4 at 0xf4e032a4 thread T0
    #0 0x943ea43 in js::AbstractFramePtr::asRematerializedFrame() const js/src/jit/RematerializedFrame.h:139
    #1 0x943ea43 in js::AbstractFramePtr::script() const js/src/vm/Stack-inl.h:631
    #2 0x943ea43 in js::Debugger::removeDebuggeeGlobal(js::FreeOp*, js::GlobalObject*, js::detail::HashTable<js::GlobalObject* const, js::HashSet<js::GlobalObject*, js::DefaultHasher<js::GlobalObject*>, js::SystemAllocPolicy>::SetOps, js::SystemAllocPolicy>::Enum*) js/src/vm/Debugger.cpp:3026
    #3 0x943f09b in js::Debugger::detachAllDebuggersFromGlobal(js::FreeOp*, js::GlobalObject*) js/src/vm/Debugger.cpp:2302
    #4 0x8fc7289 in JSCompartment::sweepGlobalObject(js::FreeOp*) js/src/jscompartment.cpp:565
    #5 0x90c68e6 in js::gc::GCRuntime::beginSweepingZoneGroup() js/src/jsgc.cpp:5067
    #6 0x90ca9a5 in js::gc::GCRuntime::beginSweepPhase(bool) js/src/jsgc.cpp:5236
    #7 0x90d610a in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:5953
    #8 0x90d7e67 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6156
    #9 0x90d92f7 in js::gc::GCRuntime::collect(bool, js::SliceBudget&, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6281
    #10 0x9052c3d in js::gc::GCRuntime::gc(js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6327
    #11 0x8f649b9 in js::DestroyContext(JSContext*, js::DestroyContextMode) js/src/jscntxt.cpp:261
    #12 0x8f64585 in JS_DestroyContext(JSContext*) js/src/jsapi.cpp:756
    #13 0x80e9f47 in DestroyContext(JSContext*, bool) js/src/shell/js.cpp:5179
    #14 0x80e9f47 in main js/src/shell/js.cpp:5907
    #15 0xf73c94d2 in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
    #16 0x80d8f24 in _start (js/src/opt32asan/js/src/shell/js+0x80d8f24)

0xf4e032a4 is located 20 bytes inside of 48-byte region [0xf4e03290,0xf4e032c0)
freed by thread T0 here:
    #0 0x80be164 in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x8d5aba0 in js_free(void*) js/src/opt32asan/js/src/../../dist/include/js/Utility.h:122
    #2 0x8d5aba0 in js::jit::RematerializedFrame::FreeInVector(js::Vector<js::jit::RematerializedFrame*, 0u, js::TempAllocPolicy>&) js/src/jit/RematerializedFrame.cpp:102
    #3 0x96b2b44 in js::jit::JitActivation::clearRematerializedFrames() js/src/vm/Stack.cpp:1480
    #4 0x96b25dc in js::jit::JitActivation::~JitActivation() js/src/vm/Stack.cpp:1421
    #5 0x89b179a in EnterIon(JSContext*, js::jit::EnterJitData&) js/src/jit/Ion.cpp:2451
    #6 0x89b179a in js::jit::IonCannon(JSContext*, js::RunState&) js/src/jit/Ion.cpp:2530
    #7 0x94a7826 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:412
    #8 0x9457190 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:641
    #9 0x94eac0e in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:677
    #10 0x8fa1590 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) js/src/jsapi.cpp:4310
    #11 0x8fa1a0a in JS_ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>) js/src/jsapi.cpp:4332
    #12 0x80f3627 in RunFile(JSContext*, JS::Handle<JSObject*>, char const*, _IO_FILE*, bool) js/src/shell/js.cpp:450
    #13 0x80f3627 in Process(JSContext*, JSObject*, char const*, bool) js/src/shell/js.cpp:583
    #14 0x80e9b09 in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) js/src/shell/js.cpp:5320
    #15 0x80e9b09 in Shell(JSContext*, js::cli::OptionParser*, char**) js/src/shell/js.cpp:5559
    #16 0x80e9b09 in main js/src/shell/js.cpp:5898
    #17 0xf73c94d2 in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226

previously allocated by thread T0 here:
    #0 0x80be53a in __interceptor_calloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:90
    #1 0x8d59825 in js_calloc(unsigned int) js/src/opt32asan/js/src/../../dist/include/js/Utility.h:105
    #2 0x8d59825 in _ZL13js_pod_callocIhEPT_j js/src/opt32asan/js/src/../../dist/include/js/Utility.h:569
    #3 0x8d59825 in unsigned char* js::MallocProvider<js::ThreadSafeContext>::pod_calloc<unsigned char>(unsigned int) js/src/vm/MallocProvider.h:106
    #4 0x8d59825 in js::jit::RematerializedFrame::New(JSContext*, unsigned char*, js::jit::InlineFrameIterator&) js/src/jit/RematerializedFrame.cpp:57
    #5 0x8d5a279 in js::jit::RematerializedFrame::RematerializeInlineFrames(JSContext*, unsigned char*, js::jit::InlineFrameIterator&, js::Vector<js::jit::RematerializedFrame*, 0u, js::TempAllocPolicy>&) js/src/jit/RematerializedFrame.cpp:74
    #6 0x96ac168 in js::jit::JitActivation::getRematerializedFrame(JSContext*, js::jit::JitFrameIterator const&, unsigned int) js/src/vm/Stack.cpp:1514
    #7 0x96abb18 in js::FrameIter::ensureHasRematerializedFrame(JSContext*) js/src/vm/Stack.cpp:969
    #8 0x958f5ec in DebuggerFrame_getOlder(JSContext*, unsigned int, JS::Value*) js/src/vm/Debugger.cpp:5354

SUMMARY: AddressSanitizer: heap-use-after-free js/src/jit/RematerializedFrame.h:139 js::AbstractFramePtr::asRematerializedFrame() const



Not marking s-s because this looks like a Debugger problem (which would be sec-moderate at most).

Comment 1

[Nicolas B. Pierron [:nbp]]

I cannot make an asan build, I will try with valgrind later, but this issue is likely related to Bug 1073033.

Comment 2

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue with a debug build running under valgrind.

Comment 3

[Nicolas B. Pierron [:nbp]]

This does not seem related to the recent modification of Scalar Replacement, and I can still reproduce this issue by returning earlier from Scalar Replacement and from Sink, which are the only 2 phases which are adding (most of) recover instructions.

Shu, any idea?

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Bail out in-place for debug mode in exception handler even if there's no pending exception, so that the DebugEpilogue is called.

Sorry Nicolas, my fault here. In bug 1103027 I thought I'd be all clever and
only bail out to baseline for an onExceptionUnwind hook if there's a catchable,
pending exception.

I forgot that the bailout needs to if there's also a RematerializedFrame, since
the Debugger.Frame that holds it as a referent needs to be cleaned up via
DebugEpilogue after the bailout. There's even a comment right there...

Anyways, this patch just undoes the incorrect optimization.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8540930 [details] [diff] [review]
Bail out in-place for debug mode in exception handler even if there's no pending exception, so that the DebugEpilogue is called.

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

::: js/src/jit/JitFrames.cpp
@@ +423,1 @@
>          // We need to bail when there is a catchable exception, and we are the

You meant Bug 1108145, right?

nit: I guess you should also revert this comment ;)

Comment 6

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 7

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

// Randomly chosen test: js/src/tests/browser.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/debug/Frame-identity-04.js
g = newGlobal();
g.eval("function f(n) { if (n) f(n - 1); debugger; }");
dbg = Debugger(g);
dbg.onDebuggerStatement = function(i) {
    for (; i; i = i.older) {}
    x;
};
g.f(1999);

crashes js debug shell on m-c rev 54e902f5e85d with --fuzzing-safe --no-threads --ion-eager at js::Debugger::removeDebuggeeGlobal.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/877e91964ea9
user:        Shu-yu Guo
date:        Mon Dec 15 18:21:08 2014 -0800
summary:     Bug 1108145 - Fix debug mode in-place Ion->Baseline bailout at loop heads. (r=jandem)

Comment 8

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

Attachment: stack for testcase in comment 7

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/68aa6c741e27

Comment 10

[Nicolas B. Pierron [:nbp]]

Should we backport this patch to 36?

Comment 11

[Shu-yu Guo [:shu]]

Yeah, we should.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8540930 [details] [diff] [review]
Bail out in-place for debug mode in exception handler even if there's no pending exception, so that the DebugEpilogue is called.

Approval Request Comment
[Feature/regressing bug #]: bug 1103027
[User impact if declined]: Rare crashes, I guess
[Describe test coverage new/current, TBPL]: on m-c
[Risks and why]: Low, since it's just a bug fix.
[String/UUID change made/needed]: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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