Fix potential integer overflow in fpdf_render_loadimage.cpp

core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.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/fxge/fx_ge.h"
 #include "../../../include/fxcodec/fx_codec.h"
 #include "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fpdfapi/fpdf_render.h"
 #include "../../../include/fpdfapi/fpdf_pageobj.h"
 #include "../fpdf_page/pageint.h"
 #include "render_int.h"
-#include <limits.h>
+#include "../../../../third_party/numerics/safe_math.h"
+
+typedef base::CheckedNumeric<FX_DWORD> FxDword;

[Tom Sepez, 2014/07/25 16:15:23]

nit: despite what palmer says about typedefs, these seem confusingly named to
me.  Sorry.  How about FX_SAFE_DWORD or FX_SAFE_INT or similar?

What do you think about moving these to the same place as those for FX_WORD?  It
would be a shame to have 17 different versions of FX_Something that all mean
base::CheckedNumeric<FX_DWORD> sprinkled throughout the code.

[jun_fang, 2014/07/30 22:27:01]

will update it in the next patch.

+typedef base::CheckedNumeric<int> Int;
+
 static unsigned int _GetBits8(FX_LPCBYTE pData, int bitpos, int nbits)
 {
     unsigned int byte = pData[bitpos / 8];
     if (nbits == 8) {
         return byte;
     } else if (nbits == 4) {
         return (bitpos % 8) ? (byte & 0x0f) : (byte >> 4);
     } else if (nbits == 2) {
         return (byte >> (6 - bitpos % 8)) & 0x03;
     } else if (nbits == 1) {
@@ skipping 143 matching lines @@
     m_Width = m_pDict->GetInteger(FX_BSTRC("Width"));
     m_Height = m_pDict->GetInteger(FX_BSTRC("Height"));
     if (m_Width <= 0 || m_Height <= 0 || m_Width > 0x01ffff || m_Height > 0x01ffff) {
         return FALSE;
     }
     m_GroupFamily = GroupFamily;
     m_bLoadMask = bLoadMask;
     if (!LoadColorInfo(m_pStream->GetObjNum() != 0 ? NULL : pFormResources, pPageResources)) {
         return FALSE;
     }
-    FX_DWORD src_pitch = m_bpc;
-    if (m_bpc != 0 && m_nComponents != 0) {
-        if (src_pitch > 0 && m_nComponents > (unsigned)INT_MAX / src_pitch) {
-            return FALSE;
-        }
-        src_pitch *= m_nComponents;
-        if (src_pitch > 0 && (FX_DWORD)m_Width > (unsigned)INT_MAX / src_pitch) {
-            return FALSE;
-        }
-        src_pitch *= m_Width;
-        if (src_pitch + 7 < src_pitch) {
-            return FALSE;
-        }
-        src_pitch += 7;
-        src_pitch /= 8;
-        if (src_pitch > 0 && (FX_DWORD)m_Height > (unsigned)INT_MAX / src_pitch) {
-            return FALSE;
-        }
+
+    if (m_bpc == 0 || m_nComponents == 0) {

[Tom Sepez, 2014/07/25 21:13:51]

We continue execution here in the old code in this case. Why do we want to
short-circuit the calls below? Is this the comparison from line 221 of the old
file moved up here?

[jun_fang, 2014/07/30 22:27:01]

You can find |FX_DWORD src_pitch = m_bpc;| at line 178 of original code. if
|m_bpc| is 0, it can't load any data at line 198 in the original code because
|m_Height * src_pitch| is 0.

bpc: bits per component
components are used in color space. For example, RGB color space has 3
components. 
They must be larger than 0 (>0). 

+        return FALSE;
     }
+
+    FxDword src_pitch = m_bpc;
+    src_pitch *= m_nComponents;
+    src_pitch *= m_Width;
+    src_pitch += 7;
+    src_pitch /= 8;
+    src_pitch *= m_Height;
+    if (!src_pitch.IsValid()) {
+        return FALSE;
+    }
+
     m_pStreamAcc = FX_NEW CPDF_StreamAcc;
-    m_pStreamAcc->LoadAllData(pStream, FALSE, m_Height * src_pitch, TRUE);
+    m_pStreamAcc->LoadAllData(pStream, FALSE, src_pitch.ValueOrDie(), TRUE);
     if (m_pStreamAcc->GetSize() == 0 || m_pStreamAcc->GetData() == NULL) {
         return FALSE;
     }
     const CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
     if (!decoder.IsEmpty() && decoder == FX_BSTRC("CCITTFaxDecode")) {
         m_bpc = 1;
     }
     if (!CreateDecoder()) {
         return FALSE;
     }
     if (m_bImageMask) {
         m_bpp = 1;
         m_bpc = 1;
         m_nComponents = 1;
         m_AlphaFlag = 1;
     } else if (m_bpc * m_nComponents == 1) {
         m_bpp = 1;
     } else if (m_bpc * m_nComponents <= 8) {
         m_bpp = 8;
     } else {
         m_bpp = 24;
     }
-    if (!m_bpc || !m_nComponents) {
-        return FALSE;
-    }
-    m_Pitch = m_Width;
-    if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
-        return FALSE;
-    }
-    m_Pitch *= m_bpp;
-    if (m_Pitch + 31 < m_Pitch) {
-        return FALSE;
-    }
-    m_Pitch += 31;
-    m_Pitch = m_Pitch / 32 * 4;
-    m_pLineBuf = FX_Alloc(FX_BYTE, m_Pitch);
+
+    FxDword pitch = m_Width;
+    pitch *= m_bpp;
+    pitch += 31;
+    pitch /= 8;
+    if (!pitch.IsValid()) return FALSE;

[Tom Sepez, 2014/07/25 21:13:51]

nit: braces, return on its own line, to match existing style.

[Tom Sepez, 2014/07/25 21:13:51]

We can return here without setting m_pitch as in the old code.  Does this
matter?

[jun_fang, 2014/07/30 22:27:01]

m_pitch will be updated if there is no problem during calculation.If there is an
error in calculating it, we don't have a choice but keep old value.It's
incorrect to assign width to pitch.

+  
+    m_pLineBuf = FX_Alloc(FX_BYTE, pitch.ValueOrDie());
     if (m_pColorSpace && bStdCS) {
         m_pColorSpace->EnableStdConversion(TRUE);
     }
     LoadPalette();
     if (m_bColorKey) {
         m_bpp = 32;
         m_AlphaFlag = 2;
-        m_Pitch = m_Width;
-        if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
-            return FALSE;
-        }
-        m_Pitch *= m_bpp;
-        if (m_Pitch + 31 < m_Pitch) {
-            return FALSE;
-        }
-        m_Pitch += 31;
-        m_Pitch = m_Pitch / 32 * 4;
-        m_pMaskedLine = FX_Alloc(FX_BYTE, m_Pitch);
+        pitch = m_Width;
+        pitch *= m_bpp;
+        pitch += 31;
+        pitch /= 8;
+        if (!pitch.IsValid()) return FALSE;

[Tom Sepez, 2014/07/25 21:13:51]

same two comments as above:  nit for {} and do we need to set m_pitch?

[jun_fang, 2014/07/30 22:27:01]

Will add {} in the next patch.

+
+        m_pMaskedLine = FX_Alloc(FX_BYTE, pitch.ValueOrDie());
     }
+    m_Pitch = pitch.ValueOrDie();
     if (ppMask) {
         *ppMask = LoadMask(*pMatteColor);
     }
     if (m_pColorSpace && bStdCS) {
         m_pColorSpace->EnableStdConversion(FALSE);
     }
     return TRUE;
 }
 int     CPDF_DIBSource::ContinueToLoadMask()
 {
     if (m_bImageMask) {
         m_bpp = 1;
         m_bpc = 1;
         m_nComponents = 1;
         m_AlphaFlag = 1;
     } else if (m_bpc * m_nComponents == 1) {
         m_bpp = 1;
     } else if (m_bpc * m_nComponents <= 8) {
         m_bpp = 8;
     } else {
         m_bpp = 24;
     }
     if (!m_bpc || !m_nComponents) {
         return 0;
     }
-    m_Pitch = m_Width;
-    if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
+    FxDword pitch = m_Width;
+    pitch *= m_bpp;
+    pitch += 31;
+    pitch /= 8;
+    if (!pitch.IsValid()) {
         return 0;
     }
-    m_Pitch *= m_bpp;
-    if (m_Pitch + 31 < m_Pitch) {
-        return 0;
-    }
-    m_Pitch += 31;
-    m_Pitch = m_Pitch / 32 * 4;
-    m_pLineBuf = FX_Alloc(FX_BYTE, m_Pitch);
+    m_pLineBuf = FX_Alloc(FX_BYTE, pitch.ValueOrDie());
     if (m_pColorSpace && m_bStdCS) {
         m_pColorSpace->EnableStdConversion(TRUE);
     }
     LoadPalette();
     if (m_bColorKey) {
         m_bpp = 32;
         m_AlphaFlag = 2;
-        m_Pitch = m_Width;
-        if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
+        pitch = m_Width;
+        pitch *= m_bpp;
+        pitch += 31;
+        pitch /= 8;
+        if (!pitch.IsValid()) {
             return 0;
         }
-        m_Pitch *= m_bpp;
-        if (m_Pitch + 31 < m_Pitch) {
-            return 0;
-        }
-        m_Pitch += 31;
-        m_Pitch = m_Pitch / 32 * 4;
-        m_pMaskedLine = FX_Alloc(FX_BYTE, m_Pitch);
+        m_pMaskedLine = FX_Alloc(FX_BYTE, pitch.ValueOrDie());
     }
+    m_Pitch = pitch.ValueOrDie();
     return 1;
 }
 int     CPDF_DIBSource::StartLoadDIBSource(CPDF_Document* pDoc, const CPDF_Stream* pStream, FX_BOOL bHasMask,
                                        CPDF_Dictionary* pFormResources, CPDF_Dictionary* pPageResources,
                                        FX_BOOL bStdCS, FX_DWORD GroupFamily, FX_BOOL bLoadMask)
 {
     if (pStream == NULL) {
         return 0;
     }
     m_pDocument = pDoc;
     m_pDict = pStream->GetDict();
     m_pStream = pStream;
     m_bStdCS = bStdCS;
     m_bHasMask = bHasMask;
     m_Width = m_pDict->GetInteger(FX_BSTRC("Width"));
     m_Height = m_pDict->GetInteger(FX_BSTRC("Height"));
     if (m_Width <= 0 || m_Height <= 0 || m_Width > 0x01ffff || m_Height > 0x01ffff) {
         return 0;
     }
     m_GroupFamily = GroupFamily;
     m_bLoadMask = bLoadMask;
     if (!LoadColorInfo(m_pStream->GetObjNum() != 0 ? NULL : pFormResources, pPageResources)) {
         return 0;
     }
-    FX_DWORD src_pitch = m_bpc;
-    if (m_bpc != 0 && m_nComponents != 0) {
-        if (src_pitch > 0 && m_nComponents > (unsigned)INT_MAX / src_pitch) {
-            return 0;
-        }
-        src_pitch *= m_nComponents;
-        if (src_pitch > 0 && (FX_DWORD)m_Width > (unsigned)INT_MAX / src_pitch) {
-            return 0;
-        }
-        src_pitch *= m_Width;
-        if (src_pitch + 7 < src_pitch) {
-            return 0;
-        }
-        src_pitch += 7;
-        src_pitch /= 8;
-        if (src_pitch > 0 && (FX_DWORD)m_Height > (unsigned)INT_MAX / src_pitch) {
-            return 0;
-        }
+
+    if (m_bpc == 0 || m_nComponents == 0) {
+        return 0;

[Tom Sepez, 2014/07/25 21:13:51]

same question as above for early return.

[jun_fang, 2014/07/30 22:27:01]

at line 353 of the original code, m_bpc will be used indirectly. If it's 0, it's
meaningless to call LoadAllData(). bpc and components are used to calculate
pitch.pitch is 0 when one of them is 0.

     }
+
+    FxDword src_pitch = m_bpc;
+    src_pitch *= m_nComponents;
+    src_pitch *= m_Width;
+    src_pitch += 7;
+    src_pitch /= 8;
+    src_pitch *= m_Height;
+    if (!src_pitch.IsValid()) {
+        return 0;
+    }
+
     m_pStreamAcc = FX_NEW CPDF_StreamAcc;
-    m_pStreamAcc->LoadAllData(pStream, FALSE, m_Height * src_pitch, TRUE);
+    m_pStreamAcc->LoadAllData(pStream, FALSE, src_pitch.ValueOrDie(), TRUE);
     if (m_pStreamAcc->GetSize() == 0 || m_pStreamAcc->GetData() == NULL) {
         return 0;
     }
     const CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
     if (!decoder.IsEmpty() && decoder == FX_BSTRC("CCITTFaxDecode")) {
         m_bpc = 1;
     }
     int ret = CreateDecoder();
     if (ret != 1) {
         if (!ret) {
@@ skipping 813 matching lines @@
 FX_BOOL CPDF_DIBSource::SkipToScanline(int line, IFX_Pause* pPause) const
 {
     if (m_pDecoder) {
         return m_pDecoder->SkipToScanline(line, pPause);
     }
     return FALSE;
 }
 void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_bpp,
                                         int dest_width, FX_BOOL bFlipX, int clip_left, int clip_width) const
 {
+    if (line < 0 || dest_scan == NULL || dest_bpp <= 0 ||
+        dest_width <= 0 || clip_left < 0 || clip_width <= 0) {
+        return;
+    }
+
     FX_DWORD bpc = GetValidBpc();
     FX_DWORD src_width = m_Width;
-    FX_DWORD src_pitch = (src_width * bpc * m_nComponents + 7) / 8;
+    FxDword pitch = src_width;
+    pitch *= bpc;
+    pitch *= m_nComponents;
+    pitch += 7;
+    pitch /= 8;
+    if (!pitch.IsValid()) { 
+        return;
+    }
+
     FX_LPCBYTE pSrcLine = NULL;
     if (m_pCachedBitmap) {
         pSrcLine = m_pCachedBitmap->GetScanline(line);
     } else if (m_pDecoder) {
         pSrcLine = m_pDecoder->GetScanline(line);
     } else {
+        FX_DWORD src_pitch = pitch.ValueOrDie();
+        pitch *= (line+1);

[Tom Sepez, 2014/07/25 21:13:51]

do you mean src_pitch *= (line + 1)

[jun_fang, 2014/07/30 22:27:01]

at line 1196 of left code (OLD), there is (line + 1) * src_pitch.
at line 1197 of left code (OLD), there is line * src_pitch.
I added a check here to avoid to check two times.

+        if (!pitch.IsValid()) { 

[Tom Sepez, 2014/07/25 21:13:51]

ditto

+            return;
+        }
+ 
         if (m_pStreamAcc->GetSize() >= (line + 1) * src_pitch) {

[Tom Sepez, 2014/07/25 21:13:51]

didn't we just do the (line +1) multiplication above?

[jun_fang, 2014/07/30 22:27:01]

Will update it in the next patch.

             pSrcLine = m_pStreamAcc->GetData() + line * src_pitch;
         }
     }
     int orig_Bpp = bpc * m_nComponents / 8;
     int dest_Bpp = dest_bpp / 8;
     if (pSrcLine == NULL) {
         FXSYS_memset32(dest_scan, 0xff, dest_Bpp * clip_width);
         return;
     }
+
+    Int max_src_x = clip_left;
+    max_src_x += clip_width - 1;
+    max_src_x *= src_width;
+    max_src_x /= dest_width;
+    if (!max_src_x.IsValid()) {
+        return;
+    }
+
     CFX_FixedBufGrow<FX_BYTE, 128> temp(orig_Bpp);
     if (bpc * m_nComponents == 1) {
         FX_DWORD set_argb = (FX_DWORD) - 1, reset_argb = 0;
         if (m_bImageMask) {
             if (m_bDefaultDecode) {
                 set_argb = 0;
                 reset_argb = (FX_DWORD) - 1;
             }
         } else if (m_bColorKey) {
             reset_argb = m_pPalette ? m_pPalette[0] : 0xff000000;
@@ skipping 292 matching lines @@
     if (!m_bCached) {
         if (m_pBitmap) {
             delete m_pBitmap;
             m_pBitmap = NULL;
         }
         if (m_pMask) {
             delete m_pMask;
         }
     }
 }