From 424911e05d9b184db0cdd1cd2e1ad0ce0ee3e975 Mon Sep 17 00:00:00 2001 From: Michal Klocek Date: Fri, 17 Aug 2018 10:52:43 +0200 Subject: [Backport] CVE-2018-6158 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reland "[oilpan] Fix GCInfoTable for multiple threads" Previously, grow and access from different threads could lead to a race on the table backing; see bug. - Rework the table to work on an existing reservation. - Commit upon growing, avoiding any copies. Reland: - Fix an issue for component builds were the singleton was instantiated multiple times. Bug: chromium:841280 Reviewed-on: https://chromium-review.googlesource.com/1068636 Change-Id: Iaddee6b594d6853dcbb29aec2c29330987c3b6a9 Reviewed-by: Michael Brüning --- .../WebKit/Source/platform/heap/GCInfo.cpp | 117 ++++++++++++++++----- .../WebKit/Source/platform/heap/GCInfo.h | 64 +++++++---- .../WebKit/Source/platform/heap/Heap.cpp | 10 +- .../third_party/WebKit/Source/platform/heap/Heap.h | 13 +-- .../WebKit/Source/platform/heap/HeapPage.cpp | 5 +- 5 files changed, 142 insertions(+), 67 deletions(-) diff --git a/chromium/third_party/WebKit/Source/platform/heap/GCInfo.cpp b/chromium/third_party/WebKit/Source/platform/heap/GCInfo.cpp index 6a2c3b94a2d..449f7349274 100644 --- a/chromium/third_party/WebKit/Source/platform/heap/GCInfo.cpp +++ b/chromium/third_party/WebKit/Source/platform/heap/GCInfo.cpp @@ -9,20 +9,64 @@ namespace blink { -// GCInfo indices start from 1 for heap objects, with 0 being treated -// specially as the index for freelist entries and large heap objects. -size_t GCInfoTable::gc_info_index_ = 0; +namespace { + +inline uintptr_t RoundUpToPageAllocationGranularity(uintptr_t address) { + return (address + base::kPageAllocationGranularityOffsetMask) & + base::kPageAllocationGranularityBaseMask; +} + +constexpr inline bool IsPowerOfTwo(size_t value) { + return value > 0 && (value & (value - 1)) == 0; +} + +constexpr size_t kEntrySize = sizeof(GCInfo*); + +// Allocation and resizing are built around the following invariants. +static_assert(IsPowerOfTwo(kEntrySize), + "GCInfoTable entries size must be power of " + "two"); +static_assert( + 0 == base::kPageAllocationGranularity % base::kSystemPageSize, + "System page size must be a multiple of page page allocation granularity"); + +size_t ComputeInitialTableLimit() { + // (Light) experimentation suggests that Blink doesn't need more than this + // while handling content on popular web properties. + constexpr size_t kInitialWantedLimit = 512; + + // Different OSes have different page sizes, so we have to choose the minimum + // of memory wanted and OS page size. + + constexpr size_t memory_wanted = kInitialWantedLimit * kEntrySize; + return RoundUpToPageAllocationGranularity(memory_wanted) / kEntrySize; +} -size_t GCInfoTable::gc_info_table_size_ = 0; -GCInfo const** g_gc_info_table = nullptr; + +size_t MaxTableSize() { + static const size_t kMaxTableSize = + RoundUpToPageAllocationGranularity(GCInfoTable::kMaxIndex * kEntrySize); + return kMaxTableSize; +} + +} // namespace + +GCInfoTable* GCInfoTable::global_table_ = nullptr; + +void GCInfoTable::CreateGlobalTable() { + DEFINE_STATIC_LOCAL(GCInfoTable, table, ()); + global_table_ = &table; +} void GCInfoTable::EnsureGCInfoIndex(const GCInfo* gc_info, size_t* gc_info_index_slot) { DCHECK(gc_info); DCHECK(gc_info_index_slot); - // Keep a global GCInfoTable lock while allocating a new slot. - DEFINE_THREAD_SAFE_STATIC_LOCAL(Mutex, mutex, ()); - MutexLocker locker(mutex); + + // Ensuring a new index involves current index adjustment as well + // as potentially resizing the table, both operations that require + // a lock. + MutexLocker locker(table_mutex_); // If more than one thread ends up allocating a slot for // the same GCInfo, have later threads reuse the slot @@ -30,37 +74,52 @@ void GCInfoTable::EnsureGCInfoIndex(const GCInfo* gc_info, if (*gc_info_index_slot) return; - int index = ++gc_info_index_; + int index = ++current_index_; size_t gc_info_index = static_cast(index); CHECK(gc_info_index < GCInfoTable::kMaxIndex); - if (gc_info_index >= gc_info_table_size_) + if (current_index_ >= limit_) Resize(); - g_gc_info_table[gc_info_index] = gc_info; + table_[gc_info_index] = gc_info; ReleaseStore(reinterpret_cast(gc_info_index_slot), index); } void GCInfoTable::Resize() { - static const int kGcInfoZapValue = 0x33; - // (Light) experimentation suggests that Blink doesn't need - // more than this while handling content on popular web properties. - const size_t kInitialSize = 512; - - size_t new_size = - gc_info_table_size_ ? 2 * gc_info_table_size_ : kInitialSize; - DCHECK(new_size < GCInfoTable::kMaxIndex); - g_gc_info_table = - reinterpret_cast(WTF::Partitions::FastRealloc( - g_gc_info_table, new_size * sizeof(GCInfo), "GCInfo")); - DCHECK(g_gc_info_table); - memset(reinterpret_cast(g_gc_info_table) + - gc_info_table_size_ * sizeof(GCInfo), - kGcInfoZapValue, (new_size - gc_info_table_size_) * sizeof(GCInfo)); - gc_info_table_size_ = new_size; + const size_t new_limit = (limit_) ? 2 * limit_ : ComputeInitialTableLimit(); + const size_t old_committed_size = limit_ * kEntrySize; + const size_t new_committed_size = new_limit * kEntrySize; + CHECK(table_); + CHECK_EQ(0u, new_committed_size % base::kPageAllocationGranularity); + CHECK_GE(MaxTableSize(), limit_ * kEntrySize); + // Recommitting and zapping assumes byte-addressable storage. + uint8_t* const current_table_end = + reinterpret_cast(table_) + old_committed_size; + const size_t table_size_delta = new_committed_size - old_committed_size; + // Commit the new size and allow read/write. + // TODO(ajwong): SetSystemPagesAccess should be part of RecommitSystemPages to + // avoid having two calls here. + bool ok = base::SetSystemPagesAccess(current_table_end, table_size_delta, + base::PageReadWrite); + CHECK(ok); + ok = base::RecommitSystemPages(current_table_end, table_size_delta, + base::PageReadWrite); + CHECK(ok); + +#if DCHECK_IS_ON() + // Check that newly-committed memory is zero-initialized. + for (size_t i = 0; i < (table_size_delta / sizeof(uintptr_t)); ++i) { + DCHECK(!reinterpret_cast(current_table_end)[i]); + } +#endif // DCHECK_IS_ON() + limit_ = new_limit; } -void GCInfoTable::Init() { - CHECK(!g_gc_info_table); +GCInfoTable::GCInfoTable() { + CHECK(!table_); + table_ = reinterpret_cast(base::AllocPages( + nullptr, MaxTableSize(), base::kPageAllocationGranularity, + base::PageInaccessible)); + CHECK(table_); Resize(); } diff --git a/chromium/third_party/WebKit/Source/platform/heap/GCInfo.h b/chromium/third_party/WebKit/Source/platform/heap/GCInfo.h index a07634d454d..c6f56591c3c 100644 --- a/chromium/third_party/WebKit/Source/platform/heap/GCInfo.h +++ b/chromium/third_party/WebKit/Source/platform/heap/GCInfo.h @@ -151,38 +151,62 @@ struct GCInfo { bool has_v_table_; }; -// s_gcInfoTable holds the per-class GCInfo descriptors; each HeapObjectHeader -// keeps an index into this table. -extern PLATFORM_EXPORT GCInfo const** g_gc_info_table; - #if DCHECK_IS_ON() PLATFORM_EXPORT void AssertObjectHasGCInfo(const void*, size_t gc_info_index); #endif -class GCInfoTable { - STATIC_ONLY(GCInfoTable); - +class PLATFORM_EXPORT GCInfoTable { public: - PLATFORM_EXPORT static void EnsureGCInfoIndex(const GCInfo*, size_t*); - - static void Init(); - - static size_t GcInfoIndex() { return gc_info_index_; } - - // The (max + 1) GCInfo index supported. + // At maximum |kMaxIndex - 1| indices are supported. // // We assume that 14 bits is enough to represent all possible types: during // telemetry runs, we see about 1,000 different types; looking at the output // of the Oilpan GC Clang plugin, there appear to be at most about 6,000 // types. Thus 14 bits should be more than twice as many bits as we will ever // need. - static const size_t kMaxIndex = 1 << 14; + static constexpr size_t kMaxIndex = 1 << 14; + + // Sets up a singleton table that can be acquired using Get(). + static void CreateGlobalTable(); + + static GCInfoTable& Get() { return *global_table_; } + + inline const GCInfo* GCInfoFromIndex(size_t index) { + DCHECK_GE(index, 1u); + DCHECK(index < kMaxIndex); + DCHECK(table_); + const GCInfo* info = table_[index]; + DCHECK(info); + return info; + } + + void EnsureGCInfoIndex(const GCInfo*, size_t*); + + size_t GcInfoIndex() { return current_index_; } private: - static void Resize(); + // Use GCInfoTable::Get() for retrieving the global table outside of testing + + // code. + GCInfoTable(); + + void Resize(); + + // Singleton for each process. Retrieved through Get(). + static GCInfoTable* global_table_; - static size_t gc_info_index_; - static size_t gc_info_table_size_; + // Holds the per-class GCInfo descriptors; each HeapObjectHeader keeps an + // index into this table. + const GCInfo** table_ = nullptr; + + // GCInfo indices start from 1 for heap objects, with 0 being treated + // specially as the index for freelist entries and large heap objects. + size_t current_index_ = 0; + + // The limit (exclusive) of the currently allocated table. + size_t limit_ = 0; + + Mutex table_mutex_; }; // GCInfoAtBaseType should be used when returning a unique 14 bit integer @@ -196,11 +220,9 @@ struct GCInfoAtBaseType { TraceTrait::Trace, FinalizerTrait::Finalize, FinalizerTrait::kNonTrivialFinalizer, std::is_polymorphic::value, }; - static size_t gc_info_index = 0; - DCHECK(g_gc_info_table); if (!AcquireLoad(&gc_info_index)) - GCInfoTable::EnsureGCInfoIndex(&kGcInfo, &gc_info_index); + GCInfoTable::Get().EnsureGCInfoIndex(&kGcInfo, &gc_info_index); DCHECK_GE(gc_info_index, 1u); DCHECK(gc_info_index < GCInfoTable::kMaxIndex); return gc_info_index; diff --git a/chromium/third_party/WebKit/Source/platform/heap/Heap.cpp b/chromium/third_party/WebKit/Source/platform/heap/Heap.cpp index b35635e42d4..c08f6f27e73 100644 --- a/chromium/third_party/WebKit/Source/platform/heap/Heap.cpp +++ b/chromium/third_party/WebKit/Source/platform/heap/Heap.cpp @@ -66,7 +66,7 @@ void ProcessHeap::Init() { total_allocated_object_size_ = 0; total_marked_object_size_ = 0; - GCInfoTable::Init(); + GCInfoTable::CreateGlobalTable(); CallbackStackMemoryPool::Instance().Initialize(); } @@ -711,7 +711,7 @@ void ThreadHeap::TakeSnapshot(SnapshotType type) { // 0 is used as index for freelist entries. Objects are indexed 1 to // gcInfoIndex. - ThreadState::GCSnapshotInfo info(GCInfoTable::GcInfoIndex() + 1); + ThreadState::GCSnapshotInfo info(GCInfoTable::Get().GcInfoIndex() + 1); String thread_dump_name = String::Format("blink_gc/thread_%lu", static_cast(thread_state_->ThreadId())); @@ -761,8 +761,8 @@ void ThreadHeap::TakeSnapshot(SnapshotType type) { size_t total_dead_count = 0; size_t total_live_size = 0; size_t total_dead_size = 0; - for (size_t gc_info_index = 1; gc_info_index <= GCInfoTable::GcInfoIndex(); - ++gc_info_index) { + for (size_t gc_info_index = 1; + gc_info_index <= GCInfoTable::Get().GcInfoIndex(); ++gc_info_index) { total_live_count += info.live_count[gc_info_index]; total_dead_count += info.dead_count[gc_info_index]; total_live_size += info.live_size[gc_info_index]; @@ -837,7 +837,7 @@ void ThreadHeap::WriteBarrierInternal(BasePage* page, const void* value) { // Mark and push trace callback. header->Mark(); PushTraceCallback(header->Payload(), - ThreadHeap::GcInfo(header->GcInfoIndex())->trace_); + GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex())->trace_); } ThreadHeap* ThreadHeap::main_thread_heap_ = nullptr; diff --git a/chromium/third_party/WebKit/Source/platform/heap/Heap.h b/chromium/third_party/WebKit/Source/platform/heap/Heap.h index 33753e94a52..6c1ba3a0ae7 100644 --- a/chromium/third_party/WebKit/Source/platform/heap/Heap.h +++ b/chromium/third_party/WebKit/Source/platform/heap/Heap.h @@ -413,15 +413,6 @@ class PLATFORM_EXPORT ThreadHeap { // heap-page if one exists. BasePage* LookupPageForAddress(Address); - static const GCInfo* GcInfo(size_t gc_info_index) { - DCHECK_GE(gc_info_index, 1u); - DCHECK(gc_info_index < GCInfoTable::kMaxIndex); - DCHECK(g_gc_info_table); - const GCInfo* info = g_gc_info_table[gc_info_index]; - DCHECK(info); - return info; - } - static void ReportMemoryUsageHistogram(); static void ReportMemoryUsageForTracing(); @@ -761,7 +752,9 @@ Address ThreadHeap::Reallocate(void* previous, size_t size) { size_t gc_info_index = GCInfoTrait::Index(); // TODO(haraken): We don't support reallocate() for finalizable objects. - DCHECK(!ThreadHeap::GcInfo(previous_header->GcInfoIndex())->HasFinalizer()); + DCHECK(!GCInfoTable::Get() + .GCInfoFromIndex(previous_header->GcInfoIndex()) + ->HasFinalizer()); DCHECK_EQ(previous_header->GcInfoIndex(), gc_info_index); HeapAllocHooks::FreeHookIfEnabled(static_cast
(previous)); Address address; diff --git a/chromium/third_party/WebKit/Source/platform/heap/HeapPage.cpp b/chromium/third_party/WebKit/Source/platform/heap/HeapPage.cpp index 63e72a88a04..d8f76cdcf92 100644 --- a/chromium/third_party/WebKit/Source/platform/heap/HeapPage.cpp +++ b/chromium/third_party/WebKit/Source/platform/heap/HeapPage.cpp @@ -96,7 +96,7 @@ void HeapObjectHeader::ZapMagic() { void HeapObjectHeader::Finalize(Address object, size_t object_size) { HeapAllocHooks::FreeHookIfEnabled(object); - const GCInfo* gc_info = ThreadHeap::GcInfo(GcInfoIndex()); + const GCInfo* gc_info = GCInfoTable::Get().GCInfoFromIndex(GcInfoIndex()); if (gc_info->HasFinalizer()) gc_info->finalize_(object); @@ -1698,7 +1698,8 @@ static bool IsUninitializedMemory(void* object_pointer, size_t object_size) { #endif static void MarkPointer(Visitor* visitor, HeapObjectHeader* header) { - const GCInfo* gc_info = ThreadHeap::GcInfo(header->GcInfoIndex()); + const GCInfo* gc_info = + GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex()); if (gc_info->HasVTable() && !VTableInitialized(header->Payload())) { // We hit this branch when a GC strikes before GarbageCollected<>'s // constructor runs. -- cgit v1.2.3