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

VERIFIED FIXED in Firefox 51

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: decoder, Assigned: Sander Mathijs van Veen)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla51
x86_64
Linux
crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51+ verified)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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.

Updated

a year ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

a year ago
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)
(Assignee)

Comment 4

a year ago
Created attachment 8779245 [details] [diff] [review]
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.
Assignee: nobody → sandervv
Flags: needinfo?(sandervv)
Attachment #8779245 - Flags: review?(jdemooij)
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=647f360d29c5
(Assignee)

Comment 6

a year ago
Created attachment 8779271 [details] [diff] [review]
bug1293258-typed-array-recursion-double-free.patch

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)
(Assignee)

Comment 7

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=651402427d08
Why can't this code use AllocateObjectBuffer<uint8_t> ?
Flags: needinfo?(sandervv)
(Assignee)

Comment 9

a year ago
Created attachment 8779313 [details] [diff] [review]
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|.
Attachment #8779271 - Attachment is obsolete: true
Attachment #8779271 - Flags: review?(jdemooij)
Flags: needinfo?(sandervv)
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb48fa5596f
(Assignee)

Comment 12

a year ago
Created attachment 8780082 [details] [diff] [review]
bug1293258-typed-array-recursion-double-free.patch
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+
(Assignee)

Comment 14

a year ago
Created attachment 8780512 [details] [diff] [review]
bug1293258-typed-array-recursion-double-free.patch

Fixed nits in patch
Attachment #8780082 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
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
status-firefox48: --- → unaffected
status-firefox49: --- → unaffected
status-firefox50: affected → unaffected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox51: --- → ?
Keywords: checkin-needed
Blocks: 1243151
https://hg.mozilla.org/mozilla-central/rev/810a43be36de
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified

Comment 17

a year ago
JSBugMon: This bug has been automatically verified fixed.
Tracking 51+ for this sec-critical issue.
tracking-firefox51: ? → +
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)
Depends on: 1296249
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.