New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 732511 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Data race in blink::HeapCompact::MovableObjectFixups::Relocate

Project Member Reported by ClusterFuzz, Jun 12 2017

Issue description

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

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.
 
Cc: msrchandra@chromium.org
Components: Blink>MemoryAllocator
Labels: M-61 Test-Predator-Wrong-CLs
Owner: sigbjo...@opera.com
Status: Assigned (was: Untriaged)
Predator and CL did not provide any possible suspects.
Using Code Search for the file, "HeapCompact.cpp" assigning to the concern owner.
Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/ec3056b0028de68485f6e5c4b6d57f225d334605

@sigbjornf -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

Comment 2 by sigbjo...@opera.com, Jun 13 2017

Cc: haraken@chromium.org hongchan@chromium.org keishi@chromium.org rtoy@chromium.org
Components: -Blink>MemoryAllocator Blink>MemoryAllocator>GarbageCollection
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.
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...


Comment 4 by sigbjo...@opera.com, 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.

Comment 5 by sigbjo...@opera.com, Jun 21 2017

Can't have this one linger, https://codereview.chromium.org/2951903003/
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Comment 7 by sigbjo...@opera.com, Jun 23 2017

Status: Fixed (was: Assigned)
Project Member

Comment 8 by ClusterFuzz, 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