1605825 - Incorrect assertion in Nursery::forwardBufferPointer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[bo13oy]

Attachment: poc.zip

Hello,

Tested Platforms:

1)Ubuntu 64-bit 16.04.3 (Memory 8G)+ SpiderMonkey Debug version JavaScript-C71.0.1

When using jsshell to load the poc.js, it manifests itself in the form of the following crash:

---cut---
==3014==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x606000022da0 at pc 0x55bfd56e26f1 bp 0x7ffe6957b100 sp 0x7ffe6957b0f8
READ of size 8 at 0x606000022da0 thread T0
#0 0x55bfd56e26f0 in IsWriteableAddress(void*) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:654:11
#1 0x55bfd56e25f6 in js::Nursery::forwardBufferPointer(js::HeapSlot**) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:678:3
#2 0x55bfd5cb1d2b in js::jit::UpdateIonJSFrameForMinorGC(JSRuntime*, js::jit::JSJitFrameIter const&) /builds/worker/workspace/build/src/js/src/jit/JitFrames.cpp:970:15
#3 0x55bfd5cb199e in js::jit::UpdateJitActivationsForMinorGC(JSRuntime*) /builds/worker/workspace/build/src/js/src/jit/JitFrames.cpp:1299:9
#4 0x55bfd56e4bb4 in js::Nursery::doCollection(JS::GCReason, js::gc::TenureCountCache&) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:1069:3
#5 0x55bfd56e3e59 in js::Nursery::collect(JS::GCReason) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:921:5
#6 0x55bfd562833d in js::gc::GCRuntime::minorGC(JS::GCReason, js::gcstats::PhaseKind) /builds/worker/workspace/build/src/js/src/gc/GC.cpp:7544:13
#7 0x55bfd55d0b12 in JSString* js::gc::GCRuntime::tryNewNurseryString<(js::AllowGC)1>(JSContext*, unsigned long, js::gc::AllocKind) /builds/worker/workspace/build/src/js/src/gc/Allocator.cpp:170:23
#8 0x55bfd55d110b in JSString* js::AllocateStringImpl<JSString, (js::AllowGC)1>(JSContext*, js::gc::InitialHeap) /builds/worker/workspace/build/src/js/src/gc/Allocator.cpp:210:16
#9 0x55bfd4b0533a in JSRope* JSRope::new_<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, unsigned long, js::gc::InitialHeap) /builds/worker/workspace/build/src/js/src/vm/StringType-inl.h:172:17
#10 0x55bfd4ec74f9 in JSString* js::ConcatStrings<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType) /builds/worker/workspace/build/src/js/src/vm/StringType.cpp:946:10
#11 0x14fdd0b16898 (<unknown module>)

