Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was passing an indirect object.

Also check that the invariant isn't violated, we need to fail
here in the wild and investigate -- these are existing UAFs.

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

6 files changed, 20 insertions, 38 deletions

diff --git a/core/fpdfapi/fpdf_parser/cpdf_array.cpp b/core/fpdfapi/fpdf_parser/cpdf_array.cpp
index 5e103fa423..da935af47d 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_array.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_array.cpp
@@ -13,6 +13,7 @@
 #include "core/fpdfapi/fpdf_parser/include/cpdf_reference.h"
 #include "core/fpdfapi/fpdf_parser/include/cpdf_stream.h"
 #include "core/fpdfapi/fpdf_parser/include/cpdf_string.h"
+#include "third_party/base/logging.h"
 #include "third_party/base/stl_util.h"
 
 CPDF_Array::CPDF_Array() {}
@@ -145,30 +146,22 @@ void CPDF_Array::RemoveAt(size_t i, size_t nCount) {
   m_Objects.erase(m_Objects.begin() + i, m_Objects.begin() + i + nCount);
 }
 
-void CPDF_Array::SetAt(size_t i,
-                       CPDF_Object* pObj,
-                       CPDF_IndirectObjectHolder* pObjs) {
+void CPDF_Array::SetAt(size_t i, CPDF_Object* pObj) {
   ASSERT(IsArray());
+  CHECK(!pObj || pObj->GetObjNum() == 0);
   if (i >= m_Objects.size()) {
     ASSERT(false);
     return;
   }
   if (CPDF_Object* pOld = m_Objects[i])
     pOld->Release();
-  if (pObj->GetObjNum()) {
-    ASSERT(pObjs);
-    pObj = new CPDF_Reference(pObjs, pObj->GetObjNum());
-  }
+
   m_Objects[i] = pObj;
 }
 
-void CPDF_Array::InsertAt(size_t index,
-                          CPDF_Object* pObj,
-                          CPDF_IndirectObjectHolder* pObjs) {
-  if (pObj->GetObjNum()) {
-    ASSERT(pObjs);
-    pObj = new CPDF_Reference(pObjs, pObj->GetObjNum());
-  }
+void CPDF_Array::InsertAt(size_t index, CPDF_Object* pObj) {
+  ASSERT(IsArray());
+  CHECK(!pObj || pObj->GetObjNum() == 0);
   if (index >= m_Objects.size()) {
     // Allocate space first.
     m_Objects.resize(index + 1, nullptr);
@@ -179,37 +172,29 @@ void CPDF_Array::InsertAt(size_t index,
   }
 }
 
-void CPDF_Array::Add(CPDF_Object* pObj, CPDF_IndirectObjectHolder* pObjs) {
-  if (pObj->GetObjNum()) {
-    ASSERT(pObjs);
-    pObj = new CPDF_Reference(pObjs, pObj->GetObjNum());
-  }
+void CPDF_Array::Add(CPDF_Object* pObj) {
+  ASSERT(IsArray());
+  CHECK(!pObj || pObj->GetObjNum() == 0);
   m_Objects.push_back(pObj);
 }
 
 void CPDF_Array::AddName(const CFX_ByteString& str) {
-  ASSERT(IsArray());
   Add(new CPDF_Name(str));
 }
 
 void CPDF_Array::AddString(const CFX_ByteString& str) {
-  ASSERT(IsArray());
   Add(new CPDF_String(str, FALSE));
 }
 
 void CPDF_Array::AddInteger(int i) {
-  ASSERT(IsArray());
   Add(new CPDF_Number(i));
 }
 
 void CPDF_Array::AddNumber(FX_FLOAT f) {
-  ASSERT(IsArray());
-  CPDF_Number* pNumber = new CPDF_Number(f);
-  Add(pNumber);
+  Add(new CPDF_Number(f));
 }
 
 void CPDF_Array::AddReference(CPDF_IndirectObjectHolder* pDoc,
                               uint32_t objnum) {
-  ASSERT(IsArray());
   Add(new CPDF_Reference(pDoc, objnum));
 }
diff --git a/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp
index ef9b65c5c4..f08ee6ca63 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_array_unittest.cpp
@@ -117,8 +117,7 @@ TEST(cpdf_array, Clone) {
         // Starts object number from 1.
         int obj_num = i * kNumOfRowElems + j + 1;
         obj_holder->ReplaceIndirectObjectIfHigherGeneration(obj_num, obj);
-        arr_elem->InsertAt(j, new CPDF_Reference(obj_holder.get(), obj_num),
-                           obj_holder.get());
+        arr_elem->InsertAt(j, new CPDF_Reference(obj_holder.get(), obj_num));
       }
       arr->InsertAt(i, arr_elem);
     }
diff --git a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp
index aeee3382f9..1a84fcb142 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_dictionary.cpp
@@ -17,6 +17,7 @@
 #include "core/fpdfapi/fpdf_parser/include/cpdf_stream.h"
 #include "core/fpdfapi/fpdf_parser/include/cpdf_string.h"
 #include "third_party/base/stl_util.h"
+#include "third_party/base/logging.h"
 
 CPDF_Dictionary::CPDF_Dictionary() {}
 
@@ -169,7 +170,7 @@ bool CPDF_Dictionary::IsSignatureDict() const {
 }
 
 void CPDF_Dictionary::SetFor(const CFX_ByteString& key, CPDF_Object* pObj) {
-  ASSERT(!pObj || pObj->GetObjNum() == 0);
+  CHECK(!pObj || pObj->GetObjNum() == 0);
   auto it = m_Map.find(key);
   if (it == m_Map.end()) {
     if (pObj)
diff --git a/core/fpdfapi/fpdf_parser/cpdf_document.cpp b/core/fpdfapi/fpdf_parser/cpdf_document.cpp
index 020e3544e9..bd11ed26e6 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_document.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_document.cpp
@@ -302,7 +302,7 @@ int InsertNewPage(CPDF_Document* pDoc,
       pPagesList = new CPDF_Array;
       pPages->SetFor("Kids", pPagesList);
     }
-    pPagesList->Add(pPageDict, pDoc);
+    pPagesList->Add(new CPDF_Reference(pDoc, pPageDict->GetObjNum()));
     pPages->SetIntegerFor("Count", nPages + 1);
     pPageDict->SetReferenceFor("Parent", pDoc, pPages->GetObjNum());
   } else {
diff --git a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
index 5772043367..cbfdf2b9ac 100644
--- a/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
+++ b/core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp
@@ -813,7 +813,8 @@ TEST(PDFObjectTest, CloneCheckLoop) {
     objects_holder.AddIndirectObject(dict_obj);
     EXPECT_EQ(1u, dict_obj->GetObjNum());
     dict_obj->SetFor("arr", arr_obj);
-    arr_obj->InsertAt(0, dict_obj, &objects_holder);
+    arr_obj->InsertAt(
+        0, new CPDF_Reference(&objects_holder, dict_obj->GetObjNum()));
     CPDF_Object* elem0 = arr_obj->GetObjectAt(0);
     ASSERT_TRUE(elem0);
     ASSERT_TRUE(elem0->IsReference());
diff --git a/core/fpdfapi/fpdf_parser/include/cpdf_array.h b/core/fpdfapi/fpdf_parser/include/cpdf_array.h
index 8c89a060eb..1e8c612710 100644
--- a/core/fpdfapi/fpdf_parser/include/cpdf_array.h
+++ b/core/fpdfapi/fpdf_parser/include/cpdf_array.h
@@ -43,15 +43,11 @@ class CPDF_Array : public CPDF_Object {
   CFX_Matrix GetMatrix();
   CFX_FloatRect GetRect();
 
-  void SetAt(size_t index,
-             CPDF_Object* pObj,
-             CPDF_IndirectObjectHolder* pObjs = nullptr);
-  void InsertAt(size_t index,
-                CPDF_Object* pObj,
-                CPDF_IndirectObjectHolder* pObjs = nullptr);
+  void SetAt(size_t index, CPDF_Object* pObj);
+  void InsertAt(size_t index, CPDF_Object* pObj);
   void RemoveAt(size_t index, size_t nCount = 1);
 
-  void Add(CPDF_Object* pObj, CPDF_IndirectObjectHolder* pObjs = nullptr);
+  void Add(CPDF_Object* pObj);
   void AddNumber(FX_FLOAT f);
   void AddInteger(int i);
   void AddString(const CFX_ByteString& str);