Return object number from CPDF_IndirectObjectHolder::AddIndirectObject()
Avoid handing the caller an unretained reference to an object that may
get replaced during execution. Most callers are simplified since they
wanted just the object number anyways.
Change-Id: Ia17f0a7911b24f442ba046b1b07c2891a16620e9
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/100130
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
index 382d3ee..6b2b296 100644
--- a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
+++ b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
@@ -498,8 +498,8 @@
gsDict->SetNewFor<CPDF_Name>("BM",
pPageObj->m_GeneralState.GetBlendMode());
}
- CPDF_Object* pDict = m_pDocument->AddIndirectObject(gsDict);
- name = RealizeResource(pDict, "ExtGState");
+ m_pDocument->AddIndirectObject(gsDict);
+ name = RealizeResource(std::move(gsDict), "ExtGState");
m_pObjHolder->GraphicsMapInsert(graphD, name);
}
*buf << "/" << PDF_NameEncode(name) << " gs ";
@@ -529,8 +529,8 @@
gsDict->SetNewFor<CPDF_Number>("ca", defaultGraphics.fillAlpha);
gsDict->SetNewFor<CPDF_Number>("CA", defaultGraphics.strokeAlpha);
gsDict->SetNewFor<CPDF_Name>("BM", "Normal");
- CPDF_Object* pDict = m_pDocument->AddIndirectObject(gsDict);
- ByteString name = RealizeResource(pDict, "ExtGState");
+ m_pDocument->AddIndirectObject(gsDict);
+ ByteString name = RealizeResource(std::move(gsDict), "ExtGState");
m_pObjHolder->GraphicsMapInsert(defaultGraphics, name);
return name;
}
@@ -580,9 +580,10 @@
pFontDict->SetFor("Encoding",
pEncoding->Realize(m_pDocument->GetByteStringPool()));
}
- pIndirectFont = m_pDocument->AddIndirectObject(pFontDict);
+ m_pDocument->AddIndirectObject(pFontDict);
+ pIndirectFont = std::move(pFontDict);
}
- dictName = RealizeResource(pIndirectFont, "Font");
+ dictName = RealizeResource(std::move(pIndirectFont), "Font");
m_pObjHolder->FontsMapInsert(data, dictName);
}
*buf << "/" << PDF_NameEncode(dictName) << " ";
diff --git a/core/fpdfapi/page/cpdf_docpagedata.cpp b/core/fpdfapi/page/cpdf_docpagedata.cpp
index 6334b04..e27019b 100644
--- a/core/fpdfapi/page/cpdf_docpagedata.cpp
+++ b/core/fpdfapi/page/cpdf_docpagedata.cpp
@@ -552,12 +552,12 @@
nStemV = width;
}
}
- CPDF_Dictionary* pFontDesc =
- ToDictionary(GetDocument()->AddIndirectObject(CalculateFontDesc(
- GetDocument(), basefont, flags, italicangle, pFont->GetAscent(),
- pFont->GetDescent(), std::move(pBBox), nStemV)));
+ RetainPtr<CPDF_Dictionary> pFontDesc = CalculateFontDesc(
+ GetDocument(), basefont, flags, italicangle, pFont->GetAscent(),
+ pFont->GetDescent(), std::move(pBBox), nStemV);
+ uint32_t new_objnum = GetDocument()->AddIndirectObject(std::move(pFontDesc));
pFontDict->SetNewFor<CPDF_Reference>("FontDescriptor", GetDocument(),
- pFontDesc->GetObjNum());
+ new_objnum);
return GetFont(pBaseDict);
}
@@ -635,10 +635,8 @@
CalculateFontDesc(GetDocument(), basefont, flags, italicangle, ascend,
descend, std::move(pBBox), pLogFont->lfWeight / 5);
pFontDesc->SetNewFor<CPDF_Number>("CapHeight", capheight);
- pFontDict->SetFor("FontDescriptor",
- GetDocument()
- ->AddIndirectObject(std::move(pFontDesc))
- ->MakeReference(GetDocument()));
+ GetDocument()->AddIndirectObject(pFontDesc);
+ pFontDict->SetFor("FontDescriptor", pFontDesc->MakeReference(GetDocument()));
hFont = SelectObject(hDC, hFont);
DeleteObject(hFont);
DeleteDC(hDC);
diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp
index 850d701..c9dcdb9 100644
--- a/core/fpdfapi/parser/cpdf_array.cpp
+++ b/core/fpdfapi/parser/cpdf_array.cpp
@@ -210,8 +210,8 @@
if (!m_Objects[index] || m_Objects[index]->IsReference())
return;
- CPDF_Object* pNew = pHolder->AddIndirectObject(std::move(m_Objects[index]));
- m_Objects[index] = pNew->MakeReference(pHolder);
+ pHolder->AddIndirectObject(m_Objects[index]);
+ m_Objects[index] = m_Objects[index]->MakeReference(pHolder);
}
void CPDF_Array::SetAt(size_t index, RetainPtr<CPDF_Object> pObj) {
diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp
index ea03f5b..4ccd646 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -296,8 +296,8 @@
if (it == m_Map.end() || it->second->IsReference())
return;
- CPDF_Object* pObj = pHolder->AddIndirectObject(std::move(it->second));
- it->second = pObj->MakeReference(pHolder);
+ pHolder->AddIndirectObject(it->second);
+ it->second = it->second->MakeReference(pHolder);
}
RetainPtr<CPDF_Object> CPDF_Dictionary::RemoveFor(ByteStringView key) {
diff --git a/core/fpdfapi/parser/cpdf_document_unittest.cpp b/core/fpdfapi/parser/cpdf_document_unittest.cpp
index 9c978f9..c6029db 100644
--- a/core/fpdfapi/parser/cpdf_document_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_document_unittest.cpp
@@ -28,15 +28,14 @@
RetainPtr<CPDF_Dictionary> CreatePageTreeNode(RetainPtr<CPDF_Array> kids,
CPDF_Document* pDoc,
int count) {
- CPDF_Array* pUnowned =
- pDoc->AddIndirectObject(std::move(kids))->AsMutableArray();
+ uint32_t new_objnum = pDoc->AddIndirectObject(kids);
auto pageNode = pDoc->NewIndirect<CPDF_Dictionary>();
pageNode->SetNewFor<CPDF_Name>("Type", "Pages");
- pageNode->SetNewFor<CPDF_Reference>("Kids", pDoc, pUnowned->GetObjNum());
+ pageNode->SetNewFor<CPDF_Reference>("Kids", pDoc, new_objnum);
pageNode->SetNewFor<CPDF_Number>("Count", count);
- for (size_t i = 0; i < pUnowned->size(); i++) {
- pUnowned->GetMutableDictAt(i)->SetNewFor<CPDF_Reference>(
- "Parent", pDoc, pageNode->GetObjNum());
+ for (size_t i = 0; i < kids->size(); i++) {
+ kids->GetMutableDictAt(i)->SetNewFor<CPDF_Reference>("Parent", pDoc,
+ pageNode->GetObjNum());
}
return pageNode;
}
@@ -54,32 +53,32 @@
// Set up test
auto zeroToTwo = pdfium::MakeRetain<CPDF_Array>();
zeroToTwo->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(0))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(0)));
zeroToTwo->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(1))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(1)));
zeroToTwo->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(2))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(2)));
RetainPtr<CPDF_Dictionary> branch1 =
CreatePageTreeNode(std::move(zeroToTwo), this, 3);
auto zeroToThree = pdfium::MakeRetain<CPDF_Array>();
zeroToThree->AppendNew<CPDF_Reference>(this, branch1->GetObjNum());
zeroToThree->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(3))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(3)));
RetainPtr<CPDF_Dictionary> branch2 =
CreatePageTreeNode(std::move(zeroToThree), this, 4);
auto fourFive = pdfium::MakeRetain<CPDF_Array>();
fourFive->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(4))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(4)));
fourFive->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(5))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(5)));
RetainPtr<CPDF_Dictionary> branch3 =
CreatePageTreeNode(std::move(fourFive), this, 2);
auto justSix = pdfium::MakeRetain<CPDF_Array>();
justSix->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(6))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(6)));
RetainPtr<CPDF_Dictionary> branch4 =
CreatePageTreeNode(std::move(justSix), this, 1);
@@ -108,9 +107,9 @@
// Set up test
auto allPages = pdfium::MakeRetain<CPDF_Array>();
allPages->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(0))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(0)));
allPages->AppendNew<CPDF_Reference>(
- this, AddIndirectObject(CreateNumberedPage(1))->GetObjNum());
+ this, AddIndirectObject(CreateNumberedPage(1)));
// Page without pageNum.
inlined_page_ = CreateNumberedPage(2);
allPages->Append(inlined_page_);
@@ -226,18 +225,18 @@
auto dict_type_name_page = pdfium::MakeRetain<CPDF_Dictionary>();
dict_type_name_page->SetNewFor<CPDF_Name>("Type", "Page");
- EXPECT_TRUE(CPDF_Document::IsValidPageObject(
- document.AddIndirectObject(dict_type_name_page)));
+ document.AddIndirectObject(dict_type_name_page);
+ EXPECT_TRUE(CPDF_Document::IsValidPageObject(dict_type_name_page.Get()));
auto dict_type_string_page = pdfium::MakeRetain<CPDF_Dictionary>();
dict_type_string_page->SetNewFor<CPDF_String>("Type", "Page", false);
- EXPECT_FALSE(CPDF_Document::IsValidPageObject(
- document.AddIndirectObject(dict_type_string_page)));
+ document.AddIndirectObject(dict_type_string_page);
+ EXPECT_FALSE(CPDF_Document::IsValidPageObject(dict_type_string_page.Get()));
auto dict_type_name_font = pdfium::MakeRetain<CPDF_Dictionary>();
dict_type_name_font->SetNewFor<CPDF_Name>("Type", "Font");
- EXPECT_FALSE(CPDF_Document::IsValidPageObject(
- document.AddIndirectObject(dict_type_name_font)));
+ document.AddIndirectObject(dict_type_name_font);
+ EXPECT_FALSE(CPDF_Document::IsValidPageObject(dict_type_name_font.Get()));
auto obj_no_type = document.NewIndirect<CPDF_Dictionary>();
EXPECT_FALSE(CPDF_Document::IsValidPageObject(obj_no_type.Get()));
@@ -254,9 +253,10 @@
CPDF_TestDocumentAllowSetParser document;
{
- CPDF_Object* first_page = document.AddIndirectObject(CreateNumberedPage(0));
- DCHECK(first_page);
- int first_page_obj_num = first_page->GetObjNum();
+ auto first_page = CreateNumberedPage(0);
+ ASSERT_TRUE(first_page);
+
+ int first_page_obj_num = document.AddIndirectObject(first_page);
ASSERT_NE(kTestPageNum, first_page_obj_num);
linearization_dict->SetNewFor<CPDF_Boolean>("Linearized", true);
diff --git a/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp b/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp
index 59d54b9..1012bcb 100644
--- a/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp
+++ b/core/fpdfapi/parser/cpdf_indirect_object_holder.cpp
@@ -84,14 +84,12 @@
return nullptr;
}
-CPDF_Object* CPDF_IndirectObjectHolder::AddIndirectObject(
+uint32_t CPDF_IndirectObjectHolder::AddIndirectObject(
RetainPtr<CPDF_Object> pObj) {
CHECK(!pObj->GetObjNum());
pObj->SetObjNum(++m_LastObjNum);
-
- auto& obj_holder = m_IndirectObjs[m_LastObjNum];
- obj_holder = std::move(pObj);
- return obj_holder.Get();
+ m_IndirectObjs[m_LastObjNum] = std::move(pObj);
+ return m_LastObjNum;
}
bool CPDF_IndirectObjectHolder::ReplaceIndirectObjectIfHigherGeneration(
diff --git a/core/fpdfapi/parser/cpdf_indirect_object_holder.h b/core/fpdfapi/parser/cpdf_indirect_object_holder.h
index 8cf4c06..3733d48 100644
--- a/core/fpdfapi/parser/cpdf_indirect_object_holder.h
+++ b/core/fpdfapi/parser/cpdf_indirect_object_holder.h
@@ -32,33 +32,31 @@
void DeleteIndirectObject(uint32_t objnum);
// Creates and adds a new object retained by the indirect object holder,
- // and returns a retained pointer to it. We have a special case to
- // handle objects that can intern strings from our ByteStringPool.
+ // and returns a retained pointer to it.
template <typename T, typename... Args>
- typename std::enable_if<!CanInternStrings<T>::value, RetainPtr<T>>::type
- NewIndirect(Args&&... args) {
- return pdfium::WrapRetain(static_cast<T*>(
- AddIndirectObject(pdfium::MakeRetain<T>(std::forward<Args>(args)...))));
- }
- template <typename T, typename... Args>
- typename std::enable_if<CanInternStrings<T>::value, RetainPtr<T>>::type
- NewIndirect(Args&&... args) {
- return pdfium::WrapRetain(
- static_cast<T*>(AddIndirectObject(pdfium::MakeRetain<T>(
- m_pByteStringPool, std::forward<Args>(args)...))));
+ RetainPtr<T> NewIndirect(Args&&... args) {
+ auto obj = New<T>(std::forward<Args>(args)...);
+ AddIndirectObject(obj);
+ return obj;
}
// Creates and adds a new object not retained by the indirect object holder,
- // but which can intern strings from it.
+ // but which can intern strings from it. We have a special cast to handle
+ // objects that can intern strings from our ByteStringPool.
template <typename T, typename... Args>
typename std::enable_if<CanInternStrings<T>::value, RetainPtr<T>>::type New(
Args&&... args) {
return pdfium::MakeRetain<T>(m_pByteStringPool,
std::forward<Args>(args)...);
}
+ template <typename T, typename... Args>
+ typename std::enable_if<!CanInternStrings<T>::value, RetainPtr<T>>::type New(
+ Args&&... args) {
+ return pdfium::MakeRetain<T>(std::forward<Args>(args)...);
+ }
- // Retains |pObj|, returns unowned pointer to it.
- CPDF_Object* AddIndirectObject(RetainPtr<CPDF_Object> pObj);
+ // Always Retains |pObj|, returns its new object number.
+ uint32_t AddIndirectObject(RetainPtr<CPDF_Object> pObj);
// If higher generation number, retains |pObj| and returns true.
bool ReplaceIndirectObjectIfHigherGeneration(uint32_t objnum,
diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp
index 16208c4..76d9c08 100644
--- a/core/fpdfapi/parser/cpdf_object_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp
@@ -99,16 +99,17 @@
// Indirect references to indirect objects.
m_ObjHolder = std::make_unique<CPDF_IndirectObjectHolder>();
- m_IndirectObjs = {m_ObjHolder->AddIndirectObject(boolean_true_obj->Clone()),
- m_ObjHolder->AddIndirectObject(number_int_obj->Clone()),
- m_ObjHolder->AddIndirectObject(str_spec_obj->Clone()),
- m_ObjHolder->AddIndirectObject(name_obj->Clone()),
- m_ObjHolder->AddIndirectObject(m_ArrayObj->Clone()),
- m_ObjHolder->AddIndirectObject(m_DictObj->Clone()),
- m_ObjHolder->AddIndirectObject(stream_obj->Clone())};
- for (CPDF_Object* pObj : m_IndirectObjs) {
- m_RefObjs.emplace_back(pdfium::MakeRetain<CPDF_Reference>(
- m_ObjHolder.get(), pObj->GetObjNum()));
+ m_IndirectObjNums = {
+ m_ObjHolder->AddIndirectObject(boolean_true_obj->Clone()),
+ m_ObjHolder->AddIndirectObject(number_int_obj->Clone()),
+ m_ObjHolder->AddIndirectObject(str_spec_obj->Clone()),
+ m_ObjHolder->AddIndirectObject(name_obj->Clone()),
+ m_ObjHolder->AddIndirectObject(m_ArrayObj->Clone()),
+ m_ObjHolder->AddIndirectObject(m_DictObj->Clone()),
+ m_ObjHolder->AddIndirectObject(stream_obj->Clone())};
+ for (uint32_t objnum : m_IndirectObjNums) {
+ m_RefObjs.emplace_back(
+ pdfium::MakeRetain<CPDF_Reference>(m_ObjHolder.get(), objnum));
}
}
@@ -194,7 +195,7 @@
RetainPtr<CPDF_Dictionary> m_DictObj;
RetainPtr<CPDF_Dictionary> m_StreamDictObj;
RetainPtr<CPDF_Array> m_ArrayObj;
- std::vector<CPDF_Object*> m_IndirectObjs;
+ std::vector<uint32_t> m_IndirectObjNums;
};
TEST_F(PDFObjectsTest, GetString) {
@@ -336,7 +337,7 @@
// Check indirect references.
for (size_t i = 0; i < m_RefObjs.size(); ++i)
- EXPECT_EQ(m_IndirectObjs[i], m_RefObjs[i]->GetDirect());
+ EXPECT_EQ(m_IndirectObjNums[i], m_RefObjs[i]->GetDirect()->GetObjNum());
}
TEST_F(PDFObjectsTest, SetString) {
diff --git a/fpdfsdk/fpdf_ppo.cpp b/fpdfsdk/fpdf_ppo.cpp
index 832826a..72c76b4 100644
--- a/fpdfsdk/fpdf_ppo.cpp
+++ b/fpdfsdk/fpdf_ppo.cpp
@@ -358,11 +358,9 @@
return 0;
}
- RetainPtr<CPDF_Object> pIndirectClone(
- dest()->AddIndirectObject(std::move(pClone)));
- dwNewObjNum = pIndirectClone->GetObjNum();
+ dwNewObjNum = dest()->AddIndirectObject(pClone);
AddObjectMapping(dwObjnum, dwNewObjNum);
- if (!UpdateReference(std::move(pIndirectClone)))
+ if (!UpdateReference(std::move(pClone)))
return 0;
return dwNewObjNum;
