The default bug view has changed. See this FAQ.

jit-test/tests/collections/Map-iterator-add-remove.js causes AddressSanitizer heap-use-after-free

VERIFIED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 months ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs, 5 keywords)

Trunk
mozilla18
x86_64
Linux
csectype-uaf, regression, sec-critical, testcase, verifyme
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15 unaffected, firefox16 unaffected, firefox17+ fixed, firefox18 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [asan][asan-test-failure][asan-test-blocker][fuzzblocker][js:p1:fx18][adv-track-main17-])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Fwiw, valgrind also sees the error, so that should be sufficient for reproducing.
(Reporter)

Updated

5 years ago
Whiteboard: [asan][asan-test-failure] → [asan][asan-test-failure][asan-test-blocker]
(Assignee)

Comment 2

5 years ago
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.
Keywords: sec-critical
Blocks: 725909
status-firefox16: --- → unaffected
status-firefox17: --- → affected
Assignee: general → jorendorff
status-firefox-esr10: --- → unaffected
status-firefox15: --- → unaffected
tracking-firefox17: --- → +
Keywords: regression
(Reporter)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Sorry. I'll be working on this today.
status-firefox18: --- → affected
This blocks Valgrind fuzzing on jsfunfuzz as well.
Blocks: 349611
Whiteboard: [asan][asan-test-failure][asan-test-blocker] → [asan][asan-test-failure][asan-test-blocker][fuzzblocker]
Whiteboard: [asan][asan-test-failure][asan-test-blocker][fuzzblocker] → [asan][asan-test-failure][asan-test-blocker][fuzzblocker][js:p1:fx18]
(Assignee)

Comment 6

5 years ago
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.
Attachment #659907 - Flags: review?(luke)

Comment 7

5 years ago
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
Attachment #659907 - Flags: review?(luke) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/278080ef7545
https://hg.mozilla.org/mozilla-central/rev/278080ef7545
status-firefox18: affected → fixed
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 788364
This needs to be uplifted to mozilla-beta to fix 17.0 - please nominate for branch approval.
(Assignee)

Comment 12

5 years ago
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.
Attachment #659907 - Flags: approval-mozilla-beta?
Comment on attachment 659907 [details] [diff] [review]
v1

Thanks Jason!
Attachment #659907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting checkin-needed for mozilla-beta.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/4bb01221995f
status-firefox17: affected → fixed
Keywords: checkin-needed
Whiteboard: [asan][asan-test-failure][asan-test-blocker][fuzzblocker][js:p1:fx18] → [asan][asan-test-failure][asan-test-blocker][fuzzblocker][js:p1:fx18][adv-track-main17-]
Can this be marked verified now that we have testcases in-testsuite?
(Reporter)

Comment 17

4 years ago
Yes, we run this test on try with ASan once a day.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 18

4 years ago
(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.
Keywords: verifyme
Group: core-security
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.