The following test causes an ASan error on mozilla-central revision (no options required): var map = Map([['a', 1]]); for (let [k, v] of map) { break; } Test is reduced from the original test jit-test/tests/collections/Map-iterator-add-remove.js ASan error: ==9144== ERROR: AddressSanitizer heap-use-after-free on address 0x7f41cd87e2a0 at pc 0xabbf39 bp 0x7fff83715500 sp 0x7fff837154f8 WRITE of size 8 at 0x7f41cd87e2a0 thread T0 #0 0xabbf39 in ~Range /home/decoder/LangFuzz/mozilla-central/js/src/builtin/MapObject.cpp:295 #1 0x5d22c5 in JSObject::hasDynamicSlots() const /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/../jsobj.h:413 #2 0x5d1809 in bool js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:318 #3 0x5c8813 in bool js::gc::FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:384 #4 0x5999b2 in js::gc::FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:421 #5 0x59a132 in js::gc::ArenaLists::finalizeNow(js::FreeOp*, js::gc::AllocKind) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:1597 #6 0x599b57 in js::gc::ArenaLists::queueObjectsForSweep(js::FreeOp*) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:1715 #7 0x5c0f72 in BeginSweepPhase(JSRuntime*) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:3542 #8 0x5bb733 in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:4118 #9 0x5a464f in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:4228 #10 0x51482b in void js::Foreground::delete_<JSContext>(JSContext*) /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/./dist/include/js/Utility.h:616 #11 0x42ac44 in DestroyContext(JSContext*, bool) /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4593 #12 0x42a92f in main /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4967 #13 0x7f41cd8aceff in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258 0x7f41cd87e2a0 is located 32 bytes inside of 48-byte region [0x7f41cd87e280,0x7f41cd87e2b0) freed by thread T0 here: #0 0xdd40d1 in __interceptor_free ??:0 #1 0x5d22c5 in JSObject::hasDynamicSlots() const /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/../jsobj.h:413 #2 0x5d1809 in bool js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:318 #3 0x5c8813 in bool js::gc::FinalizeTypedArenas<JSObject>(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:384 #4 0x5999b2 in js::gc::FinalizeArenas(js::FreeOp*, js::gc::ArenaHeader**, js::gc::ArenaList&, js::gc::AllocKind, js::SliceBudget&) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:421 #5 0x59a132 in js::gc::ArenaLists::finalizeNow(js::FreeOp*, js::gc::AllocKind) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:1597 #6 0x599b57 in js::gc::ArenaLists::queueObjectsForSweep(js::FreeOp*) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:1715 #7 0x5c0f72 in BeginSweepPhase(JSRuntime*) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:3542 #8 0x5bb733 in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:4118 #9 0x5a464f in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) /home/decoder/LangFuzz/mozilla-central/js/src/jsgc.cpp:4228 #10 0x51482b in void js::Foreground::delete_<JSContext>(JSContext*) /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/./dist/include/js/Utility.h:616 #11 0x42ac44 in DestroyContext(JSContext*, bool) /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4593 #12 0x42a92f in main /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4967 #13 0x7f41cd8aceff in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258 previously allocated by thread T0 here: #0 0xdd4191 in malloc ??:0 #1 0xac053e in js_malloc /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/./dist/include/js/Utility.h:157 #2 0x67d136 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/decoder/LangFuzz/mozilla-central/js/src/debug64-asan/../jscntxtinlines.h:387 #3 0x67bd69 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:338 #4 0x657a57 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:2405 #5 0x629f4d in js::RunScript(JSContext*, JSScript*, js::StackFrame*) /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:302 #6 0x67f5c8 in js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:488 #7 0x67fc90 in js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) /home/decoder/LangFuzz/mozilla-central/js/src/jsinterp.cpp:525 #8 0x49c549 in JS_ExecuteScript /home/decoder/LangFuzz/mozilla-central/js/src/jsapi.cpp:5500 #9 0x42c19d in Process(JSContext*, JSObject*, char const*, bool) /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:434 #10 0x429836 in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4743 #11 0x42a903 in main /home/decoder/LangFuzz/mozilla-central/js/src/shell/js.cpp:4960 #12 0x7f41cd8aceff in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258 Looks like a use-after-free, marking s-s.

Fwiw, valgrind also sees the error, so that should be sufficient for reproducing.

Yep, reproduces in valgrind for me. Yeah, this is dumb. The GC of course finalizes garbage objects in arbitrary order. When the Map finalizer is called before the MapIterator finalizer, the MapIterator ends up trying to write to a field of the Map, which is already gone. I haven't thought of a pretty way to fix this yet. No shortage of ugly ways.

Any chance of getting a fix here? I'm triaging the remaining test-blockers for ASan and this is one of the last outstanding issues right now.

Sorry. I'll be working on this today.

This blocks Valgrind fuzzing on jsfunfuzz as well.

Make Range objects a tiny bit more robust: allow destroying the Range after the OrderedHashTable is destroyed. The alternative was to make Range objects totally unrobust, and implement all the Range-updating in the JSObjects. That would have been quite a bit uglier, I think, particularly since the links in the Range::next chain should not be strong references.

Comment on attachment 659907 [details] [diff] [review] v1 Review of attachment 659907 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/MapObject.cpp @@ +232,5 @@ > * Ranges remain valid for the lifetime of the OrderedHashTable, even if > + * entries are added or removed or the table is resized. Don't do anything > + * to a Range, except destroy it, after the OrderedHashTable has been > + * destroyed. (We support destroying the two objects in either order to > + * humor the GC, bless its nondeterministic heart.) lol

This needs to be uplifted to mozilla-beta to fix 17.0 - please nominate for branch approval.

Comment on attachment 659907 [details] [diff] [review] v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 725909 User impact if declined: A GC bug, possibly exploitable. Testing completed (on m-c, etc.): On m-c for almost 6 weeks now. Risk to taking this patch (and alternatives if risky): Very low. String or UUID changes made by this patch: None.

Comment on attachment 659907 [details] [diff] [review] v1 Thanks Jason!

Setting checkin-needed for mozilla-beta.

Can this be marked verified now that we have testcases in-testsuite?

Yes, we run this test on try with ASan once a day.

(In reply to Christian Holler (:decoder) from comment #17) > Yes, we run this test on try with ASan once a day. Should have mentioned that we don't do this for any other version than mozilla-central. So the branches are not verified.

Thanks :decoder. I'm flagging this for verification in the Beta.