Prevent infinite looping in CPDF_Parser::LoadAllCrossRefV5().

BUG=pdfium:298
R=weili@chromium.org

Review URL: https://codereview.chromium.org/1496703005 .

5 files changed, 75 insertions, 14 deletions

diff --git a/core/include/fpdfapi/fpdf_parser.h b/core/include/fpdfapi/fpdf_parser.h
index ec005de812..dab0719f26 100644
--- a/core/include/fpdfapi/fpdf_parser.h
+++ b/core/include/fpdfapi/fpdf_parser.h
@@ -482,7 +482,7 @@ class CPDF_Parser {
                       FX_BOOL bSkip,
                       FX_BOOL bFirst);
 
-  FX_BOOL LoadCrossRefV5(FX_FILESIZE pos, FX_FILESIZE& prev, FX_BOOL bMainXRef);
+  FX_BOOL LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef);
 
   CPDF_Dictionary* LoadTrailerV4();
 
diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
index f64ba0deb0..bc5d3edc2b 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
@@ -582,21 +582,29 @@ bool CPDF_Parser::LoadCrossRefV4(FX_FILESIZE pos,
     }
     m_Syntax.RestorePos(SavedPos + count * recordsize);
   }
-  return !streampos || LoadCrossRefV5(streampos, streampos, FALSE);
+  return !streampos || LoadCrossRefV5(&streampos, FALSE);
 }
 
 FX_BOOL CPDF_Parser::LoadAllCrossRefV5(FX_FILESIZE xrefpos) {
-  if (!LoadCrossRefV5(xrefpos, xrefpos, TRUE)) {
+  if (!LoadCrossRefV5(&xrefpos, TRUE)) {
     return FALSE;
   }
-  while (xrefpos)
-    if (!LoadCrossRefV5(xrefpos, xrefpos, FALSE)) {
+  std::set<FX_FILESIZE> seen_xrefpos;
+  while (xrefpos) {
+    seen_xrefpos.insert(xrefpos);
+    if (!LoadCrossRefV5(&xrefpos, FALSE)) {
+      return FALSE;
+    }
+    // Check for circular references.
+    if (seen_xrefpos.find(xrefpos) != seen_xrefpos.end()) {
       return FALSE;
     }
+  }
   m_ObjectStreamMap.InitHashTable(101, FALSE);
   m_bXRefStream = TRUE;
   return TRUE;
 }
+
 FX_BOOL CPDF_Parser::RebuildCrossRef() {
   m_CrossRef.RemoveAll();
   m_V5Type.RemoveAll();
@@ -975,10 +983,8 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() {
   return m_pTrailer && m_CrossRef.GetSize() > 0;
 }
 
-FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE pos,
-                                    FX_FILESIZE& prev,
-                                    FX_BOOL bMainXRef) {
-  CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument, pos, 0, nullptr);
+FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) {
+  CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument, *pos, 0, nullptr);
   if (!pObject)
     return FALSE;
 
@@ -997,7 +1003,7 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE pos,
   if (!pStream)
     return FALSE;
 
-  prev = pStream->GetDict()->GetInteger(FX_BSTRC("Prev"));
+  *pos = pStream->GetDict()->GetInteger(FX_BSTRC("Prev"));
   int32_t size = pStream->GetDict()->GetInteger(FX_BSTRC("Size"));
   if (size < 0) {
     pStream->Release();
@@ -1563,7 +1569,7 @@ FX_DWORD CPDF_Parser::StartAsynParse(IFX_FileRead* pFileAccess,
   FX_BOOL bXRefRebuilt = FALSE;
   FX_BOOL bLoadV4 = FALSE;
   if (!(bLoadV4 = LoadCrossRefV4(dwFirstXRefOffset, 0, FALSE, FALSE)) &&
-      !LoadCrossRefV5(dwFirstXRefOffset, dwFirstXRefOffset, TRUE)) {
+      !LoadCrossRefV5(&dwFirstXRefOffset, TRUE)) {
     if (!RebuildCrossRef()) {
       return PDFPARSE_ERROR_FORMAT;
     }
@@ -1623,13 +1629,20 @@ FX_DWORD CPDF_Parser::StartAsynParse(IFX_FileRead* pFileAccess,
   return PDFPARSE_ERROR_SUCCESS;
 }
 FX_BOOL CPDF_Parser::LoadLinearizedAllCrossRefV5(FX_FILESIZE xrefpos) {
-  if (!LoadCrossRefV5(xrefpos, xrefpos, FALSE)) {
+  if (!LoadCrossRefV5(&xrefpos, FALSE)) {
     return FALSE;
   }
-  while (xrefpos)
-    if (!LoadCrossRefV5(xrefpos, xrefpos, FALSE)) {
+  std::set<FX_FILESIZE> seen_xrefpos;
+  while (xrefpos) {
+    seen_xrefpos.insert(xrefpos);
+    if (!LoadCrossRefV5(&xrefpos, FALSE)) {
+      return FALSE;
+    }
+    // Check for circular references.
+    if (seen_xrefpos.find(xrefpos) != seen_xrefpos.end()) {
       return FALSE;
     }
+  }
   m_ObjectStreamMap.InitHashTable(101, FALSE);
   m_bXRefStream = TRUE;
   return TRUE;
diff --git a/fpdfsdk/src/fpdfview_embeddertest.cpp b/fpdfsdk/src/fpdfview_embeddertest.cpp
index 35da9b6caf..3be96e7aef 100644
--- a/fpdfsdk/src/fpdfview_embeddertest.cpp
+++ b/fpdfsdk/src/fpdfview_embeddertest.cpp
@@ -199,3 +199,8 @@ TEST_F(FPDFViewEmbeddertest, Crasher_454695) {
   // Document is damanged and can't be opened.
   EXPECT_FALSE(OpenDocument("bug_454695.pdf"));
 }
+
+// The following tests pass if the document opens without infinite looping.
+TEST_F(FPDFViewEmbeddertest, Hang_298) {
+  EXPECT_FALSE(OpenDocument("bug_298.pdf"));
+}
diff --git a/testing/resources/bug_298.in b/testing/resources/bug_298.in
new file mode 100644
index 0000000000..b066778a49
--- /dev/null
+++ b/testing/resources/bug_298.in
@@ -0,0 +1,21 @@
+{{header}}
+{{object 1 0}} <<
+>>
+endobj
+{{object 2 0}} <<
+  /Type /Catalog
+  /Length 0
+  /Filter ASCIIHexDecode
+  /Prev 35
+  /W [1 2 3]
+>>
+stream
+endstream
+endobj
+trailer <<
+  /Size 6
+  /Root 1 0 R
+>>
+startxref
+35
+%%EOF
diff --git a/testing/resources/bug_298.pdf b/testing/resources/bug_298.pdf
new file mode 100644
index 0000000000..5b195597fd
--- /dev/null
+++ b/testing/resources/bug_298.pdf
@@ -0,0 +1,22 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj <<
+>>
+endobj
+2 0 obj <<
+  /Type /Catalog
+  /Length 0
+  /Filter ASCIIHexDecode
+  /Prev 35
+  /W [1 2 3]
+>>
+stream
+endstream
+endobj
+trailer <<
+  /Size 6
+  /Root 1 0 R
+>>
+startxref
+35
+%%EOF