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

VERIFIED FIXED in Firefox 47

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: decoder, Assigned: fitzgen)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla47
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 unaffected, firefox47 verified, firefox-esr38 unaffected, firefox-esr45 unaffected)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

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.
Not sure but I think this is a regression from bug 1233857.
Flags: needinfo?(nfitzgerald)
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
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.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Flags: needinfo?(nfitzgerald)
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 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|.
Attachment #8717022 - Flags: review?(jdemooij) → review+
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.
Attachment #8717022 - Attachment is obsolete: true
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
Attachment #8717919 - Flags: review+
Attachment #8717519 - Attachment is obsolete: true
Attachment #8717919 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a5695c65fe1a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Blocks: 1233857
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.