Add safe FX_Alloc2D() macro
authorTom Sepez <tsepez@chromium.org>
Mon, 18 May 2015 21:18:08 +0000 (14:18 -0700)
committerTom Sepez <tsepez@chromium.org>
Mon, 18 May 2015 21:18:08 +0000 (14:18 -0700)
This avoids unchecked multiplications when computing a size argument
to malloc(). Such an overflow is very scary, and can result in
exploitable bugs.

Along the way, kill off some return checks, since we know this can't
return NULL.

R=thestig@chromium.org

Review URL: https://codereview.chromium.org/1143663004

19 files changed:
core/include/fxcrt/fx_basic.h
core/include/fxcrt/fx_memory.h
core/src/fpdfapi/fpdf_edit/fpdf_edit_image.cpp
core/src/fpdfapi/fpdf_font/fpdf_font_cid.cpp
core/src/fpdfapi/fpdf_page/fpdf_page_colors.cpp
core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp
core/src/fpdfapi/fpdf_parser/fpdf_parser_filters.cpp
core/src/fpdfapi/fpdf_render/fpdf_render_cache.cpp
core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp
core/src/fpdftext/fpdf_text.cpp
core/src/fxcodec/codec/fx_codec_fax.cpp
core/src/fxcodec/codec/fx_codec_flate.cpp
core/src/fxcodec/codec/fx_codec_jpeg.cpp
core/src/fxcrt/fx_basic_array.cpp
core/src/fxcrt/fx_basic_memmgr_unittest.cpp
core/src/fxge/agg/agg23/fx_agg_path_storage.cpp
core/src/fxge/dib/fx_dib_engine.cpp
core/src/fxge/skia/fx_skia_device.cpp
core/src/fxge/win32/fx_win32_gdipext.cpp

