Commit e0c1a349 authored by Michael Lippautz's avatar Michael Lippautz Committed by Commit Bot

cppgc: Properly clear (Weak)Peristent and WeakMember pointers

The CL addresses two issues with (Weak)Persistent and WeakMember:
1. (Weak)Persistent pointers are cleared on heap teardown. Before this
   CL the pointers would contain stale values which could lead to UAF.
2. WeakPersistent and WeakMember are cleared using a combination of
   internal clearing methods and mutable fields which avoids the use
   of const_cast<>.

Bug: chromium:1056170
Change-Id: Ibf2b0f0856771b4f6906608cde13a6d43ebf81f3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2248190Reviewed-by: 's avatarOmer Katz <omerkatz@chromium.org>
Reviewed-by: 's avatarAnton Bikineev <bikineev@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68394}
parent a6394f06
...@@ -56,6 +56,11 @@ class PersistentNode final { ...@@ -56,6 +56,11 @@ class PersistentNode final {
bool IsUsed() const { return trace_; } bool IsUsed() const { return trace_; }
void* owner() const {
CPPGC_DCHECK(IsUsed());
return owner_;
}
private: private:
// PersistentNode acts as a designated union: // PersistentNode acts as a designated union:
// If trace_ != nullptr, owner_ points to the corresponding Persistent handle. // If trace_ != nullptr, owner_ points to the corresponding Persistent handle.
...@@ -67,11 +72,13 @@ class PersistentNode final { ...@@ -67,11 +72,13 @@ class PersistentNode final {
TraceCallback trace_ = nullptr; TraceCallback trace_ = nullptr;
}; };
class V8_EXPORT PersistentRegion { class V8_EXPORT PersistentRegion final {
using PersistentNodeSlots = std::array<PersistentNode, 256u>; using PersistentNodeSlots = std::array<PersistentNode, 256u>;
public: public:
PersistentRegion() = default; PersistentRegion() = default;
// Clears Persistent fields to avoid stale pointers after heap teardown.
~PersistentRegion();
PersistentRegion(const PersistentRegion&) = delete; PersistentRegion(const PersistentRegion&) = delete;
PersistentRegion& operator=(const PersistentRegion&) = delete; PersistentRegion& operator=(const PersistentRegion&) = delete;
......
...@@ -49,12 +49,6 @@ class V8_EXPORT LivenessBroker final { ...@@ -49,12 +49,6 @@ class V8_EXPORT LivenessBroker final {
TraceTrait<T>::GetTraceDescriptor(object).base_object_payload); TraceTrait<T>::GetTraceDescriptor(object).base_object_payload);
} }
template <typename T>
bool IsHeapObjectAlive(const WeakMember<T>& weak_member) const {
return (weak_member != kSentinelPointer) &&
IsHeapObjectAlive<T>(weak_member.Get());
}
template <typename T> template <typename T>
bool IsHeapObjectAlive(const UntracedMember<T>& untraced_member) const { bool IsHeapObjectAlive(const UntracedMember<T>& untraced_member) const {
return (untraced_member != kSentinelPointer) && return (untraced_member != kSentinelPointer) &&
......
...@@ -19,19 +19,43 @@ class Visitor; ...@@ -19,19 +19,43 @@ class Visitor;
namespace internal { namespace internal {
class MemberBase {
protected:
MemberBase() = default;
explicit MemberBase(void* value) : raw_(value) {}
void* const* GetRawSlot() const { return &raw_; }
void* GetRaw() const { return raw_; }
void SetRaw(void* value) { raw_ = value; }
void* GetRawAtomic() const {
return reinterpret_cast<const std::atomic<void*>*>(&raw_)->load(
std::memory_order_relaxed);
}
void SetRawAtomic(void* value) {
reinterpret_cast<std::atomic<void*>*>(&raw_)->store(
value, std::memory_order_relaxed);
}
void ClearFromGC() const { raw_ = nullptr; }
private:
mutable void* raw_ = nullptr;
};
// The basic class from which all Member classes are 'generated'. // The basic class from which all Member classes are 'generated'.
template <typename T, typename WeaknessTag, typename WriteBarrierPolicy, template <typename T, typename WeaknessTag, typename WriteBarrierPolicy,
typename CheckingPolicy> typename CheckingPolicy>
class BasicMember : private CheckingPolicy { class BasicMember final : private MemberBase, private CheckingPolicy {
public: public:
using PointeeType = T; using PointeeType = T;
constexpr BasicMember() = default; constexpr BasicMember() = default;
constexpr BasicMember(std::nullptr_t) {} // NOLINT constexpr BasicMember(std::nullptr_t) {} // NOLINT
BasicMember(SentinelPointer s) : raw_(s) {} // NOLINT BasicMember(SentinelPointer s) : MemberBase(s) {} // NOLINT
BasicMember(T* raw) : raw_(raw) { // NOLINT BasicMember(T* raw) : MemberBase(raw) { // NOLINT
InitializingWriteBarrier(); InitializingWriteBarrier();
this->CheckPointer(raw_); this->CheckPointer(Get());
} }
BasicMember(T& raw) : BasicMember(&raw) {} // NOLINT BasicMember(T& raw) : BasicMember(&raw) {} // NOLINT
BasicMember(const BasicMember& other) : BasicMember(other.Get()) {} BasicMember(const BasicMember& other) : BasicMember(other.Get()) {}
...@@ -108,7 +132,7 @@ class BasicMember : private CheckingPolicy { ...@@ -108,7 +132,7 @@ class BasicMember : private CheckingPolicy {
T* Get() const { T* Get() const {
// Executed by the mutator, hence non atomic load. // Executed by the mutator, hence non atomic load.
return raw_; return static_cast<T*>(MemberBase::GetRaw());
} }
void Clear() { SetRawAtomic(nullptr); } void Clear() { SetRawAtomic(nullptr); }
...@@ -120,25 +144,18 @@ class BasicMember : private CheckingPolicy { ...@@ -120,25 +144,18 @@ class BasicMember : private CheckingPolicy {
} }
private: private:
void SetRawAtomic(T* raw) {
reinterpret_cast<std::atomic<T*>*>(&raw_)->store(raw,
std::memory_order_relaxed);
}
T* GetRawAtomic() const { T* GetRawAtomic() const {
return reinterpret_cast<const std::atomic<T*>*>(&raw_)->load( return static_cast<T*>(MemberBase::GetRawAtomic());
std::memory_order_relaxed);
} }
void InitializingWriteBarrier() const { void InitializingWriteBarrier() const {
WriteBarrierPolicy::InitializingBarrier( WriteBarrierPolicy::InitializingBarrier(GetRawSlot(), GetRaw());
reinterpret_cast<const void*>(&raw_), static_cast<const void*>(raw_));
} }
void AssigningWriteBarrier() const { void AssigningWriteBarrier() const {
WriteBarrierPolicy::AssigningBarrier(reinterpret_cast<const void*>(&raw_), WriteBarrierPolicy::AssigningBarrier(GetRawSlot(), GetRaw());
static_cast<const void*>(raw_));
} }
T* raw_ = nullptr; void ClearFromGC() const { MemberBase::ClearFromGC(); }
friend class cppgc::Visitor; friend class cppgc::Visitor;
}; };
......
...@@ -15,14 +15,43 @@ ...@@ -15,14 +15,43 @@
#include "v8config.h" // NOLINT(build/include_directory) #include "v8config.h" // NOLINT(build/include_directory)
namespace cppgc { namespace cppgc {
class Visitor;
namespace internal { namespace internal {
class PersistentBase {
protected:
PersistentBase() = default;
explicit PersistentBase(void* raw) : raw_(raw) {}
void* GetValue() const { return raw_; }
void SetValue(void* value) { raw_ = value; }
PersistentNode* GetNode() const { return node_; }
void SetNode(PersistentNode* node) { node_ = node; }
// Performs a shallow clear which assumes that internal persistent nodes are
// destroyed elsewhere.
void ClearFromGC() const {
raw_ = nullptr;
node_ = nullptr;
}
private:
mutable void* raw_ = nullptr;
mutable PersistentNode* node_ = nullptr;
friend class PersistentRegion;
};
// The basic class from which all Persistent classes are generated. // The basic class from which all Persistent classes are generated.
template <typename T, typename WeaknessPolicy, typename LocationPolicy, template <typename T, typename WeaknessPolicy, typename LocationPolicy,
typename CheckingPolicy> typename CheckingPolicy>
class BasicPersistent : public LocationPolicy, class BasicPersistent final : public PersistentBase,
private WeaknessPolicy, public LocationPolicy,
private CheckingPolicy { private WeaknessPolicy,
private CheckingPolicy {
public: public:
using typename WeaknessPolicy::IsStrongPersistent; using typename WeaknessPolicy::IsStrongPersistent;
using PointeeType = T; using PointeeType = T;
...@@ -38,15 +67,15 @@ class BasicPersistent : public LocationPolicy, ...@@ -38,15 +67,15 @@ class BasicPersistent : public LocationPolicy,
BasicPersistent( // NOLINT BasicPersistent( // NOLINT
SentinelPointer s, const SourceLocation& loc = SourceLocation::Current()) SentinelPointer s, const SourceLocation& loc = SourceLocation::Current())
: LocationPolicy(loc), raw_(s) {} : PersistentBase(s), LocationPolicy(loc) {}
// Raw value contstructors. // Raw value constructors.
BasicPersistent(T* raw, // NOLINT BasicPersistent(T* raw, // NOLINT
const SourceLocation& loc = SourceLocation::Current()) const SourceLocation& loc = SourceLocation::Current())
: LocationPolicy(loc), raw_(raw) { : PersistentBase(raw), LocationPolicy(loc) {
if (!IsValid()) return; if (!IsValid()) return;
node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode( SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
this, &BasicPersistent::Trace); .AllocateNode(this, &BasicPersistent::Trace));
this->CheckPointer(Get()); this->CheckPointer(Get());
} }
...@@ -74,13 +103,11 @@ class BasicPersistent : public LocationPolicy, ...@@ -74,13 +103,11 @@ class BasicPersistent : public LocationPolicy,
BasicPersistent( BasicPersistent(
BasicPersistent&& other, BasicPersistent&& other,
const SourceLocation& loc = SourceLocation::Current()) noexcept const SourceLocation& loc = SourceLocation::Current()) noexcept
: LocationPolicy(std::move(other)), : PersistentBase(std::move(other)), LocationPolicy(std::move(other)) {
raw_(std::move(other.raw_)),
node_(std::move(other.node_)) {
if (!IsValid()) return; if (!IsValid()) return;
node_->UpdateOwner(this); GetNode()->UpdateOwner(this);
other.raw_ = nullptr; other.SetValue(nullptr);
other.node_ = nullptr; other.SetNode(nullptr);
this->CheckPointer(Get()); this->CheckPointer(Get());
} }
...@@ -114,13 +141,12 @@ class BasicPersistent : public LocationPolicy, ...@@ -114,13 +141,12 @@ class BasicPersistent : public LocationPolicy,
BasicPersistent& operator=(BasicPersistent&& other) { BasicPersistent& operator=(BasicPersistent&& other) {
if (this == &other) return *this; if (this == &other) return *this;
Clear(); Clear();
PersistentBase::operator=(std::move(other));
LocationPolicy::operator=(std::move(other)); LocationPolicy::operator=(std::move(other));
raw_ = std::move(other.raw_);
node_ = std::move(other.node_);
if (!IsValid()) return *this; if (!IsValid()) return *this;
node_->UpdateOwner(this); GetNode()->UpdateOwner(this);
other.raw_ = nullptr; other.SetValue(nullptr);
other.node_ = nullptr; other.SetNode(nullptr);
this->CheckPointer(Get()); this->CheckPointer(Get());
return *this; return *this;
} }
...@@ -156,7 +182,7 @@ class BasicPersistent : public LocationPolicy, ...@@ -156,7 +182,7 @@ class BasicPersistent : public LocationPolicy,
T* operator->() const { return Get(); } T* operator->() const { return Get(); }
T& operator*() const { return *Get(); } T& operator*() const { return *Get(); }
T* Get() const { return raw_; } T* Get() const { return static_cast<T*>(GetValue()); }
void Clear() { Assign(nullptr); } void Clear() { Assign(nullptr); }
...@@ -176,29 +202,35 @@ class BasicPersistent : public LocationPolicy, ...@@ -176,29 +202,35 @@ class BasicPersistent : public LocationPolicy,
// Ideally, handling kSentinelPointer would be done by the embedder. On the // Ideally, handling kSentinelPointer would be done by the embedder. On the
// other hand, having Persistent aware of it is beneficial since no node // other hand, having Persistent aware of it is beneficial since no node
// gets wasted. // gets wasted.
return raw_ != nullptr && raw_ != kSentinelPointer; return GetValue() != nullptr && GetValue() != kSentinelPointer;
} }
void Assign(T* ptr) { void Assign(T* ptr) {
if (IsValid()) { if (IsValid()) {
if (ptr && ptr != kSentinelPointer) { if (ptr && ptr != kSentinelPointer) {
// Simply assign the pointer reusing the existing node. // Simply assign the pointer reusing the existing node.
raw_ = ptr; SetValue(ptr);
this->CheckPointer(ptr); this->CheckPointer(ptr);
return; return;
} }
WeaknessPolicy::GetPersistentRegion(raw_).FreeNode(node_); WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
node_ = nullptr; SetNode(nullptr);
} }
raw_ = ptr; SetValue(ptr);
if (!IsValid()) return; if (!IsValid()) return;
node_ = WeaknessPolicy::GetPersistentRegion(raw_).AllocateNode( SetNode(WeaknessPolicy::GetPersistentRegion(GetValue())
this, &BasicPersistent::Trace); .AllocateNode(this, &BasicPersistent::Trace));
this->CheckPointer(Get()); this->CheckPointer(Get());
} }
T* raw_ = nullptr; void ClearFromGC() const {
PersistentNode* node_ = nullptr; if (IsValid()) {
WeaknessPolicy::GetPersistentRegion(GetValue()).FreeNode(GetNode());
PersistentBase::ClearFromGC();
}
}
friend class cppgc::Visitor;
}; };
template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1, template <typename T1, typename WeaknessPolicy1, typename LocationPolicy1,
......
...@@ -136,12 +136,11 @@ class Visitor { ...@@ -136,12 +136,11 @@ class Visitor {
template <typename PointerType> template <typename PointerType>
static void HandleWeak(const LivenessBroker& info, const void* object) { static void HandleWeak(const LivenessBroker& info, const void* object) {
const PointerType* weak = static_cast<const PointerType*>(object); const PointerType* weak = static_cast<const PointerType*>(object);
// Sentinel values are preserved for weak pointers.
if (*weak == kSentinelPointer) return;
const auto* raw = weak->Get(); const auto* raw = weak->Get();
if (raw && !info.IsHeapObjectAlive(raw)) { if (!info.IsHeapObjectAlive(raw)) {
// Object is passed down through the marker as const. Alternatives are weak->ClearFromGC();
// - non-const Trace method;
// - mutable pointer in MemberBase;
const_cast<PointerType*>(weak)->Clear();
} }
} }
......
...@@ -7,9 +7,21 @@ ...@@ -7,9 +7,21 @@
#include <algorithm> #include <algorithm>
#include <numeric> #include <numeric>
#include "include/cppgc/persistent.h"
namespace cppgc { namespace cppgc {
namespace internal { namespace internal {
PersistentRegion::~PersistentRegion() {
for (auto& slots : nodes_) {
for (auto& node : *slots) {
if (node.IsUsed()) {
static_cast<PersistentBase*>(node.owner())->ClearFromGC();
}
}
}
}
size_t PersistentRegion::NodesInUse() const { size_t PersistentRegion::NodesInUse() const {
return std::accumulate( return std::accumulate(
nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) { nodes_.cbegin(), nodes_.cend(), 0u, [](size_t acc, const auto& slots) {
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#include "src/heap/cppgc/marker.h" #include "src/heap/cppgc/marker.h"
#include "include/cppgc/allocation.h" #include "include/cppgc/allocation.h"
#include "include/cppgc/internal/pointer-policies.h"
#include "include/cppgc/member.h" #include "include/cppgc/member.h"
#include "include/cppgc/persistent.h" #include "include/cppgc/persistent.h"
#include "src/heap/cppgc/heap-object-header-inl.h" #include "src/heap/cppgc/heap-object-header-inl.h"
...@@ -245,5 +246,20 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) { ...@@ -245,5 +246,20 @@ TEST_F(MarkerTest, InConstructionObjectIsEventuallyMarkedNonEmptyStack) {
}); });
} }
TEST_F(MarkerTest, SentinelNotClearedOnWeakPersistentHandling) {
Marker marker(Heap::From(GetHeap())->AsBase());
Persistent<GCed> root = MakeGarbageCollected<GCed>(GetAllocationHandle());
auto* tmp = MakeGarbageCollected<GCed>(GetAllocationHandle());
root->SetWeakChild(tmp);
static const Marker::MarkingConfig config = {
MarkingConfig::CollectionType::kMajor,
MarkingConfig::StackState::kNoHeapPointers};
marker.StartMarking(config);
marker.FinishMarking(config);
root->SetWeakChild(kSentinelPointer);
marker.ProcessWeakness();
EXPECT_EQ(kSentinelPointer, root->weak_child());
}
} // namespace internal } // namespace internal
} // namespace cppgc } // namespace cppgc
...@@ -8,6 +8,7 @@ ...@@ -8,6 +8,7 @@
#include "include/cppgc/allocation.h" #include "include/cppgc/allocation.h"
#include "include/cppgc/garbage-collected.h" #include "include/cppgc/garbage-collected.h"
#include "include/cppgc/internal/pointer-policies.h"
#include "include/cppgc/member.h" #include "include/cppgc/member.h"
#include "include/cppgc/type-traits.h" #include "include/cppgc/type-traits.h"
#include "src/heap/cppgc/heap.h" #include "src/heap/cppgc/heap.h"
...@@ -70,6 +71,7 @@ class RootVisitor final : public VisitorBase { ...@@ -70,6 +71,7 @@ class RootVisitor final : public VisitorBase {
private: private:
std::vector<std::pair<WeakCallback, const void*>> weak_callbacks_; std::vector<std::pair<WeakCallback, const void*>> weak_callbacks_;
}; };
class PersistentTest : public testing::TestSupportingAllocationOnly {}; class PersistentTest : public testing::TestSupportingAllocationOnly {};
} // namespace } // namespace
...@@ -617,6 +619,25 @@ TEST_F(PersistentTest, TraceWeak) { ...@@ -617,6 +619,25 @@ TEST_F(PersistentTest, TraceWeak) {
EXPECT_EQ(0u, GetRegion<WeakPersistent>(heap).NodesInUse()); EXPECT_EQ(0u, GetRegion<WeakPersistent>(heap).NodesInUse());
} }
TEST_F(PersistentTest, ClearOnHeapDestruction) {
Persistent<GCed> persistent;
WeakPersistent<GCed> weak_persistent;
// Create another heap that can be destroyed during the test.
auto heap = Heap::Create(GetPlatformHandle());
persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
weak_persistent = MakeGarbageCollected<GCed>(heap->GetAllocationHandle());
const Persistent<GCed> persistent_sentinel(kSentinelPointer);
const WeakPersistent<GCed> weak_persistent_sentinel(kSentinelPointer);
heap.reset();
EXPECT_EQ(nullptr, persistent);
EXPECT_EQ(nullptr, weak_persistent);
// Sentinel values survive as they do not represent actual heap objects.
EXPECT_EQ(kSentinelPointer, persistent_sentinel);
EXPECT_EQ(kSentinelPointer, weak_persistent_sentinel);
}
#if CPPGC_SUPPORTS_SOURCE_LOCATION #if CPPGC_SUPPORTS_SOURCE_LOCATION
TEST_F(PersistentTest, LocalizedPersistent) { TEST_F(PersistentTest, LocalizedPersistent) {
GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle()); GCed* gced = MakeGarbageCollected<GCed>(GetAllocationHandle());
......
...@@ -22,6 +22,8 @@ class TestWithPlatform : public ::testing::Test { ...@@ -22,6 +22,8 @@ class TestWithPlatform : public ::testing::Test {
TestPlatform& GetPlatform() const { return *platform_; } TestPlatform& GetPlatform() const { return *platform_; }
std::shared_ptr<TestPlatform> GetPlatformHandle() const { return platform_; }
protected: protected:
static std::shared_ptr<TestPlatform> platform_; static std::shared_ptr<TestPlatform> platform_;
}; };
......
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