From 578321a8019eed33fdc42bdf884853f73286f4dc Mon Sep 17 00:00:00 2001 From: Dan Hecht Date: Tue, 2 Sep 2025 14:05:54 -0700 Subject: [PATCH] Avoid dangling pointers in WeakBlock list https://bugs.webkit.org/show_bug.cgi?id=298236 rdar://157587352 Reviewed by Keith Miller. Before this change, DoublyLinkedList leaves the next/prev pointers in a dangling state when removing a node from the list. Then, if the node is re-added, the next/prev pointers are reset when necessary. Let's make a stronger invariant: if the node is not in the list, then the prev/next pointers are nullptr. (Note that the converse is not true for single element lists.) Then, add some asserts to verify the WeakSet's WeakBlock list lifecycle to try to help track down a mysterious crash. The WeakBlock ownership can be transferred from the WeakSet to the Heap, at which point the prev/next should be nulled out and, after this change, no longer dangling. Bonus: remove MarkedSpace::freeOrShrinkBlock() since it's never called. Canonical link: https://commits.webkit.org/299454@main --- Source/JavaScriptCore/heap/Heap.cpp | 2 ++ Source/JavaScriptCore/heap/WeakBlock.cpp | 1 + Source/JavaScriptCore/heap/WeakSet.cpp | 7 ++-- Source/WTF/wtf/DoublyLinkedList.h | 43 ++++++++++++++++++------ 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/Source/JavaScriptCore/heap/Heap.cpp b/Source/JavaScriptCore/heap/Heap.cpp index 632b01f14546c..847d05898f0f1 100644 --- a/Source/JavaScriptCore/heap/Heap.cpp +++ b/Source/JavaScriptCore/heap/Heap.cpp @@ -2534,6 +2534,7 @@ bool Heap::shouldDoFullCollection() void Heap::addLogicallyEmptyWeakBlock(WeakBlock* block) { + RELEASE_ASSERT(!block->next() && !block->prev()); m_logicallyEmptyWeakBlocks.append(block); } @@ -2552,6 +2553,7 @@ bool Heap::sweepNextLogicallyEmptyWeakBlock() return false; WeakBlock* block = m_logicallyEmptyWeakBlocks[m_indexOfNextLogicallyEmptyWeakBlockToSweep]; + RELEASE_ASSERT(!block->next() && !block->prev()); block->sweep(); if (block->isEmpty()) { diff --git a/Source/JavaScriptCore/heap/WeakBlock.cpp b/Source/JavaScriptCore/heap/WeakBlock.cpp index 45e3411635359..d4915f2638b23 100644 --- a/Source/JavaScriptCore/heap/WeakBlock.cpp +++ b/Source/JavaScriptCore/heap/WeakBlock.cpp @@ -45,6 +45,7 @@ WeakBlock* WeakBlock::create(Heap& heap, CellContainer container) void WeakBlock::destroy(Heap& heap, WeakBlock* block) { + RELEASE_ASSERT(!block->next() && !block->prev()); block->~WeakBlock(); WeakBlockMalloc::free(block); heap.didFreeBlock(WeakBlock::blockSize); diff --git a/Source/JavaScriptCore/heap/WeakSet.cpp b/Source/JavaScriptCore/heap/WeakSet.cpp index 47b270ac0671b..b5324e2656abe 100644 --- a/Source/JavaScriptCore/heap/WeakSet.cpp +++ b/Source/JavaScriptCore/heap/WeakSet.cpp @@ -37,12 +37,9 @@ WeakSet::~WeakSet() remove(); Heap& heap = *this->heap(); - WeakBlock* next = nullptr; - for (WeakBlock* block = m_blocks.head(); block; block = next) { - next = block->next(); + while (WeakBlock* block = m_blocks.removeHead()) WeakBlock::destroy(heap, block); - } - m_blocks.clear(); + ASSERT(m_blocks.isEmpty()); } void WeakSet::sweep() diff --git a/Source/WTF/wtf/DoublyLinkedList.h b/Source/WTF/wtf/DoublyLinkedList.h index ecef018c5f326..efcef7a9b6372 100644 --- a/Source/WTF/wtf/DoublyLinkedList.h +++ b/Source/WTF/wtf/DoublyLinkedList.h @@ -41,8 +41,8 @@ template class DoublyLinkedListNode { template inline DoublyLinkedListNode::DoublyLinkedListNode() { - setPrev(0); - setNext(0); + setPrev(nullptr); + setNext(nullptr); } template inline void DoublyLinkedListNode::setPrev(T* prev) @@ -84,14 +84,16 @@ class DoublyLinkedList { void remove(T*); void append(DoublyLinkedList&); + DoublyLinkedList splitAt(size_t); + private: T* m_head; T* m_tail; }; template inline DoublyLinkedList::DoublyLinkedList() - : m_head(0) - , m_tail(0) + : m_head(nullptr) + , m_tail(nullptr) { } @@ -126,40 +128,59 @@ template inline T* DoublyLinkedList::tail() const template inline void DoublyLinkedList::push(T* node) { + ASSERT(!node->prev() && !node->next()); if (!m_head) { ASSERT(!m_tail); m_head = node; m_tail = node; - node->setPrev(0); - node->setNext(0); return; } ASSERT(m_tail); m_head->setPrev(node); node->setNext(m_head); - node->setPrev(0); m_head = node; } template inline void DoublyLinkedList::append(T* node) { + ASSERT(!node->prev() && !node->next()); if (!m_tail) { ASSERT(!m_head); m_head = node; m_tail = node; - node->setPrev(0); - node->setNext(0); return; } ASSERT(m_head); m_tail->setNext(node); node->setPrev(m_tail); - node->setNext(0); m_tail = node; } +template inline DoublyLinkedList DoublyLinkedList::splitAt(size_t toKeep) +{ + auto* p = head(); + + if (!p || !p->next()) + return { }; + for (size_t i = 1; i < toKeep; i++) { + p = p->next(); + if (!p->next()) + return { }; + } + + DoublyLinkedList newList { }; + newList.m_head = p->next(); + newList.m_tail = m_tail; + newList.m_head->setPrev(nullptr); + + m_tail = p; + m_tail->setNext(nullptr); + + return newList; +} + template inline void DoublyLinkedList::remove(T* node) { if (node->prev()) { @@ -177,6 +198,8 @@ template inline void DoublyLinkedList::remove(T* node) ASSERT(node == m_tail); m_tail = node->prev(); } + node->setNext(nullptr); + node->setPrev(nullptr); } template inline T* DoublyLinkedList::removeHead()