Fix the potential integer overflow from "offset + size"

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 c705ea73576de248cec87e003635782a35a5ae99..167af85a733ca73fde9ec7a4608f832b3c4264d4 100644
--- a/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
+++ b/core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp
@@ -2854,10 +2854,11 @@ FX_BOOL CPDF_DataAvail::IsObjectsAvail(CFX_PtrArray& obj_array, FX_BOOL bParsePa
                     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)

[palmer, 2014/06/12 00:32:05]

According to

./core/include/fxcrt/fx_system.h:110:typedef FX_DWORD                          
FX_UINT32;

|size| can never be < 0, so "<=" is "testing too much", if you see what I mean.

Style nit: Other code in this file uses a space after the "if": "if (...)".

[jun_fang, 2014/07/08 17:43:11]

You are right. I will change "<=" to "==" and change the style back based on the
your comments.

                         break;
-                    }
-                    size = (FX_DWORD)((FX_FILESIZE)(offset + size + 512) > m_dwFileLen ? m_dwFileLen - offset : size + 512);
+
+                    size = (FX_DWORD)(offset + size + 512 > m_dwFileLen ? m_dwFileLen - offset : size + 512);

[palmer, 2014/06/12 00:32:04]

The expression "offset + size + 512" can overflow, which might change the
outcome of this calculation in a way you don't intend. (I.e. you might get "size
+ 512" when you wanted "m_dwFileLen - offset".

The annoyance of writing 2 checks for the 2 additions, plus the check for size +
512, is a good argument for using our base/numerics library from Chromium.

I think the explicit cast to FX_DWORD might not be necessary? It's what happens
implicitly anyway.

Also, there is a logical problem with "FX_FILESIZE offset". On some platforms,
FX_FILESIZE is off_t, which is sometimes 64-bit. First, there is a problem with
the idea of having a file that might be very large (> 4 GiB) but having its size
be described with the FX_UINT32/FX_DWORD variable |size|. Second, in various
places in the code, you cast/truncated |m_dwFileLen| to a FX_DWORD anyway.

So, you should decide if you want to support large files, and if so,
consistently use a 64-bit value to describe the size of the file and the objects
in it (it might be possible to have individual objects in the file be > 4 GiB,
like an embedded video...?). If you don't want to support large files, it's
probably better to fail early when parsing them rather than truncating their
size to 4 GiB, which is effectively what is happening now.

Finally, note that if offset == m_dwFileLen, you can end up with size = 0. Which
you don't want (you check for it on line 2858).

                     if (!m_pFileAvail->IsDataAvail(offset, size)) {
                         pHints->AddSegment(offset, size);
                         ret_array.Add(pObj);
@@ -3059,8 +3060,13 @@ CPDF_Object* CPDF_DataAvail::GetObject(FX_DWORD objnum, IFX_DownloadHints* pHint
             *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;
+
+        size = (FX_DWORD)(offset + size + 512 > m_dwFileLen ? m_dwFileLen - offset : size + 512);
         if (!m_pFileAvail->IsDataAvail(offset, size)) {
             pHints->AddSegment(offset, size);
             return NULL;
@@ -3073,7 +3079,11 @@ CPDF_Object* CPDF_DataAvail::GetObject(FX_DWORD objnum, IFX_DownloadHints* pHint
     }
     FX_FILESIZE offset;
     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;
+ 
+    size = (FX_DWORD)(offset + size + 512 > m_dwFileLen ? m_dwFileLen - offset : size + 512);
     if (!m_pFileAvail->IsDataAvail(offset, size)) {
         pHints->AddSegment(offset, size);
         return NULL;