Closed Bug 1293258 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 + verified

People

(Reporter: decoder, Assigned: sandervv)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(1 file, 4 obsolete files)

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.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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
Jan/Waldo, is this probably related to bug 1279992?
Blocks: 1279992
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jdemooij)
I think that's certain enough I'll let sandervv look into this mostly on blind faith.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jdemooij) → needinfo?(sandervv)
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.
Assignee: nobody → sandervv
Flags: needinfo?(sandervv)
Attachment #8779245 - Flags: review?(jdemooij)
Add a try/catch block to assert the expected recursion.
Attachment #8779245 - Attachment is obsolete: true
Attachment #8779245 - Flags: review?(jdemooij)
Attachment #8779271 - Flags: review?(jdemooij)
Why can't this code use AllocateObjectBuffer<uint8_t> ?
Flags: needinfo?(sandervv)
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|.
Attachment #8779271 - Attachment is obsolete: true
Attachment #8779271 - Flags: review?(jdemooij)
Flags: needinfo?(sandervv)
Attachment #8779313 - Flags: review?(jdemooij)
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.
Attachment #8779313 - Flags: review?(jdemooij)
Attachment #8779313 - Attachment is obsolete: true
Attachment #8780082 - Flags: review?(jdemooij)
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.
Attachment #8780082 - Flags: review?(jdemooij) → review+
Fixed nits in patch
Attachment #8780082 - Attachment is obsolete: true
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/810a43be36de
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Tracking 51+ for this sec-critical issue.
Group: javascript-core-security → core-security-release
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.
Flags: needinfo?(sandervv)
Hi Jan, see comment 19. Adding ni? to you based on this:

https://bugzilla.mozilla.org/show_bug.cgi?id=1243151#c20
Flags: needinfo?(jdemooij)
No longer blocks: 1243151
(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.
Flags: needinfo?(sandervv)
Flags: needinfo?(jdemooij)
Group: core-security-release
Duplicate of this bug: 1324298
You need to log in before you can comment on or make changes to this bug.