1293258 - AddressSanitizer: attempting double-free on 0x614000342640 [@ __interceptor_free] or Crash [@ jemalloc_crash] with TypedArray

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision d42aacfe34af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe):

function test() {
    var arr = new Int8Array(400);
    var o = new test(true);
    arr[idx] = 9;
}
test();


Backtrace:

==31413==ERROR: AddressSanitizer: attempting double-free on 0x614000342640 in thread T0:
    #0 0x5109c0 in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38
    #1 0x110d7c1 in js::Class::doFinalize(js::FreeOp*, JSObject*) const dist/include/js/Class.h:815:5
    #2 0x110d7c1 in JSObject::finalize(js::FreeOp*) js/src/jsobjinlines.h:87
    #3 0x110d7c1 in unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) js/src/jsgc.cpp:450
    #4 0x10ee333 in bool FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) js/src/jsgc.cpp:508:26
    #5 0x1085bbd in FinalizeArenas(js::FreeOp*, js::gc::Arena**, js::gc::SortedArenaList&, js::gc::AllocKind, js::SliceBudget&, js::gc::ArenaLists::KeepArenasEnum) js/src/jsgc.cpp:542:1
    #6 0x108316f in js::gc::ArenaLists::backgroundFinalize(js::FreeOp*, js::gc::Arena*, js::gc::Arena**) js/src/jsgc.cpp:2784:5
    #7 0x108d86d in js::gc::GCRuntime::sweepBackgroundThings(js::gc::ZoneList&, js::LifoAlloc&, js::ThreadType) js/src/jsgc.cpp:3214:21
    #8 0x10a36bd in js::gc::GCRuntime::endSweepingZoneGroup() js/src/jsgc.cpp:5156:9
    #9 0x10a671d in js::gc::GCRuntime::sweepPhase(js::SliceBudget&, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:5369:9
    #10 0x10ab087 in js::gc::GCRuntime::incrementalCollectSlice(js::SliceBudget&, JS::gcreason::Reason, js::AutoLockForExclusiveAccess&) js/src/jsgc.cpp:5908:13
    #11 0x10ace04 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6148:5
    #12 0x10ae4b3 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6258:25
    #13 0x108b1a4 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6323:5
    #14 0x159a7ea in JSRuntime::destroyRuntime() js/src/vm/Runtime.cpp:423:9
    #15 0x101a62b in JSContext::~JSContext() js/src/jscntxt.cpp:932:5
    #16 0xfcef9e in void js_delete_poison<JSContext>(JSContext const*) dist/include/js/Utility.h:392:9
    #17 0xfcef9e in js::DestroyContext(JSContext*) js/src/jscntxt.cpp:137
    #18 0x5579f0 in main js/src/shell/js.cpp:7537:5
    #19 0x7f77160d682f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #20 0x465498 in _start (/home/ubuntu/mozilla-central/js/src/dist/bin/js+0x465498)

SUMMARY: AddressSanitizer: double-free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38 in __interceptor_free
==31413==ABORTING

Marking s-s and sec-critical due to double-free.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160805000432" and the hash "505e6acd9c291504700a57ddf7e88f704f65da46".
The "bad" changeset has the timestamp "20160805000922" and the hash "52a0d2d7639717858ce6868c19a37b95e7039736".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=505e6acd9c291504700a57ddf7e88f704f65da46&tochange=52a0d2d7639717858ce6868c19a37b95e7039736

Comment 2

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

Jan/Waldo, is this probably related to bug 1279992?

Comment 3

[Jeff Walden [:Waldo]]

I think that's certain enough I'll let sandervv look into this mostly on blind faith.

Comment 4

[Sander Mathijs van Veen]

Attachment: bug1293258-typed-array-recursion-double-free.patch

