From 5609f39c9d4534733f04a2be631da56948c2e96f Mon Sep 17 00:00:00 2001 From: tsepez Date: Mon, 10 Oct 2016 11:25:02 -0700 Subject: Revert "Re-land "Assert that only 0-numbered objects are Released()"" This reverts commit b69a98cf50537130f88ce3a799117f2ca8353ac5. Reason for revert: crashes on mac. BUG=654387 TBR=thestig@chromium.org Review-Url: https://codereview.chromium.org/2410483002 --- core/fpdfapi/parser/cpdf_array.cpp | 2 +- core/fpdfapi/parser/cpdf_dictionary.cpp | 2 +- core/fpdfapi/parser/cpdf_object.cpp | 4 ++- core/fpdfapi/parser/cpdf_object_unittest.cpp | 32 ++++++------------------ core/fpdfapi/parser/cpdf_parser.cpp | 37 +++++++++++++++++----------- core/fpdfapi/parser/cpdf_stream.cpp | 6 +---- 6 files changed, 36 insertions(+), 47 deletions(-) diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp index 0d0c02f28d..9a2dba0161 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 && it->GetObjNum() != kInvalidObjNum) + if (it) it->Release(); } } diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp index 31f520af0d..7ef5a53551 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 && it.second->GetObjNum() != kInvalidObjNum) + if (it.second) it.second->Release(); } } diff --git a/core/fpdfapi/parser/cpdf_object.cpp b/core/fpdfapi/parser/cpdf_object.cpp index 5c407336e2..3bf0a953db 100644 --- a/core/fpdfapi/parser/cpdf_object.cpp +++ b/core/fpdfapi/parser/cpdf_object.cpp @@ -38,7 +38,9 @@ CPDF_Object* CPDF_Object::CloneNonCyclic( } void CPDF_Object::Release() { - CHECK(!m_ObjNum); + if (m_ObjNum) + return; + delete this; } diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp index 657f536c26..b2177afd8a 100644 --- a/core/fpdfapi/parser/cpdf_object_unittest.cpp +++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp @@ -25,7 +25,6 @@ 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, @@ -96,10 +95,8 @@ class PDFObjectsTest : public testing::Test { // Indirect references to indirect objects. m_ObjHolder.reset(new CPDF_IndirectObjectHolder()); - 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()}; + m_IndirectObjs = {boolean_true_obj, number_int_obj, str_spec_obj, name_obj, + m_ArrayObj, m_DictObj, stream_obj}; for (size_t i = 0; i < m_IndirectObjs.size(); ++i) { m_ObjHolder->AddIndirectObject(m_IndirectObjs[i]); m_RefObjs.emplace_back(new CPDF_Reference( @@ -107,7 +104,7 @@ class PDFObjectsTest : public testing::Test { } } - bool Equal(const CPDF_Object* obj1, const CPDF_Object* obj2) { + bool Equal(CPDF_Object* obj1, CPDF_Object* obj2) { if (obj1 == obj2) return true; if (!obj1 || !obj2 || obj1->GetType() != obj2->GetType()) @@ -256,7 +253,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_TRUE(Equal(indirect_obj_results[i], m_RefObjs[i]->GetDict())); + EXPECT_EQ(indirect_obj_results[i], m_RefObjs[i]->GetDict()); } TEST_F(PDFObjectsTest, GetArray) { @@ -790,9 +787,10 @@ TEST(PDFDictionaryTest, CloneDirectObject) { TEST(PDFObjectTest, CloneCheckLoop) { { - // Create a dictionary/array pair with a reference loop. - CPDF_Dictionary* dict_obj = new CPDF_Dictionary(); + // Create an object with a reference loop. ScopedArray arr_obj(new CPDF_Array); + // Dictionary object. + CPDF_Dictionary* dict_obj = new CPDF_Dictionary(); dict_obj->SetFor("arr", arr_obj.get()); arr_obj->InsertAt(0, dict_obj); @@ -807,22 +805,6 @@ 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 f4cde0c198..17539101a1 100644 --- a/core/fpdfapi/parser/cpdf_parser.cpp +++ b/core/fpdfapi/parser/cpdf_parser.cpp @@ -956,34 +956,36 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() { } FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { - std::unique_ptr pObject( - ParseIndirectObjectAt(m_pDocument, *pos, 0)); + CPDF_Object* 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 (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(); return FALSE; - // Takes ownership of object (std::move someday). - uint32_t objnum = pObject->m_ObjNum; - if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( - objnum, pObject.release())) { + } + if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration(pObject->m_ObjNum, + pObject)) { return FALSE; } } - CPDF_Stream* pStream = pUnownedObject->AsStream(); + CPDF_Stream* pStream = pObject->AsStream(); if (!pStream) return FALSE; CPDF_Dictionary* pDict = pStream->GetDict(); *pos = pDict->GetIntegerFor("Prev"); int32_t size = pDict->GetIntegerFor("Size"); - if (size < 0) + if (size < 0) { + pStream->Release(); return FALSE; + } CPDF_Dictionary* pNewTrailer = ToDictionary(pDict->Clone()); if (bMainXRef) { @@ -1015,8 +1017,10 @@ 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) + if (!pArray) { + pStream->Release(); return FALSE; + } CFX_ArrayTemplate WidthArray; FX_SAFE_UINT32 dwAccWidth = 0; @@ -1025,8 +1029,10 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { dwAccWidth += WidthArray[i]; } - if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3) + if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3) { + pStream->Release(); return FALSE; + } uint32_t totalWidth = dwAccWidth.ValueOrDie(); CPDF_StreamAcc acc; @@ -1086,14 +1092,17 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) { if (type == 1) { m_SortedOffset.insert(offset); } else { - if (offset < 0 || !IsValidObjectNumber(offset)) + if (offset < 0 || !IsValidObjectNumber(offset)) { + pStream->Release(); 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 935c80c5c8..c221edec68 100644 --- a/core/fpdfapi/parser/cpdf_stream.cpp +++ b/core/fpdfapi/parser/cpdf_stream.cpp @@ -17,11 +17,7 @@ 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() { - m_ObjNum = kInvalidObjNum; - if (m_pDict && m_pDict->GetObjNum() == kInvalidObjNum) - m_pDict.release(); // lowercase release, release ownership. -} +CPDF_Stream::~CPDF_Stream() {} CPDF_Object::Type CPDF_Stream::GetType() const { return STREAM; -- cgit v1.2.3