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()); }