Assert that only 0-numbered objects are Released()

This condition holds because numbered objects are brute-force
deleted by the indirect object holder, rather than being
released.

Be careful about recursive deletion, check before advancing,
since we no longer count on Release() doing this for us.
Fix a few tests where the test was violating ownership rules.

This should be the last step before completely removing Release()
in favor of direct delete everywhere.

Review-Url: https://codereview.chromium.org/2375343004

6 files changed, 47 insertions, 36 deletions

diff --git a/core/fpdfapi/fpdf_parser/cpdf_array.cpp b/core/fpdfapi/fpdf_parser/cpdf_array.cpp
index 26e65bca34..a3be55b9f0 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_array.cpp
+++ b/core/fpdfapi/fpdf_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/fpdf_parser/cpdf_dictionary.cpp b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp
index 5696fc0c8d..eb40de7d49 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/fpdf_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/fpdf_parser/cpdf_object.cpp b/core/fpdfapi/fpdf_parser/cpdf_object.cpp
index ba7490a13f..d74003112f 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_object.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_object.cpp
@@ -38,9 +38,7 @@ CPDF_Object* CPDF_Object::CloneNonCyclic(
 }
 
 void CPDF_Object::Release() {
-  if (m_ObjNum)
-    return;
-
+  CHECK(!m_ObjNum);
   delete this;
 }
 
diff --git a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
index 9b702099bb..6181571229 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
@@ -25,6 +25,7 @@ namespace {
 using ScopedArray = std::unique_ptr<CPDF_Array, ReleaseDeleter<CPDF_Array>>;
 using ScopedDict =
     std::unique_ptr<CPDF_Dictionary, ReleaseDeleter<CPDF_Dictionary>>;
+using ScopedStream = std::unique_ptr<CPDF_Stream, ReleaseDeleter<CPDF_Stream>>;
 
 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) {
@@ -787,10 +790,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);
 
@@ -806,6 +808,22 @@ TEST(PDFObjectTest, CloneCheckLoop) {
     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.
     CPDF_Dictionary* dict_obj = new CPDF_Dictionary();
diff --git a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp b/core/fpdfapi/fpdf_parser/cpdf_parser.cpp
index 182d3869bc..bc3dfe6021 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp
+++ b/core/fpdfapi/fpdf_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<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 |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<uint32_t> 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/fpdf_parser/cpdf_stream.cpp b/core/fpdfapi/fpdf_parser/cpdf_stream.cpp
index 4f6d046397..6a0bf05937 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_stream.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_stream.cpp
@@ -19,7 +19,11 @@ CPDF_Stream::CPDF_Stream(uint8_t* pData, uint32_t size, CPDF_Dictionary* 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;