Fix potential integer overflow in fpdf_render_loadimage.cpp

core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp

Index: core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
index 81cff01f507266a1ee40c0945938bba22749aa2d..5c8382be80ed6461e68663868af0226dec695164 100644
--- a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
@@ -11,7 +11,11 @@
 #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];
@@ -175,27 +179,23 @@ FX_BOOL CPDF_DIBSource::Load(CPDF_Document* pDoc, const CPDF_Stream* pStream, CP
     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;
     }
@@ -218,20 +218,14 @@ FX_BOOL CPDF_DIBSource::Load(CPDF_Document* pDoc, const CPDF_Stream* pStream, CP
     } 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);
     }
@@ -239,18 +233,15 @@ FX_BOOL CPDF_DIBSource::Load(CPDF_Document* pDoc, const CPDF_Stream* pStream, CP
     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);
     }
@@ -276,17 +267,14 @@ int	CPDF_DIBSource::ContinueToLoadMask()
     if (!m_bpc || !m_nComponents) {
         return 0;
     }
-    m_Pitch = m_Width;
-    if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
-        return 0;
-    }
-    m_Pitch *= m_bpp;
-    if (m_Pitch + 31 < m_Pitch) {
+    FxDword pitch = m_Width;
+    pitch *= m_bpp;
+    pitch += 31;
+    pitch /= 8;
+    if (!pitch.IsValid()) {
         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);
     }
@@ -294,18 +282,16 @@ int	CPDF_DIBSource::ContinueToLoadMask()
     if (m_bColorKey) {
         m_bpp = 32;
         m_AlphaFlag = 2;
-        m_Pitch = m_Width;
-        if ((FX_DWORD)m_bpp > (unsigned)INT_MAX / m_Pitch) {
-            return 0;
-        }
-        m_Pitch *= m_bpp;
-        if (m_Pitch + 31 < m_Pitch) {
+        pitch = m_Width;
+        pitch *= m_bpp;
+        pitch += 31;
+        pitch /= 8;
+        if (!pitch.IsValid()) {
             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,
@@ -330,27 +316,23 @@ int	CPDF_DIBSource::StartLoadDIBSource(CPDF_Document* pDoc, const CPDF_Stream* p
     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;
     }
@@ -1184,15 +1166,34 @@ FX_BOOL CPDF_DIBSource::SkipToScanline(int line, IFX_Pause* pPause) const
 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;
         }
@@ -1203,6 +1204,15 @@ void CPDF_DIBSource::DownSampleScanline(int line, FX_LPBYTE dest_scan, int dest_
         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;