Closed
Bug 1293258
Opened 8 years ago
Closed 8 years ago
AddressSanitizer: attempting double-free on 0x614000342640 [@ __interceptor_free] or Crash [@ jemalloc_crash] with TypedArray
Categories
(Core :: JavaScript Engine, defect)
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)
4.55 KB,
patch
|
Details | Diff | Splinter Review |
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•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years 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?
Comment 3•8 years ago
|
||
I think that's certain enough I'll let sandervv look into this mostly on blind faith.
Flags: needinfo?(jwalden+bmo)
Updated•8 years ago
|
Flags: needinfo?(jdemooij) → needinfo?(sandervv)
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=647f360d29c5
Assignee | ||
Comment 6•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=651402427d08
Comment 8•8 years ago
|
||
Why can't this code use AllocateObjectBuffer<uint8_t> ?
Flags: needinfo?(sandervv)
Assignee | ||
Comment 9•8 years ago
|
||
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•8 years ago
|
Attachment #8779313 -
Flags: review?(jdemooij)
Comment 10•8 years ago
|
||
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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb48fa5596f
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8779313 -
Attachment is obsolete: true
Attachment #8780082 -
Flags: review?(jdemooij)
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Fixed nits in patch
Attachment #8780082 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 15•8 years ago
|
||
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-firefox51:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox51:
--- → ?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/810a43be36de
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 17•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
(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)
Updated•8 years ago
|
Group: core-security-release
Updated•4 years ago
|
Blocks: asan-maintenance
You need to log in
before you can comment on or make changes to this bug.
Description
•