Add a simple check for V4 cross reference table

core/fpdfapi/fpdf_parser/cpdf_parser.cpp

 // Copyright 2016 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "core/fpdfapi/fpdf_parser/include/cpdf_parser.h"
 
 #include <vector>
 
@@ skipping 298 matching lines @@
   if (GetObjectType(objnum) == 1)
     return GetObjectPositionOrZero(objnum);
 
   if (GetObjectType(objnum) == 2) {
     FX_FILESIZE pos = GetObjectPositionOrZero(objnum);
     return GetObjectPositionOrZero(pos);
   }
   return 0;
 }
 
+// Ideally, all the cross reference entries should be verified.
+// In reality, we rarely see well-formed cross references don't match
+// with the objects. crbug/602650 showed a case where object numbers
+// in the cross reference table are all off by one.
+bool CPDF_Parser::VerifyCrossRefV4() {
+  for (const auto& it : m_ObjectInfo) {

[dsinclair, 2016/04/21 19:47:09]

Is this going to cause a perf regression if you have a large xref table?

[Wei Li, 2016/04/21 20:56:44]

No since this only runs for one entry, not the whole table.

+    // Find the first non-zero position.
+    if (it.second.pos != 0) {

[dsinclair, 2016/04/21 19:47:09]

nit: if (it.second_pos == 0)
       continue;

[Wei Li, 2016/04/21 20:56:44]

Done.

+      FX_FILESIZE SavedPos = m_pSyntax->SavePos();
+      m_pSyntax->RestorePos(it.second.pos);
+      bool is_num = false;
+      CFX_ByteString num_str = m_pSyntax->GetNextWord(&is_num);
+      m_pSyntax->RestorePos(SavedPos);
+      if (!is_num || num_str.IsEmpty() ||
+          FXSYS_atoui(num_str.c_str()) != it.first) {
+        // If the object number read doesn't match the one stored,
+        // something wrong with the cross reference table.

[dsinclair, 2016/04/21 19:47:09]

nit: something is wrong

[Wei Li, 2016/04/21 20:56:44]

Done.

+        return false;
+      }
+    }
+  }
+  return true;
+}
+
 FX_BOOL CPDF_Parser::LoadAllCrossRefV4(FX_FILESIZE xrefpos) {
   if (!LoadCrossRefV4(xrefpos, 0, TRUE))
     return FALSE;
 
   m_pTrailer = LoadTrailerV4();
   if (!m_pTrailer)
     return FALSE;
 
   int32_t xrefsize = GetDirectInteger(m_pTrailer, "Size");
   if (xrefsize > 0 && xrefsize <= kMaxXRefSize)
@@ skipping 30 matching lines @@
 
     // SLOW ...
     XRefStreamList.insert(XRefStreamList.begin(),
                           pDict->GetIntegerBy("XRefStm"));
     m_Trailers.Add(pDict.release());
   }
 
   for (size_t i = 0; i < CrossRefList.size(); ++i) {
     if (!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE))
       return FALSE;
+    if (i == 0 && !VerifyCrossRefV4())

[dsinclair, 2016/04/21 19:47:09]

Why only verify the first one and not all of them? Is the assumption that if the
first is correct they'll all be correct?

[Wei Li, 2016/04/21 20:56:44]

This is related to your perf regression question. We want to avoid penalty for
well-behaved pdfs.

[dsinclair, 2016/04/21 21:02:54]

Right, I guess my question is, is there the potential that we're going to get an
issue where the first xref table is correct but subsequent ones are off-by-one?
So, if the bad writer extends a file and adds a new xref table, will we end up
crashing in that case?

+      return FALSE;
   }
   return TRUE;
 }
 
 FX_BOOL CPDF_Parser::LoadLinearizedAllCrossRefV4(FX_FILESIZE xrefpos,
                                                  uint32_t dwObjCount) {
   if (!LoadLinearizedCrossRefV4(xrefpos, dwObjCount))
     return FALSE;
 
   m_pTrailer = LoadTrailerV4();
@@ skipping 1263 matching lines @@
   if (!LoadLinearizedAllCrossRefV4(m_LastXRefOffset, m_dwXrefStartObjNum) &&
       !LoadLinearizedAllCrossRefV5(m_LastXRefOffset)) {
     m_LastXRefOffset = 0;
     m_pSyntax->m_MetadataObjnum = dwSaveMetadataObjnum;
     return FORMAT_ERROR;
   }
 
   m_pSyntax->m_MetadataObjnum = dwSaveMetadataObjnum;
   return SUCCESS;
 }