Replace some CPDF_Array::Add() calls with AddNew<>().
Ditto for InsertAt() and SetAt().
AddNew<>() is safer because it can not introduce a direct
cycle of objects into the array since it creates a new object
that can not have references to any old objects [cycles through
indirect objects are always possible, but do not affect memory
management when using RetainPtr<>].
This reduces the number of Add() calls that might need auditing
should a cycle be discovered in arrays again someday.
Change-Id: Ic423fcd4baefed0c692bd60961b360f6bb506e43
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/53870
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/edit/cpdf_pagecontentmanager.cpp b/core/fpdfapi/edit/cpdf_pagecontentmanager.cpp
index 0191f47..f4d51c6 100644
--- a/core/fpdfapi/edit/cpdf_pagecontentmanager.cpp
+++ b/core/fpdfapi/edit/cpdf_pagecontentmanager.cpp
@@ -66,8 +66,10 @@
// create an array with the old and the new one. The new one's index is 1.
if (contents_stream_) {
CPDF_Array* new_contents_array = doc_->NewIndirect<CPDF_Array>();
- new_contents_array->Add(contents_stream_->MakeReference(doc_.Get()));
- new_contents_array->Add(new_stream->MakeReference(doc_.Get()));
+ new_contents_array->AddNew<CPDF_Reference>(doc_.Get(),
+ contents_stream_->GetObjNum());
+ new_contents_array->AddNew<CPDF_Reference>(doc_.Get(),
+ new_stream->GetObjNum());
CPDF_Dictionary* page_dict = obj_holder_->GetDict();
page_dict->SetNewFor<CPDF_Reference>("Contents", doc_.Get(),
@@ -79,7 +81,8 @@
// If there is an array, just add the new stream to it, at the last position.
if (contents_array_) {
- contents_array_->Add(new_stream->MakeReference(doc_.Get()));
+ contents_array_->AddNew<CPDF_Reference>(doc_.Get(),
+ new_stream->GetObjNum());
return contents_array_->size() - 1;
}
diff --git a/core/fpdfapi/page/cpdf_image.cpp b/core/fpdfapi/page/cpdf_image.cpp
index 2a44215..1d01edb 100644
--- a/core/fpdfapi/page/cpdf_image.cpp
+++ b/core/fpdfapi/page/cpdf_image.cpp
@@ -246,7 +246,7 @@
auto pNewDict = m_pDocument->New<CPDF_Dictionary>();
CPDF_Stream* pCTS = m_pDocument->NewIndirect<CPDF_Stream>(
std::move(pColorTable), iPalette * 3, std::move(pNewDict));
- pCS->Add(pCTS->MakeReference(m_pDocument.Get()));
+ pCS->AddNew<CPDF_Reference>(m_pDocument.Get(), pCTS->GetObjNum());
pDict->SetNewFor<CPDF_Reference>("ColorSpace", m_pDocument.Get(),
pCS->GetObjNum());
} else {
diff --git a/core/fpdfapi/parser/cpdf_array.h b/core/fpdfapi/parser/cpdf_array.h
index 55d4cd6..7922e83 100644
--- a/core/fpdfapi/parser/cpdf_array.h
+++ b/core/fpdfapi/parser/cpdf_array.h
@@ -54,14 +54,11 @@
CFX_Matrix GetMatrix() const;
CFX_FloatRect GetRect() const;
- // Takes ownership of |pObj|, returns unowned pointer to it.
- CPDF_Object* Add(RetainPtr<CPDF_Object> pObj);
- CPDF_Object* SetAt(size_t index, RetainPtr<CPDF_Object> pObj);
- CPDF_Object* InsertAt(size_t index, RetainPtr<CPDF_Object> pObj);
-
// Creates object owned by the array, returns unowned pointer to it.
// We have special cases for objects that can intern strings from
- // a ByteStringPool.
+ // a ByteStringPool. Prefer using these templates over direct calls
+ // to Add()/SetAt()/InsertAt() since by creating a new object with no
+ // previous references, they ensure cycles can not be introduced.
template <typename T, typename... Args>
typename std::enable_if<!CanInternStrings<T>::value, T*>::type AddNew(
Args&&... args) {
@@ -103,6 +100,11 @@
index, pdfium::MakeRetain<T>(m_pPool, std::forward<Args>(args)...)));
}
+ // Takes ownership of |pObj|, returns unowned pointer to it.
+ CPDF_Object* Add(RetainPtr<CPDF_Object> pObj);
+ CPDF_Object* SetAt(size_t index, RetainPtr<CPDF_Object> pObj);
+ CPDF_Object* InsertAt(size_t index, RetainPtr<CPDF_Object> pObj);
+
void Clear();
void RemoveAt(size_t index);
void ConvertToIndirectObjectAt(size_t index, CPDF_IndirectObjectHolder* pDoc);
diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp
index 835b086..be04432 100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -540,7 +540,7 @@
continue;
}
if (bInsert) {
- pKidList->InsertAt(i, pPageDict->MakeReference(this));
+ pKidList->InsertNewAt<CPDF_Reference>(i, this, pPageDict->GetObjNum());
pPageDict->SetNewFor<CPDF_Reference>("Parent", this,
pPages->GetObjNum());
} else {
@@ -584,7 +584,7 @@
CPDF_Array* pPagesList = pPages->GetArrayFor("Kids");
if (!pPagesList)
pPagesList = pPages->SetNewFor<CPDF_Array>("Kids");
- pPagesList->Add(pPageDict->MakeReference(this));
+ pPagesList->AddNew<CPDF_Reference>(this, pPageDict->GetObjNum());
pPages->SetNewFor<CPDF_Number>("Count", nPages + 1);
pPageDict->SetNewFor<CPDF_Reference>("Parent", this, pPages->GetObjNum());
ResetTraversal();
@@ -722,7 +722,7 @@
pCIDSysInfo->SetNewFor<CPDF_Number>("Supplement", supplement);
CPDF_Array* pArray = pBaseDict->SetNewFor<CPDF_Array>("DescendantFonts");
- pArray->Add(pFontDict->MakeReference(this));
+ pArray->AddNew<CPDF_Reference>(this, pFontDict->GetObjNum());
return pFontDict;
}
diff --git a/fpdfsdk/fpdf_edittext.cpp b/fpdfsdk/fpdf_edittext.cpp
index e39da48..ddb0ee7 100644
--- a/fpdfsdk/fpdf_edittext.cpp
+++ b/fpdfsdk/fpdf_edittext.cpp
@@ -419,7 +419,7 @@
// TODO(npm): Support vertical writing
auto* pDescendant = pFontDict->SetNewFor<CPDF_Array>("DescendantFonts");
- pDescendant->Add(pCIDFont->MakeReference(pDoc));
+ pDescendant->AddNew<CPDF_Reference>(pDoc, pCIDFont->GetObjNum());
CPDF_Stream* toUnicodeStream = LoadUnicode(pDoc, to_unicode);
pFontDict->SetNewFor<CPDF_Reference>("ToUnicode", pDoc,
diff --git a/fpdfsdk/fpdf_flatten.cpp b/fpdfsdk/fpdf_flatten.cpp
index 94cc288..d6dbe9a 100644
--- a/fpdfsdk/fpdf_flatten.cpp
+++ b/fpdfsdk/fpdf_flatten.cpp
@@ -214,7 +214,8 @@
}
pContentsStream->SetDataAndRemoveFilter(sStream.AsRawSpan());
pContentsArray = pDocument->NewIndirect<CPDF_Array>();
- pContentsArray->Add(pContentsStream->MakeReference(pDocument));
+ pContentsArray->AddNew<CPDF_Reference>(pDocument,
+ pContentsStream->GetObjNum());
pPage->SetNewFor<CPDF_Reference>(pdfium::page_object::kContents, pDocument,
pContentsArray->GetObjNum());
}
diff --git a/fpdfsdk/fpdf_save.cpp b/fpdfsdk/fpdf_save.cpp
index 76a434d..22af561 100644
--- a/fpdfsdk/fpdf_save.cpp
+++ b/fpdfsdk/fpdf_save.cpp
@@ -140,7 +140,8 @@
pData->InitStreamFromFile(pDsfileWrite, std::move(pDataDict));
int iLast = pArray->size() - 2;
pArray->InsertNewAt<CPDF_String>(iLast, "datasets", false);
- pArray->InsertAt(iLast + 1, pData->MakeReference(pPDFDocument));
+ pArray->InsertNewAt<CPDF_Reference>(iLast + 1, pPDFDocument,
+ pData->GetObjNum());
}
fileList->push_back(std::move(pDsfileWrite));
}
@@ -164,7 +165,8 @@
pData->InitStreamFromFile(pfileWrite, std::move(pDataDict));
int iLast = pArray->size() - 2;
pArray->InsertNewAt<CPDF_String>(iLast, "form", false);
- pArray->InsertAt(iLast + 1, pData->MakeReference(pPDFDocument));
+ pArray->InsertNewAt<CPDF_Reference>(iLast + 1, pPDFDocument,
+ pData->GetObjNum());
}
fileList->push_back(std::move(pfileWrite));
}
diff --git a/fpdfsdk/fpdf_transformpage.cpp b/fpdfsdk/fpdf_transformpage.cpp
index 3ffbb3b..80101ae 100644
--- a/fpdfsdk/fpdf_transformpage.cpp
+++ b/fpdfsdk/fpdf_transformpage.cpp
@@ -203,13 +203,13 @@
pEndStream->SetData(ByteStringView(" Q").raw_span());
if (CPDF_Array* pContentArray = ToArray(pContentObj)) {
- pContentArray->InsertAt(0, pStream->MakeReference(pDoc));
- pContentArray->Add(pEndStream->MakeReference(pDoc));
+ pContentArray->InsertNewAt<CPDF_Reference>(0, pDoc, pStream->GetObjNum());
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pEndStream->GetObjNum());
} else if (pContentObj->IsStream() && !pContentObj->IsInline()) {
pContentArray = pDoc->NewIndirect<CPDF_Array>();
- pContentArray->Add(pStream->MakeReference(pDoc));
- pContentArray->Add(pContentObj->MakeReference(pDoc));
- pContentArray->Add(pEndStream->MakeReference(pDoc));
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pStream->GetObjNum());
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pContentObj->GetObjNum());
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pEndStream->GetObjNum());
pPageDict->SetNewFor<CPDF_Reference>(pdfium::page_object::kContents, pDoc,
pContentArray->GetObjNum());
}
@@ -359,11 +359,11 @@
pStream->SetDataFromStringstream(&strClip);
if (CPDF_Array* pArray = ToArray(pContentObj)) {
- pArray->InsertAt(0, pStream->MakeReference(pDoc));
+ pArray->InsertNewAt<CPDF_Reference>(0, pDoc, pStream->GetObjNum());
} else if (pContentObj->IsStream() && !pContentObj->IsInline()) {
CPDF_Array* pContentArray = pDoc->NewIndirect<CPDF_Array>();
- pContentArray->Add(pStream->MakeReference(pDoc));
- pContentArray->Add(pContentObj->MakeReference(pDoc));
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pStream->GetObjNum());
+ pContentArray->AddNew<CPDF_Reference>(pDoc, pContentObj->GetObjNum());
pPageDict->SetNewFor<CPDF_Reference>(pdfium::page_object::kContents, pDoc,
pContentArray->GetObjNum());
}