Normally, the nursery is used to allocate space for the elements when running in Ion code. However, when to be allocated space does not fit in the nursery anymore, the buffer will be allocated using a malloc and is added to the nursery's malloc buffer list (this is done in |nursery.allocateBuffer|). When the object is tenured, the elements buffer will be freed by the typed array finalizer. To avoid the double free, the malloced buffer should be removed from the malloc buffer list when the object is in the tenured heap, since the finalizer will take care of freeing the buffer.

Comment 5

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=647f360d29c5

Comment 6

[Sander Mathijs van Veen]

Attachment: bug1293258-typed-array-recursion-double-free.patch

Add a try/catch block to assert the expected recursion.

Comment 7

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=651402427d08

Comment 8

[Jan de Mooij [:jandem]]

Why can't this code use AllocateObjectBuffer<uint8_t> ?

Comment 9

[Sander Mathijs van Veen]

Attachment: bug1293258-typed-array-recursion-double-free.patch

Yes, it can. It did use that in a earlier version (hence the name of the method), but I removed it and used the private method |nursery.allocate| to avoid hitting the malloc path in Ion code. I reverted this back to |allocateBuffer| instead of |AllocateObjectBuffer|.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8779313 [details] [diff] [review]
bug1293258-typed-array-recursion-double-free.patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +1062,5 @@
>      if (nbytes > Nursery::MaxNurseryBufferSize)
>          return;
>  
>      Nursery& nursery = cx->runtime()->gc.nursery;
> +    void* buf = AllocateObjectBuffer<uint8_t>(cx, nbytes);

Hm I didn't realize this reports OOM on failure, which is not what we want here. What if we use:

  void* buf = nursery.allocateBuffer(obj, nbytes);

Then you shouldn't need the nursery.isInside checks below, because allocateBuffer does the right thing (malloc or nursery allocation) based on whether the object is in the nursery. We shouldn't have to duplicate the nursery logic here I think.

Comment 11

[Sander Mathijs van Veen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb48fa5596f

Comment 12

[Sander Mathijs van Veen]

Attachment: bug1293258-typed-array-recursion-double-free.patch

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8780082 [details] [diff] [review]
bug1293258-typed-array-recursion-double-free.patch

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

Thanks!

::: js/src/vm/TypedArrayObject.cpp
@@ +85,5 @@
>      setFixedSlot(TypedArrayObject::BYTEOFFSET_SLOT, Int32Value(0));
>  
>      // Free the data slot pointer if has no inline data
>      Nursery& nursery = cx->runtime()->gc.nursery;
> +    if (isTenured() && !hasBuffer() && !hasInlineElements() &&

Nit: Can you add a sentence to the comment above, something like: If the object is in the nursery, the buffer will be freed by the next nursery GC.

Also while you're here, please fix the comment "if has no inline data" -> "if the object has no inline data.", or something.

@@ +116,5 @@
>  
>      // tarray is not shared, because if it were it would have a buffer.
>      memcpy(buffer->dataPointer(), tarray->viewDataUnshared(), tarray->byteLength());
>  
>      // Free the data slot pointer if has no inline data

Same here.

Comment 14

[Sander Mathijs van Veen]

Attachment: bug1293258-typed-array-recursion-double-free.patch

Fixed nits in patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Adjusting the tracking flags since AFAICT, only 51 is affected at the moment. Please change 50 back to affected if I'm misunderstanding.

https://hg.mozilla.org/integration/mozilla-inbound/rev/810a43be36de

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/810a43be36de

Comment 17

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 18

[Marcia Knous [:marcia]]

Tracking 51+ for this sec-critical issue.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you think this fixed bug 1243151? It's marked as a duplicate, but it also seems like it may affect more versions than this issues. Thanks.

Comment 20

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

Hi Jan, see comment 19. Adding ni? to you based on this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1243151#c20

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Matt Wobensmith [:mwobensmith][:matt:] from comment #20)
> Hi Jan, see comment 19. Adding ni? to you based on this:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1243151#c20

That bug was fixed by bug 1295031, not this one. All is good here, no other branches are affected.