Fix the potential integer overflow from "offset + size"

core/src/fxcrt/extension.h

Index: core/src/fxcrt/extension.h
diff --git a/core/src/fxcrt/extension.h b/core/src/fxcrt/extension.h
index a736425d57336862a1f5a9617561a53825591e91..8931eaa600cff53c25198c0a5b9acc65015a7536 100644
--- a/core/src/fxcrt/extension.h
+++ b/core/src/fxcrt/extension.h
@@ -6,6 +6,9 @@
 
 #ifndef _FXCRT_EXTENSION_IMP_
 #define _FXCRT_EXTENSION_IMP_
+
+#include "../../../third_party/numerics/safe_math.h"
+
 class IFXCRT_FileAccess
 {
 public:
@@ -181,9 +184,13 @@ public:
     }
     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;
@@ -198,13 +205,24 @@ public:
         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;
@@ -250,7 +268,12 @@ public:
             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) {
@@ -270,10 +293,16 @@ public:
             }
             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) {