Data race in blink::HeapCompact::MovableObjectFixups::Relocate |
|||
Issue descriptionDetailed report: https://clusterfuzz.com/testcase?key=4912800676970496 Fuzzer: inferno_twister Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Data race WRITE 8 Crash Address: 0x7eb199a35b88 Crash State: blink::HeapCompact::MovableObjectFixups::Relocate blink::HeapCompact::Relocate blink::NormalPage::SweepAndCompact Sanitizer: thread (TSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=478327:478347 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4912800676970496 Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Jun 13 2017
Hmm, if I read the report correctly, the audio thread manages to deref a CrossThreadPersistent<>, a GC strikes just after (which does heap compaction), with the audio thread concurrently using the object pointer it got back from the CTP -- it tries to index a heap vector of that object (which heap compaction is trying to relocate.) TSan is brilliantly awesome - not sure how to effectively address, at least not right away.
,
Jun 14 2017
In general it's dangerous to defef CTP. Before talking about the heap compaction, the following scenario may happen: 1) The audio thread derefs CTP and stores the pointer to an on-stack raw pointer. 2) The main thread runs a GC. The stack of the audio thread is not scanned. The main thread collects the object. 3) The audio thread accesses the object via the on-stack raw pointer. Crash. Ideally we should probably remove CTP::Get() entirely...
,
Jun 14 2017
Yes, we've been preventing 2) for these auxiliary threads, like audio, by ensuring that there is some other implicit (or explicit, via a persistent) reference keeping the objects alive should a GC happen. Pragmatically, you could introduce GC / CTP recursive lock scopes over the heap object access.
,
Jun 21 2017
Can't have this one linger, https://codereview.chromium.org/2951903003/
,
Jun 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ad39770967ebd93314070776ffe92d3eaeb8a6b commit 2ad39770967ebd93314070776ffe92d3eaeb8a6b Author: sigbjornf <sigbjornf@opera.com> Date: Fri Jun 23 05:28:48 2017 Hold global GC heap lock while making audio thread access. For auxillary threads that rarely need to gain access to another thread's GC heap, we have to ensure that this happens while the heap-owning thread isn't concurrently GCing the heap. Otherwise there is the possibility that heap objects might be relocated or mutated while the auxillary thread tries to access. A CrossThreadPersistent<T> (CTP) ensures reference liveness across threads, but isn't sufficient to handle the wanted exclusive access after a non-attached thread has deref'ed the persistent. So, for this to happen, keep the global CTP lock while accessing a heap object during offline audio rendering -- it specifically accessing heap objects while a GC runs. As all GCs hold the lock on the global CTP region while they run, this ensures exclusion. It is clearly desirable to have all heap access be under the control of the heap-owning thread, and threaded code should try hard to avoid accessing another thread's heap objects. The CTP global lock is the mechanism to use when that isn't practically feasible -- feel free to add a "TODO(foo): avoid using" next to any instances that you end up introducing. As regards audio thread cross-thread usage, the code needs auditing to check if there are other places where setting up this CTP lock is required. R=haraken,rtoy,hongchan BUG= 732511 Review-Url: https://codereview.chromium.org/2951903003 Cr-Commit-Position: refs/heads/master@{#481809} [modify] https://crrev.com/2ad39770967ebd93314070776ffe92d3eaeb8a6b/third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp [modify] https://crrev.com/2ad39770967ebd93314070776ffe92d3eaeb8a6b/third_party/WebKit/Source/platform/heap/PersistentNode.h [modify] https://crrev.com/2ad39770967ebd93314070776ffe92d3eaeb8a6b/third_party/WebKit/Source/platform/heap/ThreadState.cpp
,
Jun 23 2017
,
Jun 24 2017
ClusterFuzz has detected this issue as fixed in range 481807:481809. Detailed report: https://clusterfuzz.com/testcase?key=4912800676970496 Fuzzer: inferno_twister Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Data race WRITE 8 Crash Address: 0x7eb199a35b88 Crash State: blink::HeapCompact::MovableObjectFixups::Relocate blink::HeapCompact::Relocate blink::NormalPage::SweepAndCompact Sanitizer: thread (TSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=478327:478347 Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=481807:481809 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4912800676970496 See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page. |
|||
►
Sign in to add a comment |
|||
Comment 1 by msrchandra@chromium.org
, Jun 13 2017Components: Blink>MemoryAllocator
Labels: M-61 Test-Predator-Wrong-CLs
Owner: sigbjo...@opera.com
Status: Assigned (was: Untriaged)