Prevent cxfa_contentlayoutitem linked lists from getting entangled.
Implement the basic linked-list primitives, and use them
consistently. Currently the code is doing ad-hoc manipulations
of these pointers, and creating circular lists somewhere. This
causes re-frees as we walk down the list freeing every item.
Use UnownedPtr<> to try to catch any future botches.
Tested against the test case in 925790 (CF fuzzer will subsequently
verify 913564).
Bug: chromium:913564, chromium:925790
Change-Id: I2b735b3137aa715e5bb6b1c4472a1d2fd68ae286
Reviewed-on: https://pdfium-review.googlesource.com/c/49350
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/xfa/fxfa/parser/cxfa_contentlayoutitem.cpp b/xfa/fxfa/parser/cxfa_contentlayoutitem.cpp
index 6878c8e..79b3cb7 100644
--- a/xfa/fxfa/parser/cxfa_contentlayoutitem.cpp
+++ b/xfa/fxfa/parser/cxfa_contentlayoutitem.cpp
@@ -14,6 +14,7 @@
: CXFA_LayoutItem(pNode, kContentItem) {}
CXFA_ContentLayoutItem::~CXFA_ContentLayoutItem() {
+ RemoveSelf();
if (m_pFormNode->JSObject()->GetLayoutItem() == this)
m_pFormNode->JSObject()->SetLayoutItem(nullptr);
}
@@ -24,20 +25,36 @@
CXFA_ContentLayoutItem* CXFA_ContentLayoutItem::GetFirst() {
CXFA_ContentLayoutItem* pCurNode = this;
- while (pCurNode->m_pPrev)
- pCurNode = pCurNode->m_pPrev;
+ while (auto* pPrev = pCurNode->GetPrev())
+ pCurNode = pPrev;
return pCurNode;
}
CXFA_ContentLayoutItem* CXFA_ContentLayoutItem::GetLast() {
CXFA_ContentLayoutItem* pCurNode = this;
- while (pCurNode->m_pNext)
- pCurNode = pCurNode->m_pNext;
+ while (auto* pNext = pCurNode->GetNext())
+ pCurNode = pNext;
return pCurNode;
}
+void CXFA_ContentLayoutItem::InsertAfter(CXFA_ContentLayoutItem* pItem) {
+ pItem->RemoveSelf();
+ pItem->m_pNext = m_pNext;
+ pItem->m_pPrev = this;
+ m_pNext = pItem;
+ if (pItem->m_pNext)
+ pItem->m_pNext->m_pPrev = pItem;
+}
+
+void CXFA_ContentLayoutItem::RemoveSelf() {
+ if (m_pNext)
+ m_pNext->m_pPrev = m_pPrev;
+ if (m_pPrev)
+ m_pPrev->m_pNext = m_pNext;
+}
+
CFX_RectF CXFA_ContentLayoutItem::GetRect(bool bRelative) const {
CFX_PointF sPos = m_sPos;
CFX_SizeF sSize = m_sSize;
@@ -83,8 +100,8 @@
int32_t CXFA_ContentLayoutItem::GetIndex() const {
int32_t iIndex = 0;
const CXFA_ContentLayoutItem* pCurNode = this;
- while (pCurNode->m_pPrev) {
- pCurNode = pCurNode->m_pPrev;
+ while (auto* pPrev = pCurNode->GetPrev()) {
+ pCurNode = pPrev;
++iIndex;
}
return iIndex;
@@ -93,8 +110,8 @@
int32_t CXFA_ContentLayoutItem::GetCount() const {
int32_t iCount = GetIndex() + 1;
const CXFA_ContentLayoutItem* pCurNode = this;
- while (pCurNode->m_pNext) {
- pCurNode = pCurNode->m_pNext;
+ while (auto* pNext = pCurNode->GetNext()) {
+ pCurNode = pNext;
iCount++;
}
return iCount;
diff --git a/xfa/fxfa/parser/cxfa_contentlayoutitem.h b/xfa/fxfa/parser/cxfa_contentlayoutitem.h
index ac2d8f2..7313a1e 100644
--- a/xfa/fxfa/parser/cxfa_contentlayoutitem.h
+++ b/xfa/fxfa/parser/cxfa_contentlayoutitem.h
@@ -7,6 +7,7 @@
#ifndef XFA_FXFA_PARSER_CXFA_CONTENTLAYOUTITEM_H_
#define XFA_FXFA_PARSER_CXFA_CONTENTLAYOUTITEM_H_
+#include "core/fxcrt/unowned_ptr.h"
#include "xfa/fxfa/parser/cxfa_layoutitem.h"
class CXFA_FFWidget;
@@ -20,18 +21,23 @@
CXFA_ContentLayoutItem* GetFirst();
CXFA_ContentLayoutItem* GetLast();
- CXFA_ContentLayoutItem* GetPrev() const { return m_pPrev; }
- CXFA_ContentLayoutItem* GetNext() const { return m_pNext; }
+ CXFA_ContentLayoutItem* GetPrev() const { return m_pPrev.Get(); }
+ CXFA_ContentLayoutItem* GetNext() const { return m_pNext.Get(); }
+ void InsertAfter(CXFA_ContentLayoutItem* pNext);
CFX_RectF GetRect(bool bRelative) const;
int32_t GetIndex() const;
int32_t GetCount() const;
- CXFA_ContentLayoutItem* m_pPrev = nullptr;
- CXFA_ContentLayoutItem* m_pNext = nullptr;
CFX_PointF m_sPos;
CFX_SizeF m_sSize;
mutable uint32_t m_dwStatus = 0;
+
+ private:
+ void RemoveSelf();
+
+ UnownedPtr<CXFA_ContentLayoutItem> m_pPrev;
+ UnownedPtr<CXFA_ContentLayoutItem> m_pNext;
};
inline CXFA_FFWidget* ToFFWidget(CXFA_ContentLayoutItem* item) {
diff --git a/xfa/fxfa/parser/cxfa_itemlayoutprocessor.cpp b/xfa/fxfa/parser/cxfa_itemlayoutprocessor.cpp
index db24066..f91f3f0 100644
--- a/xfa/fxfa/parser/cxfa_itemlayoutprocessor.cpp
+++ b/xfa/fxfa/parser/cxfa_itemlayoutprocessor.cpp
@@ -444,7 +444,7 @@
CXFA_ContentLayoutItem* pCurLayoutItem =
ToContentLayoutItem(pNode->JSObject()->GetLayoutItem());
while (pCurLayoutItem) {
- CXFA_ContentLayoutItem* pNextLayoutItem = pCurLayoutItem->m_pNext;
+ CXFA_ContentLayoutItem* pNextLayoutItem = pCurLayoutItem->GetNext();
pNotify->OnLayoutItemRemoving(pDocLayout, pCurLayoutItem);
delete pCurLayoutItem;
pCurLayoutItem = pNextLayoutItem;
@@ -643,7 +643,7 @@
CXFA_ContentLayoutItem* pLayoutItem = nullptr;
if (m_pOldLayoutItem) {
pLayoutItem = m_pOldLayoutItem;
- m_pOldLayoutItem = m_pOldLayoutItem->m_pNext;
+ m_pOldLayoutItem = m_pOldLayoutItem->GetNext();
return pLayoutItem;
}
pLayoutItem = pFormNode->GetDocument()
@@ -653,11 +653,7 @@
CXFA_ContentLayoutItem* pPrevLayoutItem =
ToContentLayoutItem(pFormNode->JSObject()->GetLayoutItem());
if (pPrevLayoutItem) {
- while (pPrevLayoutItem->m_pNext)
- pPrevLayoutItem = pPrevLayoutItem->m_pNext;
-
- pPrevLayoutItem->m_pNext = pLayoutItem;
- pLayoutItem->m_pPrev = pPrevLayoutItem;
+ pPrevLayoutItem->GetLast()->InsertAfter(pLayoutItem);
} else {
pFormNode->JSObject()->SetLayoutItem(pLayoutItem);
}
@@ -818,16 +814,13 @@
return pLayoutItem;
}
- if (m_pOldLayoutItem->m_pPrev)
- m_pOldLayoutItem->m_pPrev->m_pNext = nullptr;
-
CXFA_FFNotify* pNotify =
m_pOldLayoutItem->GetFormNode()->GetDocument()->GetNotify();
CXFA_LayoutProcessor* pDocLayout =
m_pOldLayoutItem->GetFormNode()->GetDocument()->GetLayoutProcessor();
CXFA_ContentLayoutItem* pOldLayoutItem = m_pOldLayoutItem;
while (pOldLayoutItem) {
- CXFA_ContentLayoutItem* pNextOldLayoutItem = pOldLayoutItem->m_pNext;
+ CXFA_ContentLayoutItem* pNextOldLayoutItem = pOldLayoutItem->GetNext();
pNotify->OnLayoutItemRemoving(pDocLayout, pOldLayoutItem);
if (pOldLayoutItem->m_pParent)
pOldLayoutItem->m_pParent->RemoveChild(pOldLayoutItem);
@@ -1725,7 +1718,7 @@
if (!pLayoutNext->m_pNextSibling && m_pCurChildPreprocessor &&
m_pCurChildPreprocessor->GetFormNode() ==
pLayoutNext->GetFormNode()) {
- pLayoutNext->m_pNext = m_pCurChildPreprocessor->m_pLayoutItem;
+ pLayoutNext->InsertAfter(m_pCurChildPreprocessor->m_pLayoutItem);
m_pCurChildPreprocessor->m_pLayoutItem = pLayoutNext;
break;
}
