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;