990071 - GenerationalGC: ASan heap-use-after-free crash [@ JS::Value::isGCThing()] with OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central built with --enable-exact-rooting --enable-gcgenerational, revision 0e19655e93df (run with --fuzzing-safe):


var appendToActual = function(s) {}
function test() {
function f() {
  var a = arguments;
  appendToActual(
	a.callee
	);
  appendToActual(
	);
}
for (var i = 0; i < 10; ++i)
  f((0), 2, 3);
for (var i = 0; i < 10; oomAfterAllocations(5))
  f({}, 'a');
} test();

Comment 1

[Christian Holler (:decoder)]

Last OOM trace:

Breakpoint 1, js_failedAllocBreakpoint () at ../../dist/include/js/Utility.h:75
75      static MOZ_NEVER_INLINE void js_failedAllocBreakpoint() { asm(""); }
#0  js_failedAllocBreakpoint () at ../../dist/include/js/Utility.h:75
#1  0x0846895e in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::ValueEdge>::compactRemoveDuplicates (this=<optimized out>, owner=<optimized out>) at ../../dist/include/js/Utility.h:102
#2  0x08469ad7 in compact (this=0xf5200c14, owner=0xf5200c14, this=0xf5200c14, owner=0xf5200c14) at gc/StoreBuffer.h:135
#3  js::gc::StoreBuffer::RelocatableMonoTypeBuffer<js::gc::StoreBuffer::ValueEdge>::compact (this=<optimized out>, owner=<optimized out>) at gc/StoreBuffer.h:213
#4  0x08465a64 in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::ValueEdge>::mark (this=<optimized out>, trc=<optimized out>, owner=<optimized out>) at gc/StoreBuffer.cpp:145
#5  0x083b5ab8 in markValues (this=0xf5200c14, trc=0xffffc870) at gc/StoreBuffer.h:447
#6  js::Nursery::collect (rt=<optimized out>, reason=<optimized out>, pretenureTypes=<optimized out>, this=<optimized out>) at gc/Nursery.cpp:700
#7  0x08d19eae in Collect (rt=0x57ef, incremental=<optimized out>, budget=0, gckind=<optimized out>, reason=JS::gcreason::DESTROY_CONTEXT) at jsgc.cpp:5076
#8  0x08d149ba in js::GC (rt=<optimized out>, gckind=<optimized out>, reason=<optimized out>, rt=<optimized out>, gckind=<optimized out>, reason=<optimized out>) at jsgc.cpp:5001
#9  0x08be77b8 in js::DestroyContext (cx=<optimized out>, mode=<optimized out>) at jscntxt.cpp:264
#10 0x08be7266 in JS_DestroyContext (cx=0xf6866200) at jsapi.cpp:775
#11 0x080dcbdb in DestroyContext (cx=<optimized out>, withGC=255) at shell/js.cpp:5587
#12 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at shell/js.cpp:6207



ASan crash trace:

==30694==ERROR: AddressSanitizer: heap-use-after-free on address 0xf5806528 at pc 0x84660ff bp 0xffffc738 sp 0xffffc72c
READ of size 8 at 0xf5806528 thread T0
    #0 0x84660fe in JS::Value::isGCThing() const opt32asan-oom/js/src/../../dist/include/js/Value.h:1078:16
    #1 0x84660fe in js::gc::StoreBuffer::ValueEdge::deref() const gc/StoreBuffer.h:263
    #2 0x84660fe in js::gc::StoreBuffer::ValueEdge::isNullEdge() const gc/StoreBuffer.h:271
    #3 0x84660fe in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::ValueEdge>::mark(js::gc::StoreBuffer*, JSTracer*) gc/StoreBuffer.cpp:160
    #4 0x83b5ab7 in js::gc::StoreBuffer::markValues(JSTracer*) gc/StoreBuffer.h:447
    #5 0x83b5ab7 in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::types::TypeObject*, 0u, js::SystemAllocPolicy>*) gc/Nursery.cpp:700
    #6 0x8d19ead in js::MinorGC(JSRuntime*, JS::gcreason::Reason) jsgc.cpp:5076
    #7 0x8d19ead in Collect(JSRuntime*, bool, long long, js::JSGCInvocationKind, JS::gcreason::Reason) jsgc.cpp:4949
    #8 0x8d149b9 in js::GC(JSRuntime*, js::JSGCInvocationKind, JS::gcreason::Reason) jsgc.cpp:5001
    #9 0x8be77b7 in js::DestroyContext(JSContext*, js::DestroyContextMode) jscntxt.cpp:264
    #10 0x8be7265 in JS_DestroyContext(JSContext*) jsapi.cpp:775
    #11 0x80dcbda in DestroyContext(JSContext*, bool) shell/js.cpp:5587
    #12 0x80dcbda in main shell/js.cpp:6207
    #13 0xf7cbe4d2 in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
    #14 0x80d72a4 in _start (opt32asan-oom/js/src/shell/js+0x80d72a4)