0x606000022da0 is located 0 bytes to the right of 64-byte region [0x606000022d60,0x606000022da0)
allocated by thread T0 here:
#0 0x55bfd42d66dd in malloc /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:145:3
#1 0x55bfd569f067 in js::HeapSlot* js_pod_arena_malloc<js::HeapSlot>(unsigned long, unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Utility.h:600:26
#2 0x55bfd56d6d47 in js::HeapSlot* js::MallocProvider<JS::Zone>::maybe_pod_malloc<js::HeapSlot>(unsigned long, unsigned long) /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:53:12
#3 0x55bfd564e332 in js::HeapSlot* js::MallocProvider<JS::Zone>::pod_malloc<js::HeapSlot>(unsigned long, unsigned long) /builds/worker/workspace/build/src/js/src/vm/MallocProvider.h:90:12
#4 0x55bfd564de3e in js::TenuringTracer::moveElementsToTenured(js::NativeObject*, js::NativeObject*, js::gc::AllocKind) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:3195:49
#5 0x55bfd564a98c in js::TenuringTracer::moveToTenuredSlow(JSObject*) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:3062:20
#6 0x55bfd564a290 in void js::TenuringTracer::traverse<JSObject>(JSObject**) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2772:11
#7 0x55bfd56d5b0a in ZZN2js14TenuringTracer8traverseIN2JS5ValueEEEvPT_ENKUlTyS4_E_clIP8JSObjectEEDaS4 /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2789:11
#8 0x55bfd56d5578 in ZN2js15MapGCThingTypedIZNS_14TenuringTracer8traverseIN2JS5ValueEEEvPT_EUlTyS5_E_EEDaRKS4_OS5 /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:1273:28
#9 0x55bfd564d067 in void js::TenuringTracer::traverse<JS::Value>(JS::Value*) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2788:18
#10 0x55bfd564d7ea in js::TenuringTracer::traceSlots(JS::Value*, JS::Value*) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2980:5
#11 0x55bfd564c907 in js::TenuringTracer::traceObjectSlots(js::NativeObject*, unsigned int, unsigned int) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2973:5
#12 0x55bfd564b97e in js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer&) const /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2851:11
#13 0x55bfd564b54e in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::SlotsEdge>::trace(js::TenuringTracer&) /builds/worker/workspace/build/src/js/src/gc/Marking.cpp:2807:15
#14 0x55bfd56e4a15 in js::Nursery::doCollection(JS::GCReason, js::gc::TenureCountCache&) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:1027:6
#15 0x55bfd56e3e59 in js::Nursery::collect(JS::GCReason) /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:921:5
#16 0x55bfd562833d in js::gc::GCRuntime::minorGC(JS::GCReason, js::gcstats::PhaseKind) /builds/worker/workspace/build/src/js/src/gc/GC.cpp:7544:13
#17 0x55bfd55d0b12 in JSString* js::gc::GCRuntime::tryNewNurseryString<(js::AllowGC)1>(JSContext*, unsigned long, js::gc::AllocKind) /builds/worker/workspace/build/src/js/src/gc/Allocator.cpp:170:23
#18 0x55bfd55d110b in JSString* js::AllocateStringImpl<JSString, (js::AllowGC)1>(JSContext*, js::gc::InitialHeap) /builds/worker/workspace/build/src/js/src/gc/Allocator.cpp:210:16
#19 0x55bfd4b0533a in JSRope* JSRope::new_<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, unsigned long, js::gc::InitialHeap) /builds/worker/workspace/build/src/js/src/vm/StringType-inl.h:172:17
#20 0x55bfd4ec74f9 in JSString* js::ConcatStrings<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSString*, (js::AllowGC)1>::HandleType) /builds/worker/workspace/build/src/js/src/vm/StringType.cpp:946:10
#21 0x14fdd0b16898 (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/workspace/build/src/js/src/gc/Nursery.cpp:654:11 in IsWriteableAddress(void*)
Shadow bytes around the buggy address:
0x0c0c7fffc560: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c0c7fffc570: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
0x0c0c7fffc580: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
0x0c0c7fffc590: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c0c7fffc5a0: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
=>0x0c0c7fffc5b0: 00 00 00 00[fa]fa fa fa 00 00 00 00 00 00 00 00
0x0c0c7fffc5c0: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c0c7fffc5d0: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
0x0c0c7fffc5e0: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
0x0c0c7fffc5f0: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
0x0c0c7fffc600: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==3014==ABORTING

---cut---

Here's a snippet of the method.
-----source code -------
s
void js::Nursery::forwardBufferPointer(HeapSlot** pSlotsElems) {
HeapSlot* old = *pSlotsElems;

if (!isInside(old)) {
return;
}

// The new location for this buffer is either stored inline with it or in
// the forwardedBuffers table.
do {
if (ForwardedBufferMap::Ptr p = forwardedBuffers.lookup(old)) {
pSlotsElems = reinterpret_cast<HeapSlot>(p->value());
break;
}

*pSlotsElems = *reinterpret_cast<HeapSlot**>(old);

} while (false);

MOZ_ASSERT(!isInside(*pSlotsElems));
MOZ_ASSERT(IsWriteableAddress(*pSlotsElems));//heap-buffer-overflow
}

-----source code -------

Thank you,
bo13oy
Qihoo 360 Vulcan Team.

Comment 1

[Christian Holler (:decoder)]

I was able to reproduce this using ASan debug JS shell builds pretty much immediately, but not in ASan opt JS shell builds at all (both builds from central).

Comment 2

[Jan de Mooij [:jandem]]

It's a debug-only issue I think. What's happening:

• Nursery GC sets a forwarding pointer for object elements.
• Because elements capacity is 0, it's an indirect forwarding pointer (uses a table instead of storing the pointer inline).
• Later in Nursery::forwardBufferPointer, in debug builds we call IsWriteableAddress which dereferences the elements pointer but that's invalid if capacity is 0. We shouldn't call IsWriteableAddress for indirect forwarding pointers.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I think sfink worked on nursery strings, so I'll needinfo him, too.

Comment 4

[Jon Coppeard (:jonco)]

Thanks for looking at this Jan. Yes, we can't assume that we can store a word in the target buffer (for indirect forwarded buffers).

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Bug 1605825 - Indirectly forwarded nursery buffers can be less that a word in size r?jandem

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Just to double check: this is just an overly strict assertion and not a security bug, so we can unhide this?

Comment 7

[Jon Coppeard (:jonco)]

Yes, this is not a security issue.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccfa20c4155e
Indirectly forwarded nursery buffers can be less that a word in size r=jandem

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/ccfa20c4155e