AuroraMiddleware/v8
1
0
Fork 0
Code Issues 2 Pull Requests Releases Wiki Activity

cppgc: Fix CTP destruction

Browse Source 
Double-checked locking pattern for destruction was missing the acquire
barrier for the initial load.

TSAN complained with a data race where:
T1: ClearAllUsedNodes(), clearing out the node
T2: a. if(GetNodeSafe()) { Lock; ... }
T2: b. operator delete

Since GetNodeSafe() was a relaxed load, operator delete was allowed to
be reordered which raced with ClearAllUsedNodes().

Bug: chromium:1239081, chromium:1242795
Change-Id: I3906555b13cc51538a1a54b7ca481a96d81fd84e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3132264
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76599}
This commit is contained in:
Michael Lippautz 2021-08-31 12:50:26 +02:00 committed by V8 LUCI CQ
parent 0ffc1ba5fc
commit 760682da3e
1 changed files with 9 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
include/cppgc/cross-thread-persistent.h
View File

@ -37,10 +37,11 @@ class CrossThreadPersistentBase : public PersistentBase {
SetNodeSafe(nullptr); SetNodeSafe(nullptr);
} }
// GetNodeSafe() can be used for a thread-safe IsValid() check. // GetNodeSafe() can be used for a thread-safe IsValid() check in a
// double-checked locking pattern. See ~BasicCrossThreadPersistent.
PersistentNode* GetNodeSafe() const { PersistentNode* GetNodeSafe() const {
return reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->load( return reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->load(
std::memory_order_relaxed); std::memory_order_acquire);
} }
// The GC writes using SetNodeSafe() while holding the lock. // The GC writes using SetNodeSafe() while holding the lock.
@ -53,12 +54,12 @@ class CrossThreadPersistentBase : public PersistentBase {
#endif #endif
#ifdef V8_IS_ASAN #ifdef V8_IS_ASAN
__atomic_store(&node_, &value, __ATOMIC_RELAXED); __atomic_store(&node_, &value, __ATOMIC_RELEASE);
#else // !V8_IS_ASAN #else // !V8_IS_ASAN
// Non-ASAN builds can use atomics. This also covers MSVC which does not // Non-ASAN builds can use atomics. This also covers MSVC which does not
// have the __atomic_store intrinsic. // have the __atomic_store intrinsic.
reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->store( reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->store(
value, std::memory_order_relaxed); value, std::memory_order_release);
#endif // !V8_IS_ASAN #endif // !V8_IS_ASAN
#undef V8_IS_ASAN #undef V8_IS_ASAN
@ -76,10 +77,11 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
using PointeeType = T; using PointeeType = T;
~BasicCrossThreadPersistent() { ~BasicCrossThreadPersistent() {
// This implements fast path for destroying empty/sentinel.
//
// Simplified version of `AssignUnsafe()` to allow calling without a // Simplified version of `AssignUnsafe()` to allow calling without a
// complete type `T`. Also performs a thread-safe check for a handle that is // complete type `T`. Uses double-checked locking with a simple thread-safe
// not valid. This implements fast path for destroying empty/sentinel // check for a valid handle based on a node.
// handles.
if (GetNodeSafe()) { if (GetNodeSafe()) {
PersistentRegionLock guard; PersistentRegionLock guard;
const void* old_value = GetValue(); const void* old_value = GetValue();