Last Comment Bug 779025 - jit-test/tests/collections/Map-iterator-add-remove.js causes AddressSanitizer heap-use-after-free
: jit-test/tests/collections/Map-iterator-add-remove.js causes AddressSanitizer...
Status: VERIFIED FIXED
[asan][asan-test-failure][asan-test-b...
: regression, sec-critical, testcase, verifyme
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Jason Orendorff [:jorendorff]
: general
Mentors:
: 788364 (view as bug list)
Depends on:
Blocks: jsfunfuzz langfuzz 725909
  Show dependency treegraph
 
Reported: 2012-07-30 19:41 PDT by Christian Holler (:decoder)
Modified: 2012-11-07 18:25 PST (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
+
fixed
fixed
unaffected


Attachments
v1 (9.06 KB, patch)
2012-09-10 16:52 PDT, Jason Orendorff [:jorendorff]
luke: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-07-30 19:41:44 PDT
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.
Comment 1 Christian Holler (:decoder) 2012-07-30 19:45:42 PDT
Fwiw, valgrind also sees the error, so that should be sufficient for reproducing.
Comment 2 Jason Orendorff [:jorendorff] 2012-07-31 08:53:41 PDT
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.
Comment 3 Christian Holler (:decoder) 2012-08-28 07:51:11 PDT
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.
Comment 4 Jason Orendorff [:jorendorff] 2012-09-05 07:37:58 PDT
Sorry. I'll be working on this today.
Comment 5 Gary Kwong [:gkw] [:nth10sd] 2012-09-07 17:56:49 PDT
This blocks Valgrind fuzzing on jsfunfuzz as well.
Comment 6 Jason Orendorff [:jorendorff] 2012-09-10 16:52:56 PDT
Created attachment 659907 [details] [diff] [review]
v1

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 7 Luke Wagner [:luke] 2012-09-11 01:12:30 PDT
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
Comment 8 Jason Orendorff [:jorendorff] 2012-09-12 09:47:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/278080ef7545
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-09-12 18:21:08 PDT
https://hg.mozilla.org/mozilla-central/rev/278080ef7545
Comment 10 Jason Orendorff [:jorendorff] 2012-09-20 13:37:12 PDT
*** Bug 788364 has been marked as a duplicate of this bug. ***
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:52:50 PDT
This needs to be uplifted to mozilla-beta to fix 17.0 - please nominate for branch approval.
Comment 12 Jason Orendorff [:jorendorff] 2012-10-24 10:16:35 PDT
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 13 Alex Keybl [:akeybl] 2012-10-24 15:22:11 PDT
Comment on attachment 659907 [details] [diff] [review]
v1

Thanks Jason!
Comment 14 Gary Kwong [:gkw] [:nth10sd] 2012-10-24 15:24:12 PDT
Setting checkin-needed for mozilla-beta.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-10-25 17:00:52 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/4bb01221995f
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-06 14:28:59 PST
Can this be marked verified now that we have testcases in-testsuite?
Comment 17 Christian Holler (:decoder) 2012-11-07 07:17:44 PST
Yes, we run this test on try with ASan once a day.
Comment 18 Christian Holler (:decoder) 2012-11-07 07:18:16 PST
(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.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-07 11:13:32 PST
Thanks :decoder. I'm flagging this for verification in the Beta.

Note You need to log in before you can comment on or make changes to this bug.