Commit d98b12d3 authored by Omer Katz's avatar Omer Katz Committed by Commit Bot

cppgc: Add missing guard for PersistentNode allocation.

Two threads might get the same PersistentNode because the
BasicCrossThreadPersistent ctor wasn't taking a lock. Then if one thread
frees the node and the other initalizes it or updates its owner, we get
some random object in our free list of PersistentNodes.

I debug a crash in Assign(Unsafe) and Clear where the PersistentNode
seemed to be allocated on stack. Empirically, adding this guard resolved
it. I can't confirm in the code that the scenario above is what was
happening.

Drive-by: adding a few DCHECKs.

Bug: chromium:1056170
Change-Id: I37d8ed5bb942a124c98d7524b7f04fe8ccb2aefd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2718144
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: 's avatarMichael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73023}
parent 110ec5d2
......@@ -44,6 +44,25 @@ class BasicCrossThreadPersistent final : public PersistentBase,
T* raw, const SourceLocation& loc = SourceLocation::Current())
: PersistentBase(raw), LocationPolicy(loc) {
if (!IsValid(raw)) return;
PersistentRegionLock guard;
PersistentRegion& region = this->GetPersistentRegion(raw);
SetNode(region.AllocateNode(this, &Trace));
this->CheckPointer(raw);
}
class UnsafeCtorTag {
private:
UnsafeCtorTag() = default;
template <typename U, typename OtherWeaknessPolicy,
typename OtherLocationPolicy, typename OtherCheckingPolicy>
friend class BasicCrossThreadPersistent;
};
BasicCrossThreadPersistent( // NOLINT
UnsafeCtorTag, T* raw,
const SourceLocation& loc = SourceLocation::Current())
: PersistentBase(raw), LocationPolicy(loc) {
if (!IsValid(raw)) return;
PersistentRegion& region = this->GetPersistentRegion(raw);
SetNode(region.AllocateNode(this, &Trace));
this->CheckPointer(raw);
......@@ -225,9 +244,12 @@ class BasicCrossThreadPersistent final : public PersistentBase,
BasicCrossThreadPersistent<U, OtherWeaknessPolicy, OtherLocationPolicy,
OtherCheckingPolicy>
To() const {
using OtherBasicCrossThreadPersistent =
BasicCrossThreadPersistent<U, OtherWeaknessPolicy, OtherLocationPolicy,
OtherCheckingPolicy>;
PersistentRegionLock guard;
return BasicCrossThreadPersistent<U, OtherWeaknessPolicy,
OtherLocationPolicy, OtherCheckingPolicy>(
return OtherBasicCrossThreadPersistent(
typename OtherBasicCrossThreadPersistent::UnsafeCtorTag(),
static_cast<U*>(Get()));
}
......
......@@ -30,6 +30,7 @@ class PersistentNode final {
PersistentNode& operator=(const PersistentNode&) = delete;
void InitializeAsUsedNode(void* owner, TraceCallback trace) {
CPPGC_DCHECK(trace);
owner_ = owner;
trace_ = trace;
}
......@@ -89,12 +90,14 @@ class V8_EXPORT PersistentRegion final {
}
PersistentNode* node = free_list_head_;
free_list_head_ = free_list_head_->FreeListNext();
CPPGC_DCHECK(!node->IsUsed());
node->InitializeAsUsedNode(owner, trace);
nodes_in_use_++;
return node;
}
void FreeNode(PersistentNode* node) {
CPPGC_DCHECK(node->IsUsed());
node->InitializeAsFreeNode(free_list_head_);
free_list_head_ = node;
CPPGC_DCHECK(nodes_in_use_ > 0);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment