Add basic checking for RebuildCrossRef

core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp

Index: core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
diff --git a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
index 2105635687edd0f3e6fe3f44ee6dff568e5b2508..a71adc8b48649d943a2a68dd0270f6eff3c4351a 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
@@ -247,12 +247,9 @@ FX_DWORD CPDF_Parser::StartParse(IFX_FileRead* pFileAccess,
   }
   FXSYS_qsort(m_SortedOffset.GetData(), m_SortedOffset.GetSize(),
               sizeof(FX_FILESIZE), CompareFileSize);
-  FX_DWORD RootObjNum = GetRootObjNum();
-  if (RootObjNum == 0) {
+  if (GetRootObjNum() == 0) {
     ReleaseEncryptHandler();
-    RebuildCrossRef();
-    RootObjNum = GetRootObjNum();
-    if (RootObjNum == 0)
+    if (!RebuildCrossRef())

[jun_fang, 2015/12/01 15:25:18]

please check the comment at line 1610.

[Wei Li, 2015/12/01 18:45:03]

Acknowledged.

       return PDFPARSE_ERROR_FORMAT;
 
     dwRet = SetEncryptHandler();
@@ -975,7 +972,7 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() {
     m_SortedOffset.Add(offset);
   }
   FX_Free(buffer);
-  return TRUE;
+  return m_pTrailer && GetRootObjNum() > 0 && m_CrossRef.GetSize() > 0;

[jun_fang, 2015/12/01 15:20:58]

The newly added condition ' GetRootObjNum() > 0' will change the existing
behavior. Please restore back to "m_pTrailer && m_CrossRef.GetSize() > 0".
Thanks!

[Wei Li, 2015/12/01 18:45:03]

I reverted to my original change. I think more thorough format checking may help
discover problems early, but at this moment I would opt for not changing current
caller behavior. :)

 }
 
 FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE pos,
@@ -1608,14 +1605,11 @@ FX_DWORD CPDF_Parser::StartAsynParse(IFX_FileRead* pFileAccess,
   }
   FXSYS_qsort(m_SortedOffset.GetData(), m_SortedOffset.GetSize(),
               sizeof(FX_FILESIZE), CompareFileSize);
-  FX_DWORD RootObjNum = GetRootObjNum();
-  if (RootObjNum == 0) {

[jun_fang, 2015/12/01 14:56:20]

It's a trigger to rebuild cross reference table when RootObjNum is 0.
After the cross reference is rebuilt, only checking RootObjNum is enough to
verify whether RebuildCrossRef succeeded. If RootObjNum is larger than 0,
m_pTrailer should not be nullptr. Please check the details in
CPDF_Parser::GetRootObjNum(). Also, we have at least one
object, the root object, in m_CrossRef.  


FX_DWORD CPDF_Parser::GetRootObjNum() {
  CPDF_Reference* pRef = ToReference(
      m_pTrailer ? m_pTrailer->GetElement(FX_BSTRC("Root")) : nullptr);
  return pRef ? pRef->GetRefObjNum() : 0;
}

[Wei Li, 2015/12/01 18:45:03]

I saw this. But RebuildCrossRef function is used at eight different places.
Checking the results should not rely on some implicit conditions of other
functions although it was ok to not checking on these two specific places. BTW,
file trailer's root element has an obj number doesn't mean m_CrossRef has that
object (for some malformed pdf). So pls notice that we are still checking a bit
more than before.  

+  if (GetRootObjNum() == 0) {
     ReleaseEncryptHandler();
-    RebuildCrossRef();
-    RootObjNum = GetRootObjNum();
-    if (RootObjNum == 0) {
+    if (!RebuildCrossRef())

[jun_fang, 2015/12/01 15:20:58]

Based on the analysis, I prefer to keep left code unchanged. It's better to add
a comment to explain why it's no need to check the return of RebuildCrossRef().

[Wei Li, 2015/12/01 18:45:03]

I would prefer keeping the check here as the comments above stated. thanks.

       return PDFPARSE_ERROR_FORMAT;
-    }
+
     dwRet = SetEncryptHandler();
     if (dwRet != PDFPARSE_ERROR_SUCCESS) {
       return dwRet;