961095 - GenerationalGC: ASAN reports use after free in JSObjWrapperKeyMarkCallback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

From https://tbpl.mozilla.org/php/getParsedLog.php?id=33163920&full=1&branch=try#error0 :

04:32:48     INFO -  ==2476==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030008c53a8 at pc 0x7f26a9e83540 bp 0x7fff2a56b050 sp 0x7fff2a56b048
04:32:48     INFO -  READ of size 8 at 0x6030008c53a8 thread T0
04:32:49     INFO -      #0 0x7f26a9e8353f in JSObjWrapperKeyMarkCallback(JSTracer*, void*, void*) /builds/slave/try-l64-asan-00000000000000000/build/dom/plugins/base/nsJSNPRuntime.cpp:948
04:32:49     INFO -      #1 0x7f26ae094b0d in js::gc::StoreBuffer::GenericBuffer::mark(js::gc::StoreBuffer*, JSTracer*) /builds/slave/try-l64-asan-00000000000000000/build/js/src/../../js/src/gc/StoreBuffer.cpp:199
04:32:49     INFO -      #2 0x7f26ae08956a in mark /builds/slave/try-l64-asan-00000000000000000/build/js/src/../../js/src/gc/StoreBuffer.cpp:292
04:32:49     INFO -      #3 0x7f26ae08956a in js::Nursery::collect(JSRuntime*, JS::gcreason::Reason, js::Vector<js::types::TypeObject*, 0ul, js::SystemAllocPolicy>*) /builds/slave/try-l64-asan-00000000000000000/build/js/src/../../js/src/gc/Nursery.cpp:649
04:32:49     INFO -      #4 0x7f26ae6dc808 in MinorGC /builds/slave/try-l64-asan-00000000000000000/build/js/src/../../js/src/jsgc.cpp:5030

[snip]

04:32:49     INFO -  SUMMARY: AddressSanitizer: heap-use-after-free /builds/slave/try-l64-asan-00000000000000000/build/dom/plugins/base/nsJSNPRuntime.cpp:948 JSObjWrapperKeyMarkCallback(JSTracer*, void*, void*)
04:32:49     INFO -  Shadow bytes around the buggy address:
04:32:49     INFO -    0x0c0680110a20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110a30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110a40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110a50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110a60: fa fa fa fa fa fa fd fd fd fa fa fa fa fa fa fa
04:32:49     INFO -  =>0x0c0680110a70: fa fa fd fd fd[fd]fa fa fa fa fa fa fa fa fd fd
04:32:49     INFO -    0x0c0680110a80: fd fa fa fa 00 00 00 fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110a90: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
04:32:49     INFO -    0x0c0680110aa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110ab0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
04:32:49     INFO -    0x0c0680110ac0: fa fa fa fa fa fa fd fd fd fd fa fa fa fa fa fa
04:32:49     INFO -  Shadow byte legend (one shadow byte represents 8 application bytes):
04:32:49     INFO -    Addressable:           00
04:32:49     INFO -    Partially addressable: 01 02 03 04 05 06 07
04:32:49     INFO -    Heap left redzone:     fa
04:32:49     INFO -    Heap right redzone:    fb
04:32:49     INFO -    Freed heap region:     fd
04:32:49     INFO -    Stack left redzone:    f1
04:32:49     INFO -    Stack mid redzone:     f2
04:32:49     INFO -    Stack right redzone:   f3
04:32:49     INFO -    Stack partial redzone: f4
04:32:49     INFO -    Stack after return:    f5
04:32:49     INFO -    Stack use after scope: f8
04:32:49     INFO -    Global redzone:        f9
04:32:49     INFO -    Global init order:     f6
04:32:49     INFO -    Poisoned by user:      f7
04:32:49     INFO -    ASan internal:         fe
04:32:49     INFO -  ==2476==ABORTING
04:32:50  WARNING -  TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug677878.html | application terminated with exit code 1

Comment 1

[Jon Coppeard (:jonco)]

Attachment: plugin-postbarrier-fix

We should be checking that the entry is still in the table before we try to rekey it.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: improve-generic-callbacks

While we're at it, we can improve the generic callbacks in two ways:
1 - push the key type through the API so you don't need to cast it in the callback
2 - check if the key is actually in the nursery before adding a storebuffer entry

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8362596 [details] [diff] [review]
plugin-postbarrier-fix

Review of attachment 8362596 [details] [diff] [review]:
-----------------------------------------------------------------

Ouch! Good find, r=me.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8362598 [details] [diff] [review]
improve-generic-callbacks

Review of attachment 8362598 [details] [diff] [review]:
-----------------------------------------------------------------

Great! r=me

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57f3381cd2cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c513e28c84f

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/57f3381cd2cb
https://hg.mozilla.org/mozilla-central/rev/1c513e28c84f