From ed099befbb300d6f9c393cb415fdb2a68c2ef471 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Fri, 15 May 2015 16:30:52 -0700 Subject: Merge to XFA: Abort on OOM by default in FX_Alloc(). Original Review URL: https://codereview.chromium.org/1128043009 Original Review URL: https://codereview.chromium.org/1142463005 R=thestig@chromium.org TBR=thestig@chromium.org Review URL: https://codereview.chromium.org/1144683002 --- BUILD.gn | 1 + core/include/fxcrt/fx_memory.h | 55 ++++++++++++++++++++----- core/include/fxcrt/fx_system.h | 16 ++++++-- core/src/fxcodec/codec/fx_codec.cpp | 2 +- core/src/fxcodec/lbmp/fx_bmp.cpp | 2 +- core/src/fxcodec/lgif/fx_gif.cpp | 2 +- core/src/fxcrt/fx_basic_memmgr.cpp | 15 ++++--- core/src/fxcrt/fx_basic_memmgr_unittest.cpp | 63 +++++++++++++++++++++++++++++ core/src/fxge/dib/fx_dib_convert.cpp | 2 +- core/src/fxge/dib/fx_dib_engine.cpp | 8 ++-- core/src/fxge/dib/fx_dib_main.cpp | 2 +- pdfium.gyp | 1 + 12 files changed, 142 insertions(+), 27 deletions(-) create mode 100644 core/src/fxcrt/fx_basic_memmgr_unittest.cpp diff --git a/BUILD.gn b/BUILD.gn index b9be538e0b..1dedaae522 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1556,6 +1556,7 @@ test("pdfium_unittests") { sources = [ "core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp", "core/src/fxcrt/fx_basic_bstring_unittest.cpp", + "core/src/fxcrt/fx_basic_memmgr_unittest.cpp", "core/src/fxcrt/fx_basic_wstring_unittest.cpp", "testing/fx_string_testhelpers.cpp", "testing/fx_string_testhelpers.h", diff --git a/core/include/fxcrt/fx_memory.h b/core/include/fxcrt/fx_memory.h index e6685ef0d4..c1c0740ae2 100644 --- a/core/include/fxcrt/fx_memory.h +++ b/core/include/fxcrt/fx_memory.h @@ -10,22 +10,59 @@ #include "fx_system.h" #ifdef __cplusplus -#include extern "C" { #endif -#define FX_Alloc(type, size) (type*)calloc(size, sizeof(type)) -#define FX_Realloc(type, ptr, size) (type*)realloc(ptr, sizeof(type) * (size)) -#define FX_AllocNL(type, size) FX_Alloc(type, size) -#define FX_ReallocNL(type, ptr, size) FX_Realloc(type, ptr, size) -#define FX_Free(ptr) free(ptr) +// For external C libraries to malloc through PDFium. These may return NULL. void* FXMEM_DefaultAlloc(size_t byte_size, int flags); void* FXMEM_DefaultRealloc(void* pointer, size_t new_size, int flags); void FXMEM_DefaultFree(void* pointer, int flags); #ifdef __cplusplus -} +} // extern "C" + +#include +#include +#include #define FX_NEW new +NEVER_INLINE void FX_OutOfMemoryTerminate(); + +inline void* FX_SafeRealloc(void* ptr, size_t num_members, size_t member_size) { + if (num_members < std::numeric_limits::max() / member_size) { + return realloc(ptr, num_members * member_size); + } + return nullptr; +} + +inline void* FX_AllocOrDie(size_t num_members, size_t member_size) { + // TODO(tsepez): See if we can avoid the implicit memset(0). + if (void* result = calloc(num_members, member_size)) { + return result; + } + 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; + } + FX_OutOfMemoryTerminate(); // Never returns. + return nullptr; // Suppress compiler warning. +} + +// Never returns NULL. +#define FX_Alloc(type, size) (type*)FX_AllocOrDie(size, sizeof(type)) +#define FX_Realloc(type, ptr, size) \ + (type*)FX_ReallocOrDie(ptr, size, sizeof(type)) + +// May return NULL. +#define FX_TryAlloc(type, size) (type*)calloc(size, sizeof(type)) +#define FX_TryRealloc(type, ptr, size) \ + (type*)FX_SafeRealloc(ptr, size, sizeof(type)) + +#define FX_Free(ptr) free(ptr) + class CFX_DestructObject { public: @@ -71,5 +108,5 @@ private: void* m_pFirstTrunk; }; -#endif -#endif +#endif // __cplusplust +#endif // _FX_MEMORY_H_ diff --git a/core/include/fxcrt/fx_system.h b/core/include/fxcrt/fx_system.h index 9cc165f7a5..96030ca551 100644 --- a/core/include/fxcrt/fx_system.h +++ b/core/include/fxcrt/fx_system.h @@ -6,10 +6,12 @@ #ifndef _FX_SYSTEM_H_ #define _FX_SYSTEM_H_ + #define _FX_WIN32_DESKTOP_ 1 #define _FX_LINUX_DESKTOP_ 4 #define _FX_MACOSX_ 7 #define _FX_ANDROID_ 12 + #define _FXM_PLATFORM_WINDOWS_ 1 #define _FXM_PLATFORM_LINUX_ 2 #define _FXM_PLATFORM_APPLE_ 3 @@ -341,12 +343,20 @@ int FXSYS_round(FX_FLOAT f); #define PRIuS "zu" #endif -#else // _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ +#else // _FXM_PLATFORM_ != _FXM_PLATFORM_WINDOWS_ #if !defined(PRIuS) #define PRIuS "Iu" #endif -#endif +#endif // _FXM_PLATFORM_ != _FXM_PLATFORM_WINDOWS_ -#endif +// Prevent a function from ever being inlined, typically because we'd +// like it to appear in stack traces. +#if _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ +#define NEVER_INLINE __declspec(noinline) +#else // _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ +#define NEVER_INLINE __attribute__((__noinline__)) +#endif // _FXM_PLATFORM_ == _FXM_PLATFORM_WINDOWS_ + +#endif // _FX_SYSTEM_H_ diff --git a/core/src/fxcodec/codec/fx_codec.cpp b/core/src/fxcodec/codec/fx_codec.cpp index 36dff5e5b2..0c30684e68 100644 --- a/core/src/fxcodec/codec/fx_codec.cpp +++ b/core/src/fxcodec/codec/fx_codec.cpp @@ -123,7 +123,7 @@ void CCodec_ScanlineDecoder::DownScale(int dest_width, int dest_height) FX_Free(m_pDataCache); m_pDataCache = NULL; } - m_pDataCache = (CCodec_ImageDataCache*)FX_AllocNL(FX_BYTE, sizeof(CCodec_ImageDataCache) + m_Pitch * m_OutputHeight); + m_pDataCache = (CCodec_ImageDataCache*)FX_TryAlloc(FX_BYTE, sizeof(CCodec_ImageDataCache) + m_Pitch * m_OutputHeight); if (m_pDataCache == NULL) { return; } diff --git a/core/src/fxcodec/lbmp/fx_bmp.cpp b/core/src/fxcodec/lbmp/fx_bmp.cpp index 4c6eb61757..e868fdeaef 100644 --- a/core/src/fxcodec/lbmp/fx_bmp.cpp +++ b/core/src/fxcodec/lbmp/fx_bmp.cpp @@ -882,7 +882,7 @@ FX_BOOL _bmp_encode_image( bmp_compress_struct_p bmp_ptr, FX_LPBYTE& dst_buf, FX pal_size = sizeof(FX_DWORD) * bmp_ptr->info_header.biClrUsed; } dst_size = head_size + sizeof(FX_DWORD) * bmp_ptr->pal_num; - dst_buf = FX_AllocNL(FX_BYTE, dst_size); + dst_buf = FX_TryAlloc(FX_BYTE, dst_size); if (dst_buf == NULL) { return FALSE; } diff --git a/core/src/fxcodec/lgif/fx_gif.cpp b/core/src/fxcodec/lgif/fx_gif.cpp index 08c1803e8a..bc6109e88e 100644 --- a/core/src/fxcodec/lgif/fx_gif.cpp +++ b/core/src/fxcodec/lgif/fx_gif.cpp @@ -1147,7 +1147,7 @@ static FX_BOOL _gif_write_header( gif_compress_struct_p gif_ptr, FX_LPBYTE& dst_ return TRUE; } dst_len = sizeof(GifHeader) + sizeof(GifLSD) + sizeof(GifGF); - dst_buf = FX_AllocNL(FX_BYTE, dst_len); + dst_buf = FX_TryAlloc(FX_BYTE, dst_len); if (dst_buf == NULL) { return FALSE; } diff --git a/core/src/fxcrt/fx_basic_memmgr.cpp b/core/src/fxcrt/fx_basic_memmgr.cpp index 3b3211c20f..63c609daec 100644 --- a/core/src/fxcrt/fx_basic_memmgr.cpp +++ b/core/src/fxcrt/fx_basic_memmgr.cpp @@ -4,10 +4,9 @@ // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com -#include "../../include/fxcrt/fx_basic.h" -#ifdef __cplusplus -extern "C" { -#endif +#include // For abort(). +#include "../../include/fxcrt/fx_memory.h" + void* FXMEM_DefaultAlloc(size_t byte_size, int flags) { return (void*)malloc(byte_size); @@ -20,9 +19,13 @@ void FXMEM_DefaultFree(void* pointer, int flags) { free(pointer); } -#ifdef __cplusplus + +NEVER_INLINE void FX_OutOfMemoryTerminate() { + // Termimate cleanly if we can, else crash at a specific address (0xbd). + abort(); + reinterpret_cast(0xbd)(); } -#endif + CFX_GrowOnlyPool::CFX_GrowOnlyPool(size_t trunk_size) { m_TrunkSize = trunk_size; diff --git a/core/src/fxcrt/fx_basic_memmgr_unittest.cpp b/core/src/fxcrt/fx_basic_memmgr_unittest.cpp new file mode 100644 index 0000000000..565021d29e --- /dev/null +++ b/core/src/fxcrt/fx_basic_memmgr_unittest.cpp @@ -0,0 +1,63 @@ +// Copyright 2015 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. + +#include + +#include "testing/gtest/include/gtest/gtest.h" +#include "../../include/fxcrt/fx_memory.h" + +namespace { + +const size_t kMaxByteAlloc = std::numeric_limits::max(); +const size_t kMaxIntAlloc = kMaxByteAlloc / sizeof(int); +const size_t kOverflowIntAlloc = kMaxIntAlloc + 100; + +} // namespace + +// TODO(tsepez): re-enable OOM tests if we can find a way to +// prevent it from hosing the bots. +TEST(fxcrt, DISABLED_FX_AllocOOM) { + EXPECT_DEATH_IF_SUPPORTED(FX_Alloc(int, kMaxIntAlloc), ""); + + int* ptr = FX_Alloc(int, 1); + EXPECT_TRUE(ptr); + EXPECT_DEATH_IF_SUPPORTED(FX_Realloc(int, ptr, kMaxIntAlloc), ""); + FX_Free(ptr); +} + +TEST(fxcrt, FX_AllocOverflow) { + EXPECT_DEATH_IF_SUPPORTED(FX_Alloc(int, kOverflowIntAlloc), ""); + + int* ptr = FX_Alloc(int, 1); + EXPECT_TRUE(ptr); + EXPECT_DEATH_IF_SUPPORTED(FX_Realloc(int, ptr, kOverflowIntAlloc), ""); + FX_Free(ptr); +} + +TEST(fxcrt, DISABLED_FX_TryAllocOOM) { + EXPECT_FALSE(FX_TryAlloc(int, kMaxIntAlloc)); + + int* ptr = FX_Alloc(int, 1); + EXPECT_TRUE(ptr); + EXPECT_FALSE(FX_TryRealloc(int, ptr, kMaxIntAlloc)); + FX_Free(ptr); +} + +TEST(fxcrt, FX_TryAllocOverflow) { + EXPECT_FALSE(FX_TryAlloc(int, kOverflowIntAlloc)); + + int* ptr = FX_Alloc(int, 1); + EXPECT_TRUE(ptr); + EXPECT_FALSE(FX_TryRealloc(int, ptr, kOverflowIntAlloc)); + FX_Free(ptr); +} + +TEST(fxcrt, DISABLED_FXMEM_DefaultOOM) { + EXPECT_FALSE(FXMEM_DefaultAlloc(kMaxByteAlloc, 0)); + + void* ptr = FXMEM_DefaultAlloc(1, 0); + EXPECT_TRUE(ptr); + EXPECT_FALSE(FXMEM_DefaultRealloc(ptr, kMaxByteAlloc, 0)); + FXMEM_DefaultFree(ptr, 0); +} diff --git a/core/src/fxge/dib/fx_dib_convert.cpp b/core/src/fxge/dib/fx_dib_convert.cpp index cfcbc707cd..69432fde4d 100644 --- a/core/src/fxge/dib/fx_dib_convert.cpp +++ b/core/src/fxge/dib/fx_dib_convert.cpp @@ -1015,7 +1015,7 @@ FX_BOOL CFX_DIBitmap::ConvertFormat(FXDIB_Format dest_format, void* pIccTransfor } int dest_bpp = dest_format & 0xff; int dest_pitch = (dest_bpp * m_Width + 31) / 32 * 4; - FX_LPBYTE dest_buf = FX_AllocNL(FX_BYTE, dest_pitch * m_Height + 4); + FX_LPBYTE dest_buf = FX_TryAlloc(FX_BYTE, dest_pitch * m_Height + 4); if (dest_buf == NULL) { return FALSE; } diff --git a/core/src/fxge/dib/fx_dib_engine.cpp b/core/src/fxge/dib/fx_dib_engine.cpp index f4c0ef16be..4d398d8c45 100644 --- a/core/src/fxge/dib/fx_dib_engine.cpp +++ b/core/src/fxge/dib/fx_dib_engine.cpp @@ -28,7 +28,7 @@ void CWeightTable::Calc(int dest_len, int dest_min, int dest_max, int src_len, i if ((dest_max - dest_min) > (int)((1U << 30) - 4) / m_ItemSize) { return; } - m_pWeightTables = FX_AllocNL(FX_BYTE, (dest_max - dest_min) * m_ItemSize + 4); + m_pWeightTables = FX_TryAlloc(FX_BYTE, (dest_max - dest_min) * m_ItemSize + 4); if (m_pWeightTables == NULL) { return; } @@ -202,7 +202,7 @@ CStretchEngine::CStretchEngine(IFX_ScanlineComposer* pDestBitmap, FXDIB_Format d } size += 31; size = size / 32 * 4; - m_pDestScanline = FX_AllocNL(FX_BYTE, size); + m_pDestScanline = FX_TryAlloc(FX_BYTE, size); if (m_pDestScanline == NULL) { return; } @@ -311,7 +311,7 @@ FX_BOOL CStretchEngine::StartStretchHorz() if (m_DestWidth == 0 || m_pDestScanline == NULL || m_SrcClip.Height() > (int)((1U << 29) / m_InterPitch) || m_SrcClip.Height() == 0) { return FALSE; } - m_pInterBuf = FX_AllocNL(unsigned char, m_SrcClip.Height() * m_InterPitch); + m_pInterBuf = FX_TryAlloc(unsigned char, m_SrcClip.Height() * m_InterPitch); if (m_pInterBuf == NULL) { return FALSE; } @@ -321,7 +321,7 @@ FX_BOOL CStretchEngine::StartStretchHorz() return FALSE; } FX_DWORD size = (m_DestClip.Width() * 8 + 31) / 32 * 4; - m_pDestMaskScanline = FX_AllocNL(unsigned char, size); + m_pDestMaskScanline = FX_TryAlloc(unsigned char, size); if (!m_pDestMaskScanline) { return FALSE; } diff --git a/core/src/fxge/dib/fx_dib_main.cpp b/core/src/fxge/dib/fx_dib_main.cpp index 2cb41f62b7..d9e401897b 100644 --- a/core/src/fxge/dib/fx_dib_main.cpp +++ b/core/src/fxge/dib/fx_dib_main.cpp @@ -84,7 +84,7 @@ FX_BOOL CFX_DIBitmap::Create(int width, int height, FXDIB_Format format, FX_LPBY int size = pitch * height + 4; int oomlimit = _MAX_OOM_LIMIT_; if (oomlimit >= 0 && size >= oomlimit) { - m_pBuffer = FX_AllocNL(FX_BYTE, size); + m_pBuffer = FX_TryAlloc(FX_BYTE, size); } else { m_pBuffer = FX_Alloc(FX_BYTE, size); } diff --git a/pdfium.gyp b/pdfium.gyp index 7d712c647c..8cd3c372e5 100644 --- a/pdfium.gyp +++ b/pdfium.gyp @@ -924,6 +924,7 @@ 'sources': [ 'core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp', 'core/src/fxcrt/fx_basic_bstring_unittest.cpp', + 'core/src/fxcrt/fx_basic_memmgr_unittest.cpp', 'core/src/fxcrt/fx_basic_wstring_unittest.cpp', 'testing/fx_string_testhelpers.h', 'testing/fx_string_testhelpers.cpp', -- cgit v1.2.3