1246593 - Crash [@ JS::Value::isString] or Assertion failure: start >= 0, at gc/StoreBuffer.h:276 with heap-use-after-free

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 76733110704b (build with --enable-gczeal --enable-optimize="-O2 -g" --enable-address-sanitizer --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --disable-debug, run with --fuzzing-safe --ion-offthread-compile=off):

length = 10000;
array = Array();
for (i = length;i > -100000; i--)
  array[i] = {};



Backtrace:

==16348==ERROR: AddressSanitizer: SEGV on unknown address 0x631ffffd3fe0 (pc 0x0000018294f4 sp 0x7fff280d01b0 bp 0x7fff280d01f0 T0)
    #0 0x18294f3 in JS::Value::isString() const js/src/opt64asan/dist/include/js/Value.h:1178:16
    #1 0x18294f3 in _ZN2js13DispatchTypedINS_24TenuringTraversalFunctorIN2JS5ValueEEEJPNS_14TenuringTracerEEEEDTclfp_cvP8JSObjectLDnEspclsr7mozillaE7ForwardIT0_Efp1_EEET_RKS3_DpOS9_ js/src/opt64asan/dist/include/js/Value.h:1886
    #2 0x18294f3 in void js::TenuringTracer::traverse<JS::Value>(JS::Value*) js/src/gc/Marking.cpp:2000
    #3 0x18294f3 in js::TenuringTracer::traceSlots(JS::Value*, JS::Value*) js/src/gc/Marking.cpp:2208
    #4 0x18294f3 in js::HeapSlotArray::operator+(int) const js/src/gc/Nursery.h:78
    #5 0x18294f3 in js::gc::StoreBuffer::SlotsEdge::trace(js::TenuringTracer&) const js/src/gc/Marking.cpp:2045
    #6 0x1851d2b in js::gc::StoreBuffer::MonoTypeBuffer<js::gc::StoreBuffer::SlotsEdge>::trace(js::gc::StoreBuffer*, js::TenuringTracer&) js/src/gc/Marking.cpp:2013
    #7 0x1831b56 in js::gc::StoreBuffer::traceSlots(js::TenuringTracer&) js/src/gc/StoreBuffer.h:466
    #8 0x1831b56 in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, mozilla::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/gc/Nursery.cpp:452
    #9 0x1065f8d in js::gc::GCRuntime::minorGCImpl(JS::gcreason::Reason, mozilla::Vector<js::ObjectGroup*, 0ul, js::SystemAllocPolicy>*) js/src/jsgc.cpp:6684
    #10 0x1065f8d in js::gc::GCRuntime::minorGC(JS::gcreason::Reason) js/src/gc/GCRuntime.h:602
    #11 0xffc22a in js::gc::GCRuntime::gcIfRequested(JSContext*) js/src/jsgc.cpp:6741
    #12 0x1562227 in InvokeInterruptCallback(JSContext*) js/src/vm/Runtime.cpp:565
    #13 0x1562227 in JSRuntime::handleInterrupt(JSContext*) js/src/vm/Runtime.cpp:669
    #14 0xd73641 in js::CheckForInterrupt(JSContext*) js/src/jscntxt.h:669
    #15 0xd73641 in js::jit::InterruptCheck(JSContext*) js/src/jit/VMFunctions.cpp:474


This also aborted with heap-use-after-free before. Marking s-s and sec-high at least.

Comment 1

[Jan de Mooij [:jandem]]

Not sure but I think this is a regression from bug 1233857.

Comment 2

[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/cbb480545732
user:        Nick Fitzgerald
date:        Thu Feb 04 11:18:00 2016 +0100
summary:     Bug 1233857 - Teach the JIT how to put individual elements' edges in the store buffer; r=jandem

This iteration took 232.717 seconds to run.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Ensure that the element index is greater than zero for the PostWriteElementBarrier

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ff593b88061

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8717022 [details] [diff] [review]
Ensure that the element index is greater than zero for the PostWriteElementBarrier

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

::: js/src/jit/VMFunctions.cpp
@@ +623,4 @@
>  {
>      MOZ_ASSERT(!IsInsideNursery(obj));
>      if (obj->is<NativeObject>() &&
> +        index > 0 &&

Maybe we should be checking that 0 <= index < dense-initialized-length here?

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8717022 [details] [diff] [review]
Ensure that the element index is greater than zero for the PostWriteElementBarrier

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

Good catch.

::: js/src/jit/VMFunctions.cpp
@@ +623,4 @@
>  {
>      MOZ_ASSERT(!IsInsideNursery(obj));
>      if (obj->is<NativeObject>() &&
> +        index > 0 &&

|index >= 0| As you suggested, we should probably also check |index < dense-initialized-length|.

Comment 7

[Jan de Mooij [:jandem]]

Forgot to mention this, but if you add the initLength check, you can combine it with the > 0 check, like so:

uint32_t(index) < initLength

If index < 0, this will always be false. The JITs also do this for bounds checks.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Ensure that the element index is greater than zero for the PostWriteElementBarrier

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c647e4ae51de

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Ensure that the element index is greater than zero for the PostWriteElementBarrier

Ok, this iteration ignores the new testcase on SM(cgc) because it makes 101,000
allocations and doing a compacting gc every 100 of those makes the test take
wayyyyy too long.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dcf7523874a

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Ensure that the element index is greater than zero for the PostWriteElementBarrier

Sigh... wrong SM(cgc) timeout whitelist...

Yet Another Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6801fc23f6d3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5695c65fe1a

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.