From 22b176d0ee7f1dcbc7bca6e5eef65c19fa10f726 Mon Sep 17 00:00:00 2001 From: thestig Date: Thu, 25 Aug 2016 11:14:21 -0700 Subject: Check for nullptrs in CPDF_Dictionary dtor. BUG=597440 Review-Url: https://codereview.chromium.org/2273293003 --- core/fpdfapi/fpdf_parser/cpdf_array.cpp | 23 +++++++----- core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp | 11 +++++- core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp | 45 +++++++++++++++++++++-- 3 files changed, 63 insertions(+), 16 deletions(-) diff --git a/core/fpdfapi/fpdf_parser/cpdf_array.cpp b/core/fpdfapi/fpdf_parser/cpdf_array.cpp index 83f99c215b..5e103fa423 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_array.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_array.cpp @@ -6,6 +6,8 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_array.h" +#include + #include "core/fpdfapi/fpdf_parser/include/cpdf_name.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_number.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_reference.h" @@ -51,7 +53,7 @@ CPDF_Object* CPDF_Array::CloneNonCyclic( pVisited->insert(this); CPDF_Array* pCopy = new CPDF_Array(); for (size_t i = 0; i < GetCount(); i++) { - CPDF_Object* value = m_Objects.at(i); + CPDF_Object* value = m_Objects[i]; if (!pdfium::ContainsKey(*pVisited, value)) pCopy->m_Objects.push_back(value->CloneNonCyclic(bDirect, pVisited)); } @@ -83,31 +85,31 @@ CFX_Matrix CPDF_Array::GetMatrix() { CPDF_Object* CPDF_Array::GetObjectAt(size_t i) const { if (i >= m_Objects.size()) return nullptr; - return m_Objects.at(i); + return m_Objects[i]; } CPDF_Object* CPDF_Array::GetDirectObjectAt(size_t i) const { if (i >= m_Objects.size()) return nullptr; - return m_Objects.at(i)->GetDirect(); + return m_Objects[i]->GetDirect(); } CFX_ByteString CPDF_Array::GetStringAt(size_t i) const { if (i >= m_Objects.size()) return CFX_ByteString(); - return m_Objects.at(i)->GetString(); + return m_Objects[i]->GetString(); } int CPDF_Array::GetIntegerAt(size_t i) const { if (i >= m_Objects.size()) return 0; - return m_Objects.at(i)->GetInteger(); + return m_Objects[i]->GetInteger(); } FX_FLOAT CPDF_Array::GetNumberAt(size_t i) const { if (i >= m_Objects.size()) return 0; - return m_Objects.at(i)->GetNumber(); + return m_Objects[i]->GetNumber(); } CPDF_Dictionary* CPDF_Array::GetDictAt(size_t i) const { @@ -137,7 +139,7 @@ void CPDF_Array::RemoveAt(size_t i, size_t nCount) { return; for (size_t j = 0; j < nCount; ++j) { - if (CPDF_Object* p = m_Objects.at(i + j)) + if (CPDF_Object* p = m_Objects[i + j]) p->Release(); } m_Objects.erase(m_Objects.begin() + i, m_Objects.begin() + i + nCount); @@ -147,10 +149,11 @@ void CPDF_Array::SetAt(size_t i, CPDF_Object* pObj, CPDF_IndirectObjectHolder* pObjs) { ASSERT(IsArray()); - ASSERT(i < m_Objects.size()); - if (i >= m_Objects.size()) + if (i >= m_Objects.size()) { + ASSERT(false); return; - if (CPDF_Object* pOld = m_Objects.at(i)) + } + if (CPDF_Object* pOld = m_Objects[i]) pOld->Release(); if (pObj->GetObjNum()) { ASSERT(pObjs); diff --git a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp index 8fef074d4b..e79bac1839 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp @@ -6,6 +6,9 @@ #include "core/fpdfapi/fpdf_parser/include/cpdf_dictionary.h" +#include +#include + #include "core/fpdfapi/fpdf_parser/cpdf_boolean.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_array.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_name.h" @@ -18,9 +21,13 @@ CPDF_Dictionary::CPDF_Dictionary() {} CPDF_Dictionary::~CPDF_Dictionary() { + // Mark the object as deleted so that it will not be deleted again + // in case of cyclic references. m_ObjNum = kInvalidObjNum; - for (const auto& it : m_Map) - it.second->Release(); + for (const auto& it : m_Map) { + if (it.second) + it.second->Release(); + } } CPDF_Object::Type CPDF_Dictionary::GetType() const { diff --git a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp index 02e50bcfec..ec982ab58a 100644 --- a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp @@ -747,6 +747,44 @@ TEST(PDFArrayTest, AddReferenceAndGetObjectAt) { } } +TEST(PDFArrayTest, CloneDirectObject) { + CPDF_IndirectObjectHolder objects_holder; + ScopedArray array(new CPDF_Array); + array->AddReference(&objects_holder, 1234); + ASSERT_EQ(1U, array->GetCount()); + CPDF_Object* obj = array->GetObjectAt(0); + ASSERT_TRUE(obj); + EXPECT_TRUE(obj->IsReference()); + + CPDF_Object* cloned_array_object = array->CloneDirectObject(); + ASSERT_TRUE(cloned_array_object); + ASSERT_TRUE(cloned_array_object->IsArray()); + + ScopedArray cloned_array(cloned_array_object->AsArray()); + ASSERT_EQ(1U, cloned_array->GetCount()); + CPDF_Object* cloned_obj = cloned_array->GetObjectAt(0); + EXPECT_FALSE(cloned_obj); +} + +TEST(PDFDictionaryTest, CloneDirectObject) { + CPDF_IndirectObjectHolder objects_holder; + ScopedDict dict(new CPDF_Dictionary); + dict->SetAtReference("foo", &objects_holder, 1234); + ASSERT_EQ(1U, dict->GetCount()); + CPDF_Object* obj = dict->GetObjectBy("foo"); + ASSERT_TRUE(obj); + EXPECT_TRUE(obj->IsReference()); + + CPDF_Object* cloned_dict_object = dict->CloneDirectObject(); + ASSERT_TRUE(cloned_dict_object); + ASSERT_TRUE(cloned_dict_object->IsDictionary()); + + ScopedDict cloned_dict(cloned_dict_object->AsDictionary()); + ASSERT_EQ(1U, cloned_dict->GetCount()); + CPDF_Object* cloned_obj = cloned_dict->GetObjectBy("foo"); + EXPECT_FALSE(cloned_obj); +} + TEST(PDFObjectTest, CloneCheckLoop) { { // Create an object with a reference loop. @@ -768,15 +806,14 @@ TEST(PDFObjectTest, CloneCheckLoop) { EXPECT_EQ(nullptr, cloned_dict->AsDictionary()->GetObjectBy("arr")); } { - std::unique_ptr m_ObjHolder( - new CPDF_IndirectObjectHolder()); + CPDF_IndirectObjectHolder objects_holder; // Create an object with a reference loop. CPDF_Dictionary* dict_obj = new CPDF_Dictionary; CPDF_Array* arr_obj = new CPDF_Array; - m_ObjHolder->AddIndirectObject(dict_obj); + objects_holder.AddIndirectObject(dict_obj); EXPECT_EQ(1u, dict_obj->GetObjNum()); dict_obj->SetAt("arr", arr_obj); - arr_obj->InsertAt(0, dict_obj, m_ObjHolder.get()); + arr_obj->InsertAt(0, dict_obj, &objects_holder); CPDF_Object* elem0 = arr_obj->GetObjectAt(0); ASSERT_TRUE(elem0); ASSERT_TRUE(elem0->IsReference()); -- cgit v1.2.3