Assert that only 0-numbered objects are Released()

core/fpdfapi/fpdf_parser/cpdf_parser.cpp

Index: core/fpdfapi/fpdf_parser/cpdf_parser.cpp
diff --git a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp b/core/fpdfapi/fpdf_parser/cpdf_parser.cpp
index 182d3869bcf1b867ca42032134d0bccc75a94866..860180402d8f2a9e03eee86da0c6c20a326dd47d 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_parser.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_parser.cpp
@@ -956,36 +956,35 @@ 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;
 
   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)) {
-      return FALSE;
-    }
   }
 
   CPDF_Stream* pStream = pObject->AsStream();

[Lei Zhang, 2016/09/30 23:49:01]

Do you know if |pObject| is expected to be a stream? Can we move this up to line
963 and bail out early if it's not a stream? Then the if (m_pDocument) block
that has now been split in two can become one again.

[Tom Sepez, 2016/10/01 00:13:11]

Done.

   if (!pStream)
     return FALSE;
 
+  if (m_pDocument) {
+    // Takes ownership of object (std::move someday).
+    uint32_t objnum = pObject->m_ObjNum;
+    if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration(

[Lei Zhang, 2016/09/30 23:49:01]

Or should this stay up with the other if (m_pDocument) block? By moving this
down, do we miss out on objects with higher generation numbers?

[Tom Sepez, 2016/10/01 00:13:11]

Done.

[Lei Zhang, 2016/10/01 01:24:06]

So I still wonder if changing the order of AsStream() and
ReplaceIndirectObjectIfHigherGeneration() will effect parsing of PDFs with
objects with generation numbers. I'm worried we don't have enough test coverage
there.

[Tom Sepez, 2016/10/03 17:35:05]

Done.

+            objnum, pObject.release())) {
+      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 +1016,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 +1026,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 +1087,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;
 }