From 9de00fdc4d4e4dba9e754b337a05809482e4fea2 Mon Sep 17 00:00:00 2001 From: weili Date: Mon, 12 Sep 2016 15:15:19 -0700 Subject: Fix some leaks associated with memory allocator Use CFX_DefStore to only replace CFX_FixedStore, but not CFX_StaticStore, since CFX_StaticStore has different behaviors. CFX_StaticStore doesn't require its users to explicitly call free(), it frees all the allocated memory during destruction. Use CFX_DefStore to replace CFX_StaticStore would cause leaks. Also remove two undeclared, but defined, functions. BUG=pdfium:242 Review-Url: https://codereview.chromium.org/2328403002 --- xfa/fgas/crt/fgas_memory.cpp | 74 +++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 38 deletions(-) (limited to 'xfa/fgas') diff --git a/xfa/fgas/crt/fgas_memory.cpp b/xfa/fgas/crt/fgas_memory.cpp index e587f5a5b0..c68241fcf1 100644 --- a/xfa/fgas/crt/fgas_memory.cpp +++ b/xfa/fgas/crt/fgas_memory.cpp @@ -7,35 +7,14 @@ #include "xfa/fgas/crt/fgas_memory.h" #ifndef MEMORY_TOOL_REPLACES_ALLOCATOR -#define MEMORY_TOOL_REPLACES_ALLOCATOR // Temporary, for CF testing. +// Use CFX_DefStore to replace CFX_FixedStore to simplify memory +// management so that some problems such Use-After-Free can be +// detected by Asan or ClusterFuzz tools. +#define MEMORY_TOOL_REPLACES_ALLOCATOR #endif #include -#ifdef MEMORY_TOOL_REPLACES_ALLOCATOR - -namespace { - -class CFX_DefStore : public IFX_MemoryAllocator, public CFX_Target { - public: - CFX_DefStore() {} - ~CFX_DefStore() override {} - - void* Alloc(size_t size) override { return FX_Alloc(uint8_t, size); } - void Free(void* pBlock) override { FX_Free(pBlock); } -}; - -} // namespace - -std::unique_ptr IFX_MemoryAllocator::Create( - FX_ALLOCTYPE eType, - size_t chunkSize, - size_t blockSize) { - return std::unique_ptr(new CFX_DefStore()); -} - -#else // MEMORY_TOOL_REPLACES_ALLOCATOR - namespace { struct FX_STATICSTORECHUNK { @@ -61,6 +40,19 @@ class CFX_StaticStore : public IFX_MemoryAllocator, public CFX_Target { FX_STATICSTORECHUNK* FindChunk(size_t size); }; +#ifdef MEMORY_TOOL_REPLACES_ALLOCATOR + +class CFX_DefStore : public IFX_MemoryAllocator, public CFX_Target { + public: + CFX_DefStore() {} + ~CFX_DefStore() override {} + + void* Alloc(size_t size) override { return FX_Alloc(uint8_t, size); } + void Free(void* pBlock) override { FX_Free(pBlock); } +}; + +#else + struct FX_FIXEDSTORECHUNK { uint8_t* FirstFlag() { return reinterpret_cast(this + 1); } uint8_t* FirstBlock() { return FirstFlag() + iChunkSize; } @@ -85,6 +77,8 @@ class CFX_FixedStore : public IFX_MemoryAllocator, public CFX_Target { FX_FIXEDSTORECHUNK* m_pChunk; }; +#endif // MEMORY_TOOL_REPLACES_ALLOCATOR + } // namespace #define FX_4BYTEALIGN(size) (((size) + 3) & ~3) @@ -98,7 +92,12 @@ std::unique_ptr IFX_MemoryAllocator::Create( return std::unique_ptr( new CFX_StaticStore(chunkSize)); case FX_ALLOCTYPE_Fixed: - return std::unique_ptr(new CFX_FixedStore(blockSize, chunkSize); +#ifdef MEMORY_TOOL_REPLACES_ALLOCATOR + return std::unique_ptr(new CFX_DefStore()); +#else + return std::unique_ptr( + new CFX_FixedStore(blockSize, chunkSize)); +#endif // MEMORY_TOOL_REPLACES_ALLOCATOR default: ASSERT(0); return std::unique_ptr(); @@ -112,6 +111,7 @@ CFX_StaticStore::CFX_StaticStore(size_t iDefChunkSize) m_pLastChunk(nullptr) { ASSERT(m_iDefChunkSize != 0); } + CFX_StaticStore::~CFX_StaticStore() { FX_STATICSTORECHUNK* pChunk = m_pChunk; while (pChunk) { @@ -120,6 +120,7 @@ CFX_StaticStore::~CFX_StaticStore() { pChunk = pNext; } } + FX_STATICSTORECHUNK* CFX_StaticStore::AllocChunk(size_t size) { ASSERT(size != 0); FX_STATICSTORECHUNK* pChunk = (FX_STATICSTORECHUNK*)FX_Alloc( @@ -135,6 +136,7 @@ FX_STATICSTORECHUNK* CFX_StaticStore::AllocChunk(size_t size) { m_pLastChunk = pChunk; return pChunk; } + FX_STATICSTORECHUNK* CFX_StaticStore::FindChunk(size_t size) { ASSERT(size != 0); if (!m_pLastChunk || m_pLastChunk->iFreeSize < size) { @@ -142,6 +144,7 @@ FX_STATICSTORECHUNK* CFX_StaticStore::FindChunk(size_t size) { } return m_pLastChunk; } + void* CFX_StaticStore::Alloc(size_t size) { size = FX_4BYTEALIGN(size); ASSERT(size != 0); @@ -153,18 +156,16 @@ void* CFX_StaticStore::Alloc(size_t size) { m_iAllocatedSize += size; return p; } -size_t CFX_StaticStore::SetDefChunkSize(size_t size) { - ASSERT(size != 0); - size_t v = m_iDefChunkSize; - m_iDefChunkSize = size; - return v; -} + +#ifndef MEMORY_TOOL_REPLACES_ALLOCATOR + CFX_FixedStore::CFX_FixedStore(size_t iBlockSize, size_t iBlockNumsInChunk) : m_iBlockSize(FX_4BYTEALIGN(iBlockSize)), m_iDefChunkSize(FX_4BYTEALIGN(iBlockNumsInChunk)), m_pChunk(nullptr) { ASSERT(m_iBlockSize != 0 && m_iDefChunkSize != 0); } + CFX_FixedStore::~CFX_FixedStore() { FX_FIXEDSTORECHUNK* pChunk = m_pChunk; while (pChunk) { @@ -173,6 +174,7 @@ CFX_FixedStore::~CFX_FixedStore() { pChunk = pNext; } } + FX_FIXEDSTORECHUNK* CFX_FixedStore::AllocChunk() { int32_t iTotalSize = sizeof(FX_FIXEDSTORECHUNK) + m_iDefChunkSize + m_iBlockSize * m_iDefChunkSize; @@ -188,6 +190,7 @@ FX_FIXEDSTORECHUNK* CFX_FixedStore::AllocChunk() { m_pChunk = pChunk; return pChunk; } + void* CFX_FixedStore::Alloc(size_t size) { if (size > m_iBlockSize) { return nullptr; @@ -213,6 +216,7 @@ void* CFX_FixedStore::Alloc(size_t size) { pChunk->iFreeNum--; return pChunk->FirstBlock() + i * m_iBlockSize; } + void CFX_FixedStore::Free(void* pBlock) { FX_FIXEDSTORECHUNK* pPrior = nullptr; FX_FIXEDSTORECHUNK* pChunk = m_pChunk; @@ -246,11 +250,5 @@ void CFX_FixedStore::Free(void* pBlock) { FX_Free(pChunk); } } -size_t CFX_FixedStore::SetDefChunkSize(size_t iChunkSize) { - ASSERT(iChunkSize != 0); - size_t v = m_iDefChunkSize; - m_iDefChunkSize = FX_4BYTEALIGN(iChunkSize); - return v; -} #endif // MEMORY_TOOL_REPLACES_ALLOCATOR -- cgit v1.2.3