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

cppgc: Fix benign data race in CTP destruction

Browse Source 
Consider reading the internal node pointer instead of the actual pointer
when trying to figure out whether a node needs to be destroyed. This
preserves the non-atomiticity of the actual pointer which highlights
races using TSAN while fixing destruction.

Bug: chromium:1239081
Change-Id: I1d1fa29d40d86e4b156269abc90142ee71a8d8f4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3110199
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76415}
This commit is contained in:
Michael Lippautz 2021-08-20 22:34:02 +02:00 committed by V8 LUCI CQ
parent 64d0ceb37a
commit 44f284343b
1 changed files with 31 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -34,7 +34,34 @@ class CrossThreadPersistentBase : public PersistentBase {
V8_CLANG_NO_SANITIZE("address")
void ClearFromGC() const {
raw_ = nullptr;
node_ = nullptr;
SetNodeSafe(nullptr);
}
// GetNodeSafe() can be used for a thread-safe IsValid() check.
PersistentNode* GetNodeSafe() const {
return reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->load(
std::memory_order_relaxed);
}
// The GC writes using SetNodeSafe() while holding the lock.
V8_CLANG_NO_SANITIZE("address")
void SetNodeSafe(PersistentNode* value) const {
#if defined(__has_feature)
#if __has_feature(address_sanitizer)
#define V8_IS_ASAN 1
#endif
#endif
#ifdef V8_IS_ASAN
__atomic_store(&node_, &value, __ATOMIC_RELAXED);
#else // !V8_IS_ASAN
// Non-ASAN builds can use atomics. This also covers MSVC which does not
// have the __atomic_store intrinsic.
reinterpret_cast<std::atomic<PersistentNode*>*>(&node_)->store(
value, std::memory_order_relaxed);
#endif // !V8_IS_ASAN
#undef V8_IS_ASAN
}
};
@ -210,11 +237,10 @@ class BasicCrossThreadPersistent final : public CrossThreadPersistentBase,
*/
void Clear() {
// Simplified version of `Assign()` to allow calling without a complete type
// `T`.
const void* old_value = GetValue();
if (IsValid(old_value)) {
// `T`. Also performs a thread-safe check for a handle that is not valid.
if (GetNodeSafe()) {
PersistentRegionLock guard;
old_value = GetValue();
const void* old_value = GetValue();
// The fast path check (IsValid()) does not acquire the lock. Reload
// the value to ensure the reference has not been cleared.
if (IsValid(old_value)) {