Bad indexing in CPDF_Document::FindPageIndex when page tree corrupt.

Moving to std::vector from the more forgiving CFX_ArrayTemplate
revealed the dubious page tree traversal, which depends on the
correctness of the /Count entries to properly summarize the total
descendants under a given node.

The only "correct" thing to do is to throw away these counts as parsed,
and re-compute them, perhaps in CountPages().  But I'm not willing to do
that since it may break unknown documents in the wild.

Pass out-params as pointers while we're at it.

BUG=680376

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

2 files changed, 23 insertions, 19 deletions

diff --git a/core/fpdfapi/parser/cpdf_document.cpp b/core/fpdfapi/parser/cpdf_document.cpp
index 9e60aaa882..e425cfc328 100644
--- a/core/fpdfapi/parser/cpdf_document.cpp
+++ b/core/fpdfapi/parser/cpdf_document.cpp
@@ -516,18 +516,18 @@ void CPDF_Document::SetPageObjNum(int iPage, uint32_t objNum) {
 }
 
 int CPDF_Document::FindPageIndex(CPDF_Dictionary* pNode,
-                                 uint32_t& skip_count,
+                                 uint32_t* skip_count,
                                  uint32_t objnum,
-                                 int& index,
+                                 int* index,
                                  int level) {
   if (!pNode->KeyExist("Kids")) {
     if (objnum == pNode->GetObjNum())
-      return index;
+      return *index;
 
-    if (skip_count)
-      skip_count--;
+    if (*skip_count)
+      (*skip_count)--;
 
-    index++;
+    (*index)++;
     return -1;
   }
 
@@ -539,20 +539,17 @@ int CPDF_Document::FindPageIndex(CPDF_Dictionary* pNode,
     return -1;
 
   size_t count = pNode->GetIntegerFor("Count");
-  if (count <= skip_count) {
-    skip_count -= count;
-    index += count;
+  if (count <= *skip_count) {
+    (*skip_count) -= count;
+    (*index) += count;
     return -1;
   }
 
   if (count && count == pKidList->GetCount()) {
     for (size_t i = 0; i < count; i++) {
-      if (CPDF_Reference* pKid = ToReference(pKidList->GetObjectAt(i))) {
-        if (pKid->GetRefObjNum() == objnum) {
-          m_PageList[index + i] = objnum;
-          return static_cast<int>(index + i);
-        }
-      }
+      CPDF_Reference* pKid = ToReference(pKidList->GetObjectAt(i));
+      if (pKid && pKid->GetRefObjNum() == objnum)
+        return static_cast<int>(*index + i);
     }
   }
 
@@ -585,8 +582,15 @@ int CPDF_Document::GetPageIndex(uint32_t objnum) {
   if (!pPages)
     return -1;
 
-  int index = 0;
-  return FindPageIndex(pPages, skip_count, objnum, index);
+  int start_index = 0;
+  int found_index = FindPageIndex(pPages, &skip_count, objnum, &start_index);
+
+  // Corrupt page tree may yield out-of-range results.
+  if (found_index < 0 || found_index >= pdfium::CollectionSize<int>(m_PageList))
+    return -1;
+
+  m_PageList[found_index] = objnum;
+  return found_index;
 }
 
 int CPDF_Document::GetPageCount() const {
diff --git a/core/fpdfapi/parser/cpdf_document.h b/core/fpdfapi/parser/cpdf_document.h
index 65455480a4..3848ad6220 100644
--- a/core/fpdfapi/parser/cpdf_document.h
+++ b/core/fpdfapi/parser/cpdf_document.h
@@ -106,9 +106,9 @@ class CPDF_Document : public CPDF_IndirectObjectHolder {
   // When this method is called, m_pTreeTraversal[level] exists.
   CPDF_Dictionary* TraversePDFPages(int iPage, int* nPagesToGo, size_t level);
   int FindPageIndex(CPDF_Dictionary* pNode,
-                    uint32_t& skip_count,
+                    uint32_t* skip_count,
                     uint32_t objnum,
-                    int& index,
+                    int* index,
                     int level = 0);
   std::unique_ptr<CPDF_Object> ParseIndirectObject(uint32_t objnum) override;
   void LoadDocInternal();