index bd53d9e..f258029 100644 (file)
@@ -1454,10 +1454,7 @@ public:
         while (nCount > 0) {
             FX_INT32 temp_count = FX_MIN(nCount, FX_DATALIST_LENGTH);
             DataList list;
-            list.data = FX_Alloc(FX_BYTE, temp_count * unit);
-            if (!list.data) {
-                break;
-            }
+            list.data = FX_Alloc2D(FX_BYTE, temp_count, unit);
             list.start = nStart;
             list.count = temp_count;
             Append(list);
index afce911..a75ff25 100644 (file)
@@ -41,6 +41,14 @@ inline void* FX_AllocOrDie(size_t num_members, size_t member_size) {
     return nullptr;  // Suppress compiler warning.
 }
 
+inline void* FX_AllocOrDie2D(size_t w, size_t h, size_t member_size) {
+    if (w < std::numeric_limits<size_t>::max() / h) {
+        return FX_AllocOrDie(w * h, member_size);
+    }
+    FX_OutOfMemoryTerminate();  // Never returns.
+    return nullptr;  // Suppress compiler warning.
+}
+
 inline void* FX_ReallocOrDie(void* ptr, size_t num_members, size_t member_size) {
     if (void* result = FX_SafeRealloc(ptr, num_members, member_size)) {
         return result;
@@ -51,6 +59,7 @@ inline void* FX_ReallocOrDie(void* ptr, size_t num_members, size_t member_size)
 
 // Never returns NULL.
 #define FX_Alloc(type, size) (type*)FX_AllocOrDie(size, sizeof(type))
+#define FX_Alloc2D(type, w, h) (type*)FX_AllocOrDie2D(w, h, sizeof(type))
 #define FX_Realloc(type, ptr, size) \
     (type*)FX_ReallocOrDie(ptr, size, sizeof(type))
 
index 3a3756d..1328fcd 100644 (file)
@@ -162,7 +162,7 @@ void CPDF_Image::SetImage(const CFX_DIBitmap* pBitmap, FX_INT32 iCompress, IFX_F
             pCS->AddName(FX_BSTRC("Indexed"));
             pCS->AddName(FX_BSTRC("DeviceRGB"));
             pCS->AddInteger(iPalette - 1);
-            FX_LPBYTE pColorTable = FX_Alloc(FX_BYTE, iPalette * 3);
+            FX_LPBYTE pColorTable = FX_Alloc2D(FX_BYTE, iPalette, 3);
             FX_LPBYTE ptr = pColorTable;
             for (FX_INT32 i = 0; i < iPalette; i ++) {
                 FX_DWORD argb = pBitmap->GetPaletteArgb(i);
index 6911942..a08660c 100644 (file)
@@ -254,7 +254,7 @@ void CPDF_CMapParser::ParseWord(FX_BSTR word)
             if (nSegs > 1) {
                 m_pCMap->m_CodingScheme = CPDF_CMap::MixedFourBytes;
                 m_pCMap->m_nCodeRanges = nSegs;
-                m_pCMap->m_pLeadingBytes = FX_Alloc(FX_BYTE, nSegs * sizeof(_CMap_CodeRange));
+                m_pCMap->m_pLeadingBytes = FX_Alloc2D(FX_BYTE, nSegs, sizeof(_CMap_CodeRange));
                 FXSYS_memcpy32(m_pCMap->m_pLeadingBytes, m_CodeRanges.GetData(), nSegs * sizeof(_CMap_CodeRange));
             } else if (nSegs == 1) {
                 m_pCMap->m_CodingScheme = (m_CodeRanges[0].m_CharSize == 2) ? CPDF_CMap::TwoBytes : CPDF_CMap::OneByte;
index b6bf795..8b9ff8e 100644 (file)
@@ -643,7 +643,7 @@ FX_BOOL CPDF_ICCBasedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray)
         }
     }
     CPDF_Array* pRanges = pDict->GetArray(FX_BSTRC("Range"));
-    m_pRanges = FX_Alloc(FX_FLOAT, m_nComponents * 2);
+    m_pRanges = FX_Alloc2D(FX_FLOAT, m_nComponents, 2);
     for (int i = 0; i < m_nComponents * 2; i ++) {
         if (pRanges) {
             m_pRanges[i] = pRanges->GetNumber(i);
@@ -715,8 +715,8 @@ void CPDF_ICCBasedCS::TranslateImageLine(FX_LPBYTE pDestBuf, FX_LPCBYTE pSrcBuf,
             CPDF_ModuleMgr::Get()->GetIccModule()->TranslateScanline(m_pProfile->m_pTransform, pDestBuf, pSrcBuf, pixels);
         } else {
             if (m_pCache == NULL) {
-                ((CPDF_ICCBasedCS*)this)->m_pCache = FX_Alloc(FX_BYTE, nMaxColors * 3);
-                FX_LPBYTE temp_src = FX_Alloc(FX_BYTE, nMaxColors * m_nComponents);
+                ((CPDF_ICCBasedCS*)this)->m_pCache = FX_Alloc2D(FX_BYTE, nMaxColors, 3);
+                FX_LPBYTE temp_src = FX_Alloc2D(FX_BYTE, nMaxColors, m_nComponents);
                 FX_LPBYTE pSrc = temp_src;
                 for (int i = 0; i < nMaxColors; i ++) {
                     FX_DWORD color = i;
@@ -804,7 +804,7 @@ FX_BOOL CPDF_IndexedCS::v_Load(CPDF_Document* pDoc, CPDF_Array* pArray)
     }
     m_pCountedBaseCS = pDocPageData->FindColorSpacePtr(m_pBaseCS->GetArray());
     m_nBaseComponents = m_pBaseCS->CountComponents();
-    m_pCompMinMax = FX_Alloc(FX_FLOAT, m_nBaseComponents * 2);
+    m_pCompMinMax = FX_Alloc2D(FX_FLOAT, m_nBaseComponents, 2);
     FX_FLOAT defvalue;
     for (int i = 0; i < m_nBaseComponents; i ++) {
         m_pBaseCS->GetDefaultValue(i, defvalue, m_pCompMinMax[i * 2], m_pCompMinMax[i * 2 + 1]);
index bd1cdb6..8ec490a 100644 (file)
@@ -670,8 +670,8 @@ FX_BOOL CPDF_ExpIntFunc::v_Init(CPDF_Object* pObj)
         }
     }
     CPDF_Array* pArray1 = pDict->GetArray(FX_BSTRC("C1"));
-    m_pBeginValues = FX_Alloc(FX_FLOAT, m_nOutputs * 2);
-    m_pEndValues = FX_Alloc(FX_FLOAT, m_nOutputs * 2);
+    m_pBeginValues = FX_Alloc2D(FX_FLOAT, m_nOutputs, 2);
+    m_pEndValues = FX_Alloc2D(FX_FLOAT, m_nOutputs, 2);
     for (int i = 0; i < m_nOutputs; i ++) {
         m_pBeginValues[i] = pArray0 ? pArray0->GetFloat(i) : 0.0f;
         m_pEndValues[i] = pArray1 ? pArray1->GetFloat(i) : 1.0f;
@@ -768,7 +768,7 @@ FX_BOOL CPDF_StitchFunc::v_Init(CPDF_Object* pObj)
         m_pBounds[i + 1] = pArray->GetFloat(i);
     }
     m_pBounds[m_nSubs] = m_pDomains[1];
-    m_pEncode = FX_Alloc(FX_FLOAT, m_nSubs * 2);
+    m_pEncode = FX_Alloc2D(FX_FLOAT, m_nSubs, 2);
     pArray = pDict->GetArray(FX_BSTRC("Encode"));
     if (pArray == NULL) {
         return FALSE;
@@ -857,7 +857,7 @@ FX_BOOL CPDF_Function::Init(CPDF_Object* pObj)
     if (m_nInputs == 0) {
         return FALSE;
     }
-    m_pDomains = FX_Alloc(FX_FLOAT, m_nInputs * 2);
+    m_pDomains = FX_Alloc2D(FX_FLOAT, m_nInputs, 2);
     for (int i = 0; i < m_nInputs * 2; i ++) {
         m_pDomains[i] = pDomains->GetFloat(i);
     }
@@ -865,7 +865,7 @@ FX_BOOL CPDF_Function::Init(CPDF_Object* pObj)
     m_nOutputs = 0;
     if (pRanges) {
         m_nOutputs = pRanges->GetCount() / 2;
-        m_pRanges = FX_Alloc(FX_FLOAT, m_nOutputs * 2);
+        m_pRanges = FX_Alloc2D(FX_FLOAT, m_nOutputs, 2);
         for (int i = 0; i < m_nOutputs * 2; i ++) {
             m_pRanges[i] = pRanges->GetFloat(i);
         }
index ac3f2b2..6fa7419 100644 (file)
@@ -296,7 +296,7 @@ void CPDF_DecryptFilter::v_FilterFinish(CFX_BinaryBuf& dest_buf)
 extern "C" {
     static void* my_alloc_func (void* opaque, unsigned int items, unsigned int size)
     {
-        return FX_Alloc(FX_BYTE, items * size);
+        return FX_Alloc2D(FX_BYTE, items, size);
     }
     static void   my_free_func  (void* opaque, void* address)
     {
index 658fc47..83d5f0a 100644 (file)
@@ -43,7 +43,7 @@ void CPDF_PageRenderCache::CacheOptimization(FX_INT32 dwLimitCacheSize)
         return;
     }
     int nCount = m_ImageCaches.GetCount();
-    CACHEINFO* pCACHEINFO = (CACHEINFO*)FX_Alloc(FX_BYTE, (sizeof (CACHEINFO)) * nCount);
+    CACHEINFO* pCACHEINFO = (CACHEINFO*)FX_Alloc2D(FX_BYTE, sizeof(CACHEINFO), nCount);
     FX_POSITION pos = m_ImageCaches.GetStartPosition();
     int i = 0;
     while (pos) {
index 2d7e1ae..f9eec23 100644 (file)
@@ -422,7 +422,7 @@ static void _DrawLatticeGouraudShading(CFX_DIBitmap* pBitmap, CFX_AffineMatrix*
     if (!stream.Load(pShadingStream, pFuncs, nFuncs, pCS)) {
         return;
     }
-    CPDF_MeshVertex* vertex = FX_Alloc(CPDF_MeshVertex, row_verts * 2);
+    CPDF_MeshVertex* vertex = FX_Alloc2D(CPDF_MeshVertex, row_verts, 2);
     if (!stream.GetVertexRow(vertex, row_verts, pObject2Bitmap)) {
         FX_Free(vertex);
         return;
index 6c1e225..a0b0104 100644 (file)
@@ -57,10 +57,9 @@ void CTextPage::ProcessObject(CPDF_PageObject* pObject)
     CPDF_TextObject* pText = (CPDF_TextObject*)pObject;
     CPDF_Font* pFont = pText->m_TextState.GetFont();
     int count = pText->CountItems();
-    FX_FLOAT* pPosArray = FX_Alloc(FX_FLOAT, count * 2);
-    if (pPosArray) {
-        pText->CalcCharPos(pPosArray);
-    }
+    FX_FLOAT* pPosArray = FX_Alloc2D(FX_FLOAT, count, 2);
+    pText->CalcCharPos(pPosArray);
+
     FX_FLOAT fontsize_h = pText->m_TextState.GetFontSizeH();
     FX_FLOAT fontsize_v = pText->m_TextState.GetFontSizeV();
     FX_DWORD space_charcode = pFont->CharCodeFromUnicode(' ');
index 667b713..33e89e4 100644 (file)
@@ -949,10 +949,7 @@ CCodec_FaxEncoder::CCodec_FaxEncoder(FX_LPCBYTE src_buf, int width, int height,
         return;
     }
     FXSYS_memset8(m_pRefLine, 0xff, m_Pitch);
-    m_pLineBuf = FX_Alloc(FX_BYTE, m_Pitch * 8);
-    if (m_pLineBuf == NULL) {
-        return;
-    }
+    m_pLineBuf = FX_Alloc2D(FX_BYTE, m_Pitch, 8);
     m_DestBuf.EstimateSize(0, 10240);
 }
 CCodec_FaxEncoder::~CCodec_FaxEncoder()
index bbee167..4d43cc5 100644 (file)
@@ -13,7 +13,7 @@ extern "C"
 {
     static void* my_alloc_func (void* opaque, unsigned int items, unsigned int size)
     {
-        return FX_Alloc(FX_BYTE, items * size);
+        return FX_Alloc2D(FX_BYTE, items, size);
     }
     static void   my_free_func  (void* opaque, void* address)
     {
@@ -241,9 +241,7 @@ static FX_BOOL PNG_PredictorEncode(FX_LPBYTE& data_buf, FX_DWORD& data_size,
         return FALSE;
     const int row_count = (data_size + row_size - 1) / row_size;
     const int last_row_size = data_size % row_size;
-    FX_LPBYTE dest_buf = FX_Alloc( FX_BYTE, (row_size + 1) * row_count);
-    if (dest_buf == NULL)
-        return FALSE;
+    FX_LPBYTE dest_buf = FX_Alloc2D(FX_BYTE, row_size + 1, row_count);
     int byte_cnt = 0;
     FX_LPBYTE pSrcData = data_buf;
     FX_LPBYTE pDestData = dest_buf;
@@ -397,9 +395,7 @@ static FX_BOOL PNG_Predictor(FX_LPBYTE& data_buf, FX_DWORD& data_size,
         return FALSE;
     const int row_count = (data_size + row_size) / (row_size + 1);
     const int last_row_size = data_size % (row_size + 1);
-    FX_LPBYTE dest_buf = FX_Alloc( FX_BYTE, row_size * row_count);
-    if (dest_buf == NULL)
-        return FALSE;
+    FX_LPBYTE dest_buf = FX_Alloc2D(FX_BYTE, row_size, row_count);
     int byte_cnt = 0;
     FX_LPBYTE pSrcData = data_buf;
     FX_LPBYTE pDestData = dest_buf;
index d4c9926..60575d4 100644 (file)
@@ -153,10 +153,7 @@ static void _JpegEncode(const CFX_DIBSource* pSource, FX_LPBYTE& dest_buf, FX_ST
     }
     FX_LPBYTE line_buf = NULL;
     if (nComponents > 1) {
-        line_buf = FX_Alloc(FX_BYTE, width * nComponents);
-        if (line_buf == NULL) {
-            return;
-        }
+        line_buf = FX_Alloc2D(FX_BYTE, width, nComponents);
     }
     jpeg_set_defaults(&cinfo);
     if(quality != 75) {
index 9bdc607..5a2a2e5 100644 (file)
@@ -189,10 +189,7 @@ void* CFX_BaseSegmentedArray::Add()
     if (m_DataSize % m_SegmentSize) {
         return GetAt(m_DataSize ++);
     }
-    void* pSegment = FX_Alloc(FX_BYTE, m_UnitSize * m_SegmentSize);
-    if (!pSegment) {
-        return NULL;
-    }
+    void* pSegment = FX_Alloc2D(FX_BYTE, m_UnitSize, m_SegmentSize);
     if (m_pIndex == NULL) {
         m_pIndex = pSegment;
         m_DataSize ++;
index 565021d..c70f3b1 100644 (file)
@@ -12,6 +12,8 @@ namespace {
 const size_t kMaxByteAlloc = std::numeric_limits<size_t>::max();
 const size_t kMaxIntAlloc = kMaxByteAlloc / sizeof(int);
 const size_t kOverflowIntAlloc = kMaxIntAlloc + 100;
+const size_t kWidth = 640;
+const size_t kOverflowIntAlloc2D = kMaxIntAlloc / kWidth + 10;
 
 }  // namespace
 
@@ -35,6 +37,11 @@ TEST(fxcrt, FX_AllocOverflow) {
     FX_Free(ptr);
 }
 
+TEST(fxcrt, FX_AllocOverflow2D) {
+    EXPECT_DEATH_IF_SUPPORTED(
+        FX_Alloc2D(int, kWidth, kOverflowIntAlloc2D), "");
+}
+
 TEST(fxcrt, DISABLED_FX_TryAllocOOM) {
     EXPECT_FALSE(FX_TryAlloc(int, kMaxIntAlloc));
 
index 8c4b701..b4b184e 100644 (file)
@@ -51,10 +51,7 @@ void path_storage::allocate_block(unsigned nb)
 {
     if(nb >= m_max_blocks) {
         FX_FLOAT** new_coords =
-            FX_Alloc( FX_FLOAT*, (m_max_blocks + block_pool) * 2);
-        if (!new_coords) {
-            return;
-        }
+            FX_Alloc2D(FX_FLOAT*, m_max_blocks + block_pool, 2);
         unsigned char** new_cmds =
             (unsigned char**)(new_coords + m_max_blocks + block_pool);
         if(m_coord_blocks) {
index 5053c30..7c40171 100644 (file)
@@ -316,10 +316,7 @@ FX_BOOL CStretchEngine::StartStretchHorz()
         return FALSE;
     }
     if (m_pSource && m_bHasAlpha && m_pSource->m_pAlphaMask) {
-        m_pExtraAlphaBuf = FX_Alloc(unsigned char, m_SrcClip.Height() * m_ExtraMaskPitch);
-        if (!m_pExtraAlphaBuf) {
-            return FALSE;
-        }
+        m_pExtraAlphaBuf = FX_Alloc2D(unsigned char, m_SrcClip.Height(), m_ExtraMaskPitch);
         FX_DWORD size = (m_DestClip.Width() * 8 + 31) / 32 * 4;
         m_pDestMaskScanline = FX_TryAlloc(unsigned char, size);
         if (!m_pDestMaskScanline) {
index cc4059d..a483eca 100644 (file)
@@ -210,7 +210,7 @@ static void SkRasterizeStroke(SkPaint& spaint, SkPath* dstPathData, SkPath& path
                dstPathData->transform(smatrix);
        } else {
                int count = (pGraphState->m_DashCount+1)/2;
-               SkScalar* intervals = FX_Alloc(SkScalar, count* sizeof (SkScalar));
+               SkScalar* intervals = FX_Alloc2D(SkScalar, count, sizeof(SkScalar));
                // Set dash pattern
                for (int i = 0; i < count; i ++) {
                        FX_FIXFLOAT on = pGraphState->m_DashArray[i*2];
index fae1883..49c3f2b 100644 (file)
@@ -1266,16 +1266,14 @@ CFX_DIBitmap* CGdiplusExt::LoadDIBitmap(WINDIB_Open_Args_ args)
     int height = abs(pInfo->pbmi->bmiHeader.biHeight);
     int width = pInfo->pbmi->bmiHeader.biWidth;
     int dest_pitch = (width * pInfo->pbmi->bmiHeader.biBitCount + 31) / 32 * 4;
-    LPBYTE pData = FX_Alloc(BYTE, dest_pitch * height);
-    if (pData == NULL) {
-        FreeDIBitmap(pInfo);
-        return NULL;
-    }
+    LPBYTE pData = FX_Alloc2D(BYTE, dest_pitch, height);
     if (dest_pitch == pInfo->Stride) {
         FXSYS_memcpy32(pData, pInfo->pScan0, dest_pitch * height);
-    } else for (int i = 0; i < height; i ++) {
+    } else {
+        for (int i = 0; i < height; i ++) {
             FXSYS_memcpy32(pData + dest_pitch * i, pInfo->pScan0 + pInfo->Stride * i, dest_pitch);
         }
+    }
     CFX_DIBitmap* pDIBitmap = _FX_WindowsDIB_LoadFromBuf(pInfo->pbmi, pData, pInfo->pbmi->bmiHeader.biBitCount == 32);
     FX_Free(pData);
     FreeDIBitmap(pInfo);