From 836f7d5d23b0951c8be86ccca54ebad43179febe Mon Sep 17 00:00:00 2001 From: tsepez Date: Mon, 10 Oct 2016 14:31:05 -0700 Subject: Land all the fixes from 5609f39c but don't enable assert Split this off so that we don't keep losing this when the assert is reverted again. Review-Url: https://codereview.chromium.org/2401423005 --- core/fpdfapi/parser/cpdf_array.cpp | 2 +- core/fpdfapi/parser/cpdf_dictionary.cpp | 2 +- core/fpdfapi/parser/cpdf_object_unittest.cpp | 32 ++++++++++++++++++------ core/fpdfapi/parser/cpdf_parser.cpp | 37 +++++++++++----------------- core/fpdfapi/parser/cpdf_stream.cpp | 6 ++++- 5 files changed, 46 insertions(+), 33 deletions(-) diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp index 9a2dba0161..0d0c02f28d 100644 --- a/core/fpdfapi/parser/cpdf_array.cpp +++ b/core/fpdfapi/parser/cpdf_array.cpp @@ -23,7 +23,7 @@ CPDF_Array::~CPDF_Array() { // in case of cyclic references. m_ObjNum = kInvalidObjNum; for (auto& it : m_Objects) { - if (it) + if (it && it->GetObjNum() != kInvalidObjNum) it->Release(); } } diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp index 54aafd43b1..2aa5248be0 100644 --- a/core/fpdfapi/parser/cpdf_dictionary.cpp +++ b/core/fpdfapi/parser/cpdf_dictionary.cpp @@ -30,7 +30,7 @@ CPDF_Dictionary::~CPDF_Dictionary() { // in case of cyclic references. m_ObjNum = kInvalidObjNum; for (const auto& it : m_Map) { - if (it.second) + if (it.second && it.second->GetObjNum() != kInvalidObjNum) it.second->Release(); } } diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp index 502d2a06d2..8215226ef3 100644 --- a/core/fpdfapi/parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp @@ -25,6 +25,7 @@ namespace { using ScopedArray = std::unique_ptr>; using ScopedDict = std::unique_ptr>; +using ScopedStream = std::unique_ptr>; void TestArrayAccessors(const CPDF_Array* arr, size_t index, @@ -95,8 +96,10 @@ class PDFObjectsTest : public testing::Test { // Indirect references to indirect objects. m_ObjHolder.reset(new CPDF_IndirectObjectHolder()); - m_IndirectObjs = {boolean_true_obj, number_int_obj, str_spec_obj, name_obj, - m_ArrayObj, m_DictObj, stream_obj}; + m_IndirectObjs = {boolean_true_obj->Clone(), number_int_obj->Clone(), + str_spec_obj->Clone(), name_obj->Clone(), + m_ArrayObj->Clone(), m_DictObj->Clone(), + stream_obj->Clone()}; for (size_t i = 0; i < m_IndirectObjs.size(); ++i) { m_ObjHolder->AddIndirectObject(m_IndirectObjs[i]); m_RefObjs.emplace_back(new CPDF_Reference( @@ -104,7 +107,7 @@ class PDFObjectsTest : public testing::Test { } } - bool Equal(CPDF_Object* obj1, CPDF_Object* obj2) { + bool Equal(const CPDF_Object* obj1, const CPDF_Object* obj2) { if (obj1 == obj2) return true; if (!obj1 || !obj2 || obj1->GetType() != obj2->GetType()) @@ -253,7 +256,7 @@ TEST_F(PDFObjectsTest, GetDict) { const CPDF_Dictionary* const indirect_obj_results[] = { nullptr, nullptr, nullptr, nullptr, nullptr, m_DictObj, m_StreamDictObj}; for (size_t i = 0; i < m_RefObjs.size(); ++i) - EXPECT_EQ(indirect_obj_results[i], m_RefObjs[i]->GetDict()); + EXPECT_TRUE(Equal(indirect_obj_results[i], m_RefObjs[i]->GetDict())); } TEST_F(PDFObjectsTest, GetArray) { @@ -802,10 +805,9 @@ TEST(PDFDictionaryTest, CloneDirectObject) { TEST(PDFObjectTest, CloneCheckLoop) { { - // Create an object with a reference loop. - ScopedArray arr_obj(new CPDF_Array); - // Dictionary object. + // Create a dictionary/array pair with a reference loop. CPDF_Dictionary* dict_obj = new CPDF_Dictionary(); + ScopedArray arr_obj(new CPDF_Array); dict_obj->SetFor("arr", arr_obj.get()); arr_obj->InsertAt(0, dict_obj); @@ -820,6 +822,22 @@ TEST(PDFObjectTest, CloneCheckLoop) { // Recursively referenced object is not cloned. EXPECT_EQ(nullptr, cloned_dict->AsDictionary()->GetObjectFor("arr")); } + { + // Create a dictionary/stream pair with a reference loop. + CPDF_Dictionary* dict_obj = new CPDF_Dictionary(); + ScopedStream stream_obj(new CPDF_Stream(nullptr, 0, dict_obj)); + dict_obj->SetFor("stream", stream_obj.get()); + + // Clone this object to see whether stack overflow will be triggered. + ScopedStream cloned_stream(stream_obj->Clone()->AsStream()); + // Cloned object should be the same as the original. + ASSERT_TRUE(cloned_stream); + CPDF_Object* cloned_dict = cloned_stream->GetDict(); + ASSERT_TRUE(cloned_dict); + ASSERT_TRUE(cloned_dict->IsDictionary()); + // Recursively referenced object is not cloned. + EXPECT_EQ(nullptr, cloned_dict->AsDictionary()->GetObjectFor("stream")); + } { CPDF_IndirectObjectHolder objects_holder; // Create an object with a reference loop. diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp index 17539101a1..f4cde0c198 100644 --- a/core/fpdfapi/parser/cpdf_parser.cpp +++ b/core/fpdfapi/parser/cpdf_parser.cpp @@ -956,36 +956,34 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { } FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { - CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument, *pos, 0); + std::unique_ptr pObject( + ParseIndirectObjectAt(m_pDocument, *pos, 0)); if (!pObject) return FALSE; + CPDF_Object* pUnownedObject = pObject.get(); + if (m_pDocument) { CPDF_Dictionary* pRootDict = m_pDocument->GetRoot(); - if (pRootDict && pRootDict->GetObjNum() == pObject->m_ObjNum) { - // If |pObject| has an objnum assigned then this will leak as Release() - // will early exit. - if (pObject->IsStream()) - pObject->Release(); + if (pRootDict && pRootDict->GetObjNum() == pObject->m_ObjNum) return FALSE; - } - if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration(pObject->m_ObjNum, - pObject)) { + // Takes ownership of object (std::move someday). + uint32_t objnum = pObject->m_ObjNum; + if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( + objnum, pObject.release())) { return FALSE; } } - CPDF_Stream* pStream = pObject->AsStream(); + CPDF_Stream* pStream = pUnownedObject->AsStream(); if (!pStream) return FALSE; CPDF_Dictionary* pDict = pStream->GetDict(); *pos = pDict->GetIntegerFor("Prev"); int32_t size = pDict->GetIntegerFor("Size"); - if (size < 0) { - pStream->Release(); + if (size < 0) return FALSE; - } CPDF_Dictionary* pNewTrailer = ToDictionary(pDict->Clone()); if (bMainXRef) { @@ -1017,10 +1015,8 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { arrIndex.push_back(std::make_pair(0, size)); pArray = pDict->GetArrayFor("W"); - if (!pArray) { - pStream->Release(); + if (!pArray) return FALSE; - } CFX_ArrayTemplate WidthArray; FX_SAFE_UINT32 dwAccWidth = 0; @@ -1029,10 +1025,8 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { dwAccWidth += WidthArray[i]; } - if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3) { - pStream->Release(); + if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3) return FALSE; - } uint32_t totalWidth = dwAccWidth.ValueOrDie(); CPDF_StreamAcc acc; @@ -1092,17 +1086,14 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { if (type == 1) { m_SortedOffset.insert(offset); } else { - if (offset < 0 || !IsValidObjectNumber(offset)) { - pStream->Release(); + if (offset < 0 || !IsValidObjectNumber(offset)) return FALSE; - } m_ObjectInfo[offset].type = 255; } } } segindex += count; } - pStream->Release(); return TRUE; } diff --git a/core/fpdfapi/parser/cpdf_stream.cpp b/core/fpdfapi/parser/cpdf_stream.cpp index c221edec68..935c80c5c8 100644 --- a/core/fpdfapi/parser/cpdf_stream.cpp +++ b/core/fpdfapi/parser/cpdf_stream.cpp @@ -17,7 +17,11 @@ CPDF_Stream::CPDF_Stream() {} CPDF_Stream::CPDF_Stream(uint8_t* pData, uint32_t size, CPDF_Dictionary* pDict) : m_pDict(pDict), m_dwSize(size), m_pDataBuf(pData) {} -CPDF_Stream::~CPDF_Stream() {} +CPDF_Stream::~CPDF_Stream() { + m_ObjNum = kInvalidObjNum; + if (m_pDict && m_pDict->GetObjNum() == kInvalidObjNum) + m_pDict.release(); // lowercase release, release ownership. +} CPDF_Object::Type CPDF_Stream::GetType() const { return STREAM; -- cgit v1.2.3