Fix the potential integer overflow from "offset + size"

core/src/fxcrt/extension.h

 // 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
 
 #ifndef _FXCRT_EXTENSION_IMP_
 #define _FXCRT_EXTENSION_IMP_
+
+#include "../../../third_party/numerics/safe_math.h"
+
 class IFXCRT_FileAccess
 {
 public:
     virtual ~IFXCRT_FileAccess() {}
     virtual FX_BOOL             Open(FX_BSTR fileName, FX_DWORD dwMode) = 0;
     virtual FX_BOOL             Open(FX_WSTR fileName, FX_DWORD dwMode) = 0;
     virtual void                Close() = 0;
     virtual void                Release() = 0;
     virtual FX_FILESIZE GetSize() const = 0;
     virtual FX_FILESIZE GetPosition() const = 0;
@@ skipping 155 matching lines @@
     virtual FX_FILESIZE                 GetPosition()
     {
         FX_FILESIZE pos = (FX_FILESIZE)m_nCurPos;
         if (m_bUseRange) {
             pos -= (FX_FILESIZE)m_nOffset;
         }
         return pos;
     }
     virtual FX_BOOL                             SetRange(FX_FILESIZE offset, FX_FILESIZE size)
     {
-        if (offset < 0 || (size_t)(offset + size) > m_nCurSize) {
+        base::CheckedNumeric<FX_FILESIZE> range = size;
+        range += size;

[palmer, 2014/07/23 17:58:36]

BUG: The original code checked offset + size, but this code checks size + size.

[jun_fang, 2014/07/24 07:14:39]

This issue was reported in the original bug. 

+ 
+        if (!range.IsValid() || offset <= 0 || size <= 0 || range.ValueOrDie() > m_nCurSize) {
             return FALSE;
         }
+        
         m_nOffset = (size_t)offset, m_nSize = (size_t)size;
         m_bUseRange = TRUE;
         m_nCurPos = m_nOffset;
         return TRUE;
     }
     virtual void                                ClearRange()
     {
         m_bUseRange = FALSE;
     }
     virtual FX_BOOL                             ReadBlock(void* buffer, FX_FILESIZE offset, size_t size)
     {
         if (!buffer || !size) {
             return FALSE;
         }
+
+        base::CheckedNumeric<FX_FILESIZE> safeOffset = offset;
         if (m_bUseRange) {
-            offset += (FX_FILESIZE)m_nOffset;
+            safeOffset += m_nOffset;
         }
-        if ((size_t)offset + size > m_nCurSize) {
+         
+        if (!safeOffset.IsValid())
+            return FALSE;
+
+        offset = safeOffset.ValueOrDie();
+
+        base::CheckedNumeric<size_t> newPos = size;
+        newPos += offset;
+        if (newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() > m_nCurSize) {

[palmer, 2014/07/23 17:58:36]

ValueOrDie will cause a program abort, and you'll never get to return FALSE.
FWIW. If you want to make sure to return FALSE and not die on integer overflow,
try this:

base::CheckedNumeric<size_t> newPos = size;
newPos += offset;
if (!newPos.IsValid() || newPos.ValueOrDefault(0) == 0 || newPos.ValueOrDie() >
m_nCurSize) {
    return FALSE;
}

It looks a bit weird, but it's OK because !newPos.IsValid() is evaluated first —
newPos.ValueOrDie() will either not die, or not get called at all.

[jun_fang, 2014/07/24 07:14:39]

Will update.

[jschuh, 2014/07/24 13:48:46]

You can skip the !newPos.IsValid() check, because it's made redundant by the
newPos.ValueOrDefault(0) == 0 condition, which will short-circuit to have you
return FALSE on an invalid value.

 

             return FALSE;
         }
-        m_nCurPos = (size_t)offset + size;
+
+        m_nCurPos = newPos.ValueOrDie();
         if (m_dwFlags & FX_MEMSTREAM_Consecutive) {
             FXSYS_memcpy32(buffer, (FX_LPBYTE)m_Blocks[0] + (size_t)offset, size);
             return TRUE;
         }
         size_t nStartBlock = (size_t)offset / m_nGrowSize;
         offset -= (FX_FILESIZE)(nStartBlock * m_nGrowSize);
         while (size) {
             size_t nRead = m_nGrowSize - (size_t)offset;
             if (nRead > size) {
                 nRead = size;
@@ skipping 25 matching lines @@
     }
     virtual     FX_BOOL                         WriteBlock(const void* buffer, FX_FILESIZE offset, size_t size)
     {
         if (!buffer || !size) {
             return FALSE;
         }
         if (m_bUseRange) {
             offset += (FX_FILESIZE)m_nOffset;
         }
         if (m_dwFlags & FX_MEMSTREAM_Consecutive) {
-            m_nCurPos = (size_t)offset + size;
+            base::CheckedNumeric<size_t> newPos = size; 
+            newPos += offset;
+            if (!newPos.IsValid())
+                return FALSE;
+
+            m_nCurPos = newPos.ValueOrDie();
             if (m_nCurPos > m_nTotalSize) {
                 m_nTotalSize = (m_nCurPos + m_nGrowSize - 1) / m_nGrowSize * m_nGrowSize;
                 if (m_Blocks.GetSize() < 1) {
                     void* block = FX_Alloc(FX_BYTE, m_nTotalSize);
                     m_Blocks.Add(block);
                 } else {
                     m_Blocks[0] = FX_Realloc(FX_BYTE, m_Blocks[0], m_nTotalSize);
                 }
                 if (!m_Blocks[0]) {
                     m_Blocks.RemoveAll();
                     return FALSE;
                 }
             }
             FXSYS_memcpy32((FX_LPBYTE)m_Blocks[0] + (size_t)offset, buffer, size);
             if (m_nCurSize < m_nCurPos) {
                 m_nCurSize = m_nCurPos;
             }
             return TRUE;
         }
-        if (!ExpandBlocks((size_t)offset + size)) {
+
+        base::CheckedNumeric<size_t> newPos = size;
+        newPos += offset;
+        if (!newPos.IsValid())
+            return FALSE;
+
+        if (!ExpandBlocks(newPos.ValueOrDie())) {
             return FALSE;
         }
-        m_nCurPos = (size_t)offset + size;
+        m_nCurPos = newPos.ValueOrDie();
         size_t nStartBlock = (size_t)offset / m_nGrowSize;
         offset -= (FX_FILESIZE)(nStartBlock * m_nGrowSize);
         while (size) {
             size_t nWrite = m_nGrowSize - (size_t)offset;
             if (nWrite > size) {
                 nWrite = size;
             }
             FXSYS_memcpy32((FX_LPBYTE)m_Blocks[(int)nStartBlock] + (size_t)offset, buffer, nWrite);
             buffer = ((FX_LPBYTE)buffer) + nWrite;
             size -= nWrite;
@@ skipping 102 matching lines @@
     FX_DWORD mt[MT_N];
 } FX_MTRANDOMCONTEXT, * FX_LPMTRANDOMCONTEXT;
 typedef FX_MTRANDOMCONTEXT const * FX_LPCMTRANDOMCONTEXT;
 #if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_
 FX_BOOL FX_GenerateCryptoRandom(FX_LPDWORD pBuffer, FX_INT32 iCount);
 #endif
 #ifdef __cplusplus
 }
 #endif
 #endif