0xf5806528 is located 24 bytes inside of 44-byte region [0xf5806510,0xf580653c)
freed by thread T0 here:
    #0 0x80bc4e4 in __interceptor_free asan/asan_malloc_linux.cc:64
    #1 0x912ec6c in js_free(void*) opt32asan-oom/js/src/../../dist/include/js/Utility.h:120
    #2 0x912ec6c in js::ArgumentsObject* js::ArgumentsObject::create<CopyFrameArgs>(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSFunction*>, unsigned int, CopyFrameArgs&) vm/ArgumentsObject.cpp:215
    #3 0x9054ab4 in js::ArgumentsObject::createExpected(JSContext*, js::AbstractFramePtr) vm/ArgumentsObject.cpp:237
    #4 0x8b19e72 in js::jit::NewArgumentsObject(JSContext*, js::jit::BaselineFrame*, JS::MutableHandle<JS::Value>) jit/VMFunctions.cpp:813
    #5 0x85fd7cb in EnterBaseline(JSContext*, js::jit::EnterJitData&) jit/BaselineJIT.cpp:121
[...]

previously allocated by thread T0 here:
    #0 0x80bc75c in __interceptor_malloc asan/asan_malloc_linux.cc:74
    #1 0x912e824 in js_malloc(unsigned int) opt32asan-oom/js/src/../../dist/include/js/Utility.h:97
    #2 0x912e824 in js::MallocProvider<js::ThreadSafeContext>::malloc_(unsigned int) vm/MallocProvider.h:53
    #3 0x912e824 in js::ArgumentsObject* js::ArgumentsObject::create<CopyFrameArgs>(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSFunction*>, unsigned int, CopyFrameArgs&) vm/ArgumentsObject.cpp:197
    #4 0x9054ab4 in js::ArgumentsObject::createExpected(JSContext*, js::AbstractFramePtr) vm/ArgumentsObject.cpp:237
    #5 0x8b19e72 in js::jit::NewArgumentsObject(JSContext*, js::jit::BaselineFrame*, JS::MutableHandle<JS::Value>) jit/VMFunctions.cpp:813
    #6 0x85fd7cb in EnterBaseline(JSContext*, js::jit::EnterJitData&) jit/BaselineJIT.cpp:121
[...]

SUMMARY: AddressSanitizer: heap-use-after-free opt32asan-oom/js/src/../../dist/include/js/Value.h:1078 JS::Value::isGCThing() const


Marking s-s and sec-high based on trace and use-after-free.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug990071-argumentsCrash

I haven't been able to reproduce this exact problem (I keep hitting unhandleable OOMs instead), but from looking at the backtrace and the code it seems we are adding store buffer entries when we initialize the arguments object, only to free its memory when JSObeject::create() fails.  Here's a patch for that.

Decoder, do you have some way to test this?

Comment 3

[Christian Holler (:decoder)]

Comment on attachment 8400048 [details] [diff] [review]
bug990071-argumentsCrash

With that patch, the crash changes and I get:

Assertion failure: [unhandlable oom] Failed to allocate for MonoTypeBuffer::put., at js/src/jscntxt.cpp:1373


So I guess that's the right fix :)

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8400048 [details] [diff] [review]
bug990071-argumentsCrash

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

Yup, looks right. r=me. It's annoying how hard it is to get the allocation and initialization ordering just right everywhere.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8400048 [details] [diff] [review]
bug990071-argumentsCrash

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

Difficult as relies on controlling the timing of memory allocation failures.

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

No.

Which older supported branches are affected by this flaw?

None, unless built with GGC enabled.

If not all supported branches, which bug introduced the flaw?

Bug 829421, but it's only been a problem since GGC was enabled in bug 619558.

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

N/A.

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

Low risk.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8400048 [details] [diff] [review]
bug990071-argumentsCrash

Only sec-high or sec-critical bugs that affect more branches than trunk need to request sec-approval.  As this is trunk-only, you can go ahead and land it when you are ready.

Comment 7

[Jon Coppeard (:jonco)]

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

Comment 8

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

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

Comment 9

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on 2014-03-30.
Verified fixed on 2014-06-17.