Fix free entry handling for cross reference v4 reading.

While reading free entries in v4 cross reference table, changing the
start_objnum caused crossref table built wrong.

BUG=569795
R=jun_fang@foxitsoftware.com, thestig@chromium.org

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

3 files changed, 180 insertions, 46 deletions

diff --git a/core/include/fpdfapi/fpdf_parser.h b/core/include/fpdfapi/fpdf_parser.h
index a266b20ba2..de0aa6196c 100644
--- a/core/include/fpdfapi/fpdf_parser.h
+++ b/core/include/fpdfapi/fpdf_parser.h
@@ -476,10 +476,7 @@ class CPDF_Parser {
 
   FX_BOOL LoadAllCrossRefV5(FX_FILESIZE pos);
 
-  bool LoadCrossRefV4(FX_FILESIZE pos,
-                      FX_FILESIZE streampos,
-                      FX_BOOL bSkip,
-                      FX_BOOL bFirst);
+  bool LoadCrossRefV4(FX_FILESIZE pos, FX_FILESIZE streampos, FX_BOOL bSkip);
 
   FX_BOOL LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef);
 
diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
index f18b733ab7..bdbb939148 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
@@ -344,7 +344,7 @@ FX_FILESIZE CPDF_Parser::GetObjectOffset(FX_DWORD objnum) {
 }
 
 FX_BOOL CPDF_Parser::LoadAllCrossRefV4(FX_FILESIZE xrefpos) {
-  if (!LoadCrossRefV4(xrefpos, 0, TRUE, FALSE)) {
+  if (!LoadCrossRefV4(xrefpos, 0, TRUE)) {
     return FALSE;
   }
   m_pTrailer = LoadTrailerV4();
@@ -370,7 +370,7 @@ FX_BOOL CPDF_Parser::LoadAllCrossRefV4(FX_FILESIZE xrefpos) {
   xrefpos = newxrefpos;
   while (xrefpos) {
     CrossRefList.InsertAt(0, xrefpos);
-    LoadCrossRefV4(xrefpos, 0, TRUE, FALSE);
+    LoadCrossRefV4(xrefpos, 0, TRUE);
     nonstd::unique_ptr<CPDF_Dictionary, ReleaseDeleter<CPDF_Dictionary>> pDict(
         LoadTrailerV4());
     if (!pDict)
@@ -388,7 +388,7 @@ FX_BOOL CPDF_Parser::LoadAllCrossRefV4(FX_FILESIZE xrefpos) {
     m_Trailers.Add(pDict.release());
   }
   for (int32_t i = 0; i < CrossRefList.GetSize(); i++) {
-    if (!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE, i == 0))
+    if (!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE))
       return FALSE;
   }
   return TRUE;
@@ -412,7 +412,7 @@ FX_BOOL CPDF_Parser::LoadLinearizedAllCrossRefV4(FX_FILESIZE xrefpos,
   xrefpos = GetDirectInteger(m_pTrailer, "Prev");
   while (xrefpos) {
     CrossRefList.InsertAt(0, xrefpos);
-    LoadCrossRefV4(xrefpos, 0, TRUE, FALSE);
+    LoadCrossRefV4(xrefpos, 0, TRUE);
     CPDF_Dictionary* pDict = LoadTrailerV4();
     if (!pDict) {
       return FALSE;
@@ -422,7 +422,7 @@ FX_BOOL CPDF_Parser::LoadLinearizedAllCrossRefV4(FX_FILESIZE xrefpos,
     m_Trailers.Add(pDict);
   }
   for (int32_t i = 1; i < CrossRefList.GetSize(); i++)
-    if (!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE, i == 0)) {
+    if (!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE)) {
       return FALSE;
     }
   return TRUE;
@@ -497,8 +497,7 @@ bool CPDF_Parser::FindPosInOffsets(FX_FILESIZE pos) const {
 
 bool CPDF_Parser::LoadCrossRefV4(FX_FILESIZE pos,
                                  FX_FILESIZE streampos,
-                                 FX_BOOL bSkip,
-                                 FX_BOOL bFirst) {
+                                 FX_BOOL bSkip) {
   m_Syntax.RestorePos(pos);
   if (m_Syntax.GetKeyword() != "xref")
     return false;
@@ -527,17 +526,13 @@ bool CPDF_Parser::LoadCrossRefV4(FX_FILESIZE pos,
     FX_DWORD count = m_Syntax.GetDirectNum();
     m_Syntax.ToNextWord();
     SavedPos = m_Syntax.SavePos();
-    FX_BOOL bFirstItem = FALSE;
     const int32_t recordsize = 20;
-    if (bFirst)
-      bFirstItem = TRUE;
     m_dwXrefStartObjNum = start_objnum;
     if (!bSkip) {
       std::vector<char> buf(1024 * recordsize + 1);
       char* pBuf = pdfium::vector_as_array(&buf);
       pBuf[1024 * recordsize] = '\0';
       int32_t nBlocks = count / 1024 + 1;
-      FX_BOOL bFirstBlock = TRUE;
       for (int32_t block = 0; block < nBlocks; block++) {
         int32_t block_size = block == nBlocks - 1 ? count % 1024 : 1024;
         m_Syntax.ReadBlock(reinterpret_cast<uint8_t*>(pBuf),
@@ -546,18 +541,6 @@ bool CPDF_Parser::LoadCrossRefV4(FX_FILESIZE pos,
           FX_DWORD objnum = start_objnum + block * 1024 + i;
           char* pEntry = pBuf + i * recordsize;
           if (pEntry[17] == 'f') {
-            if (bFirstItem) {
-              objnum = 0;
-              bFirstItem = FALSE;
-            }
-            if (bFirstBlock) {
-              FX_FILESIZE offset = (FX_FILESIZE)FXSYS_atoi64(pEntry);
-              int32_t version = FXSYS_atoi(pEntry + 11);
-              if (offset == 0 && version == 65535 && start_objnum != 0) {
-                start_objnum--;
-                objnum = 0;
-              }
-            }
             m_CrossRef.SetAtGrow(objnum, 0);
             m_V5Type.SetAtGrow(objnum, 0);
           } else {
@@ -580,9 +563,6 @@ bool CPDF_Parser::LoadCrossRefV4(FX_FILESIZE pos,
             }
             m_V5Type.SetAtGrow(objnum, 1);
           }
-          if (bFirstBlock) {
-            bFirstBlock = FALSE;
-          }
         }
       }
     }
@@ -1574,7 +1554,7 @@ FX_DWORD CPDF_Parser::StartAsynParse(IFX_FileRead* pFileAccess,
   FX_FILESIZE dwFirstXRefOffset = m_Syntax.SavePos();
   FX_BOOL bXRefRebuilt = FALSE;
   FX_BOOL bLoadV4 = FALSE;
-  if (!(bLoadV4 = LoadCrossRefV4(dwFirstXRefOffset, 0, FALSE, FALSE)) &&
+  if (!(bLoadV4 = LoadCrossRefV4(dwFirstXRefOffset, 0, FALSE)) &&
       !LoadCrossRefV5(&dwFirstXRefOffset, TRUE)) {
     if (!RebuildCrossRef()) {
       return PDFPARSE_ERROR_FORMAT;
diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp
index 640feac9aa..aba639c850 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser_unittest.cpp
@@ -7,12 +7,65 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/utils/path_service.h"
 
+// Functions to help test an array's content against expected results.
+template <class TYPE>
+bool CompareArray(const CFX_ArrayTemplate<TYPE>& array1,
+                  const TYPE* array2,
+                  size_t size) {
+  if (array1.GetSize() != size)
+    return false;
+
+  for (int i = 0; i < size; ++i)
+    if (array1.GetAt(i) != array2[i])
+      return false;
+  return true;
+}
+template bool CompareArray<uint8_t>(const CFX_ByteArray&,
+                                    const uint8_t*,
+                                    size_t);
+template bool CompareArray<FX_WORD>(const CFX_WordArray&,
+                                    const FX_WORD*,
+                                    size_t);
+template bool CompareArray<FX_DWORD>(const CFX_DWordArray&,
+                                     const FX_DWORD*,
+                                     size_t);
+template bool CompareArray<FX_FILESIZE>(const CFX_FileSizeArray&,
+                                        const FX_FILESIZE*,
+                                        size_t);
+
+// Provide a way to read test data from a buffer instead of a file.
+class CFX_TestBufferRead : public IFX_FileRead {
+ public:
+  CFX_TestBufferRead(const unsigned char* buffer_in, size_t buf_size)
+      : buffer_(buffer_in), total_size_(buf_size) {}
+
+  // IFX_Stream
+  void Release() override { delete this; }
+
+  // IFX_FileRead
+  virtual FX_BOOL ReadBlock(void* buffer, FX_FILESIZE offset, size_t size) {
+    if (offset < 0 || offset + size > total_size_) {
+      return FALSE;
+    }
+
+    memcpy(buffer, buffer_ + offset, size);
+    return TRUE;
+  }
+  virtual FX_FILESIZE GetSize() { return (FX_FILESIZE)total_size_; };
+
+ protected:
+  const unsigned char* buffer_;
+  size_t total_size_;
+};
+
+// A wrapper class to help test member functions of CPDF_Parser.
 class CPDF_TestParser : public CPDF_Parser {
  public:
   CPDF_TestParser() {}
   ~CPDF_TestParser() {}
 
-  bool InitTest(const FX_CHAR* path) {
+  // Setup reading from a file and initial states.
+  bool InitTestFromFile(const FX_CHAR* path) {
     IFX_FileRead* pFileAccess = FX_CreateFileRead(path);
     if (!pFileAccess)
       return false;
@@ -22,11 +75,23 @@ class CPDF_TestParser : public CPDF_Parser {
     return true;
   }
 
+  // Setup reading from a buffer and initial states.
+  bool InitTestFromBuffer(const unsigned char* buffer, size_t len) {
+    CFX_TestBufferRead* buffer_reader = new CFX_TestBufferRead(buffer, len);
+
+    // For the test file, the header is set at the beginning.
+    m_Syntax.InitParser(buffer_reader, 0);
+    return true;
+  }
+
  private:
-  // Add test case as private friend so that RebuildCrossRef in CPDF_Parser
-  // can be accessed.
+  // Add test cases here as private friend so that protected members in
+  // CPDF_Parser can be accessed by test cases.
+  // Need to access RebuildCrossRef.
   FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefCorrectly);
   FRIEND_TEST(fpdf_parser_parser, RebuildCrossRefFailed);
+  // Need to access LoadCrossRefV4.
+  FRIEND_TEST(fpdf_parser_parser, LoadCrossRefV4);
 };
 
 // TODO(thestig) Using unique_ptr with ReleaseDeleter is still not ideal.
@@ -197,20 +262,14 @@ TEST(fpdf_parser_parser, RebuildCrossRefCorrectly) {
   std::string test_file;
   ASSERT_TRUE(PathService::GetTestFilePath("parser_rebuildxref_correct.pdf",
                                            &test_file));
-  ASSERT_TRUE(parser.InitTest(test_file.c_str())) << test_file;
+  ASSERT_TRUE(parser.InitTestFromFile(test_file.c_str())) << test_file;
 
   ASSERT_TRUE(parser.RebuildCrossRef());
   const FX_FILESIZE offsets[] = {0, 15, 61, 154, 296, 374, 450};
   const FX_WORD versions[] = {0, 0, 2, 4, 6, 8, 0};
-  static_assert(FX_ArraySize(offsets) == FX_ArraySize(versions),
-                "numbers of offsets and versions should be same.");
-  ASSERT_EQ(FX_ArraySize(offsets), parser.m_CrossRef.GetSize());
-  ASSERT_EQ(FX_ArraySize(versions), parser.m_ObjVersion.GetSize());
-
-  for (int i = 0; i < FX_ArraySize(offsets); ++i) {
-    EXPECT_EQ(offsets[i], parser.m_CrossRef.GetAt(i));
-    EXPECT_EQ(versions[i], parser.m_ObjVersion.GetAt(i));
-  }
+  ASSERT_TRUE(CompareArray(parser.m_CrossRef, offsets, FX_ArraySize(offsets)));
+  ASSERT_TRUE(
+      CompareArray(parser.m_ObjVersion, versions, FX_ArraySize(versions)));
 }
 
 TEST(fpdf_parser_parser, RebuildCrossRefFailed) {
@@ -218,7 +277,105 @@ TEST(fpdf_parser_parser, RebuildCrossRefFailed) {
   std::string test_file;
   ASSERT_TRUE(PathService::GetTestFilePath(
       "parser_rebuildxref_error_notrailer.pdf", &test_file));
-  ASSERT_TRUE(parser.InitTest(test_file.c_str())) << test_file;
+  ASSERT_TRUE(parser.InitTestFromFile(test_file.c_str())) << test_file;
 
   ASSERT_FALSE(parser.RebuildCrossRef());
-}
\ No newline at end of file
+}
+
+TEST(fpdf_parser_parser, LoadCrossRefV4) {
+  {
+    const unsigned char xref_table[] =
+        "xref \n"
+        "0 6 \n"
+        "0000000003 65535 f \n"
+        "0000000017 00000 n \n"
+        "0000000081 00000 n \n"
+        "0000000000 00007 f \n"
+        "0000000331 00000 n \n"
+        "0000000409 00000 n \n"
+        "trail";  // Needed to end cross ref table reading.
+    CPDF_TestParser parser;
+    ASSERT_TRUE(
+        parser.InitTestFromBuffer(xref_table, FX_ArraySize(xref_table)));
+
+    ASSERT_TRUE(parser.LoadCrossRefV4(0, 0, FALSE));
+    const FX_FILESIZE offsets[] = {0, 17, 81, 0, 331, 409};
+    const uint8_t types[] = {0, 1, 1, 0, 1, 1};
+    ASSERT_TRUE(
+        CompareArray(parser.m_CrossRef, offsets, FX_ArraySize(offsets)));
+    ASSERT_TRUE(CompareArray(parser.m_V5Type, types, FX_ArraySize(types)));
+  }
+  {
+    const unsigned char xref_table[] =
+        "xref \n"
+        "0 1 \n"
+        "0000000000 65535 f \n"
+        "3 1 \n"
+        "0000025325 00000 n \n"
+        "8 2 \n"
+        "0000025518 00002 n \n"
+        "0000025635 00000 n \n"
+        "12 1 \n"
+        "0000025777 00000 n \n"
+        "trail";  // Needed to end cross ref table reading.
+    CPDF_TestParser parser;
+    ASSERT_TRUE(
+        parser.InitTestFromBuffer(xref_table, FX_ArraySize(xref_table)));
+
+    ASSERT_TRUE(parser.LoadCrossRefV4(0, 0, FALSE));
+    const FX_FILESIZE offsets[] = {0, 0,     0,     25325, 0, 0,    0,
+                                   0, 25518, 25635, 0,     0, 25777};
+    const uint8_t types[] = {0, 0, 0, 1, 0, 0, 0, 0, 1, 1, 0, 0, 1};
+    ASSERT_TRUE(
+        CompareArray(parser.m_CrossRef, offsets, FX_ArraySize(offsets)));
+    ASSERT_TRUE(CompareArray(parser.m_V5Type, types, FX_ArraySize(types)));
+  }
+  {
+    const unsigned char xref_table[] =
+        "xref \n"
+        "0 1 \n"
+        "0000000000 65535 f \n"
+        "3 1 \n"
+        "0000025325 00000 n \n"
+        "8 2 \n"
+        "0000000000 65535 f \n"
+        "0000025635 00000 n \n"
+        "12 1 \n"
+        "0000025777 00000 n \n"
+        "trail";  // Needed to end cross ref table reading.
+    CPDF_TestParser parser;
+    ASSERT_TRUE(
+        parser.InitTestFromBuffer(xref_table, FX_ArraySize(xref_table)));
+
+    ASSERT_TRUE(parser.LoadCrossRefV4(0, 0, FALSE));
+    const FX_FILESIZE offsets[] = {0, 0, 0,     25325, 0, 0,    0,
+                                   0, 0, 25635, 0,     0, 25777};
+    const uint8_t types[] = {0, 0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 0, 1};
+    ASSERT_TRUE(
+        CompareArray(parser.m_CrossRef, offsets, FX_ArraySize(offsets)));
+    ASSERT_TRUE(CompareArray(parser.m_V5Type, types, FX_ArraySize(types)));
+  }
+  {
+    const unsigned char xref_table[] =
+        "xref \n"
+        "0 7 \n"
+        "0000000002 65535 f \n"
+        "0000000023 00000 n \n"
+        "0000000003 65535 f \n"
+        "0000000004 65535 f \n"
+        "0000000000 65535 f \n"
+        "0000000045 00000 n \n"
+        "0000000179 00000 n \n"
+        "trail";  // Needed to end cross ref table reading.
+    CPDF_TestParser parser;
+    ASSERT_TRUE(
+        parser.InitTestFromBuffer(xref_table, FX_ArraySize(xref_table)));
+
+    ASSERT_TRUE(parser.LoadCrossRefV4(0, 0, FALSE));
+    const FX_FILESIZE offsets[] = {0, 23, 0, 0, 0, 45, 179};
+    const uint8_t types[] = {0, 1, 0, 0, 0, 1, 1};
+    ASSERT_TRUE(
+        CompareArray(parser.m_CrossRef, offsets, FX_ArraySize(offsets)));
+    ASSERT_TRUE(CompareArray(parser.m_V5Type, types, FX_ArraySize(types)));
+  }
+}