Fix the potential integer overflow from "offset + size"

core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp

 // Copyright 2014 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 "../../../include/fpdfapi/fpdf_parser.h"
 #include "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fpdfapi/fpdf_page.h"
 #include "../../../../third_party/numerics/safe_math.h"
@@ skipping 2847 matching lines @@
                             new_obj_array.Add(value);
                         }
                     }
                 }
                 break;
             case PDFOBJ_REFERENCE: {
                     CPDF_Reference *pRef = (CPDF_Reference*)pObj;
                     FX_DWORD dwNum = pRef->GetRefObjNum();
                     FX_FILESIZE offset;
                     FX_DWORD size = GetObjectSize(pRef->GetRefObjNum(), offset);
-                    if (!size) {
+
+                    if (size == 0 || offset < 0 || offset >= m_dwFileLen)
                         break;
-                    }
-                    size = (FX_DWORD)((FX_FILESIZE)(offset + size + 512) > m_dwFileLen ? m_dwFileLen - offset : size + 512);
+                    
+                    base::CheckedNumeric<FX_DWORD> safe_size = size;

[palmer, 2014/07/09 18:29:14]

You should check the value that you actually use, i.e. |size|. You don't need
|safe_size|.

base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(),
offset);
if (size.ValueOrDefault(0) == 0 || offset < 0 || offset >= m_dwFileLen) {
    break;
}

safe_size += offset;
safe_size += 512;
if (!safe_size.IsValid()) {
    break;
}

if (safe_size.ValueOrDie() > m_dwFileLen) {
    size = m_dwFileLen - offset; 
} else {
    size = size + 512;
}
if (!safe_size.IsValid()) {
    break;
}

Then use size.ValidOrDie() on lines 2883 and 2884.

[jun_fang, 2014/07/10 22:21:54]

safe_size is a temporary variable which is used to judge whether the result of
|size+offset+512| overflows. Even we use the method you suggested:
base::CheckedNumeric<FX_DWORD> size = GetObjectSize(pRef->GetRefObjNum(). We
still need another temporary variable to check the sum of integer. 

+                    safe_size += offset;
+                    safe_size += 512;
+                    if (!safe_size.IsValid())
+                        break;
+
+                    if (safe_size.ValueOrDie() > m_dwFileLen)
+                        size = m_dwFileLen - offset; 

[jun_fang, 2014/07/10 22:21:54]

size = m_dwFileLen - offset; |size| should be valid because we check |offset <
m_dwFileLen| at the line of 2869.

+                    else
+                        size = size + 512;

[jun_fang, 2014/07/10 22:21:54]

because safe_size is valid, we can make sure size is valid here.
safe_size = size + offset + 512;

+
                     if (!m_pFileAvail->IsDataAvail(offset, size)) {
                         pHints->AddSegment(offset, size);
                         ret_array.Add(pObj);
                         count++;
                     } else if (!m_objnum_array.Find(dwNum)) {
                         m_objnum_array.AddObjNum(dwNum);
                         CPDF_Object *pReferred = m_pDocument->GetIndirectObject(pRef->GetRefObjNum(), NULL);
                         if (pReferred) {
                             new_obj_array.Add(pReferred);
                         }
@@ skipping 181 matching lines @@
     CPDF_Object *pRet = NULL;
     if (pExistInFile) {
         *pExistInFile = TRUE;
     }
     if (m_pDocument == NULL) {
         FX_FILESIZE offset = m_parser.GetObjectOffset(objnum);
         if (offset < 0) {
             *pExistInFile = FALSE;
             return NULL;
         }
+
         FX_DWORD size = (FX_DWORD)m_parser.GetObjectSize(objnum);
-        size = (FX_DWORD)(((FX_FILESIZE)(offset + size + 512)) > m_dwFileLen ? m_dwFileLen - offset : size + 512);
+        if (size == 0 || offset < 0 || offset >= m_dwFileLen)
+            return NULL;
+  
+        base::CheckedNumeric<FX_DWORD> safe_size = size;

[palmer, 2014/07/09 18:29:14]

Same as above.

+        safe_size += offset;
+        safe_size += 512;
+        if (!safe_size.IsValid()) 
+            return NULL;
+
+        if (safe_size.ValueOrDie() > m_dwFileLen)
+            size = m_dwFileLen - offset;
+        else
+            size = size + 512;
+
         if (!m_pFileAvail->IsDataAvail(offset, size)) {
             pHints->AddSegment(offset, size);
             return NULL;
         }
         pRet = m_parser.ParseIndirectObject(NULL, objnum);
         if (!pRet && pExistInFile) {
             *pExistInFile = FALSE;
         }
         return pRet;
     }
     FX_FILESIZE offset = 0;
     FX_DWORD size = GetObjectSize(objnum, offset);
-    size = (FX_DWORD)((FX_FILESIZE)(offset + size + 512) > m_dwFileLen ? m_dwFileLen - offset : size + 512);
+    if (size == 0 || offset < 0 || offset >= m_dwFileLen)
+        return NULL;
+
+    base::CheckedNumeric<FX_DWORD> safe_size = size;

[palmer, 2014/07/09 18:29:14]

Same as above.

+    safe_size += offset;
+    safe_size += 512;
+    if (!safe_size.IsValid()) 
+        return NULL;
+
+    if (safe_size.ValueOrDie() > m_dwFileLen)
+        size = m_dwFileLen - offset;
+    else
+        size = size + 512;
+ 
     if (!m_pFileAvail->IsDataAvail(offset, size)) {
         pHints->AddSegment(offset, size);
         return NULL;
     }
     CPDF_Parser *pParser = (CPDF_Parser *)(m_pDocument->GetParser());
     pRet = pParser->ParseIndirectObject(NULL, objnum, NULL);
     if (!pRet && pExistInFile) {
         *pExistInFile = FALSE;
     }
     return pRet;
@@ skipping 1286 matching lines @@
 {
     FX_INT32 iSize = m_childNode.GetSize();
     for (FX_INT32 i = 0; i < iSize; ++i) {
         CPDF_PageNode *pNode = (CPDF_PageNode*)m_childNode[i];
         if (pNode) {
             delete pNode;
         }
     }
     m_childNode.RemoveAll();
 }