From dc359b03ab6a70ee52a91119ff6704cae92f4809 Mon Sep 17 00:00:00 2001 From: thestig Date: Tue, 9 Aug 2016 15:46:20 -0700 Subject: Fix a leak with FPDFPageObj_NewImgeObj(). BUG=pdfium:545 Review-Url: https://codereview.chromium.org/2194393002 --- BUILD.gn | 1 + .../fpdf_edit/cpdf_pagecontentgenerator.cpp | 4 +- core/fpdfapi/fpdf_page/cpdf_image.cpp | 95 ++++++++++------------ core/fpdfapi/fpdf_page/cpdf_imageobject.cpp | 43 +++++++--- core/fpdfapi/fpdf_page/fpdf_page_doc.cpp | 6 +- core/fpdfapi/fpdf_page/fpdf_page_parser.cpp | 10 +-- core/fpdfapi/fpdf_page/include/cpdf_image.h | 20 ++--- core/fpdfapi/fpdf_page/include/cpdf_imageobject.h | 11 ++- fpdfsdk/fpdfeditimg.cpp | 2 +- fpdfsdk/fpdfeditimg_unittest.cpp | 52 ++++++++++++ pdfium.gyp | 1 + 11 files changed, 156 insertions(+), 89 deletions(-) create mode 100644 fpdfsdk/fpdfeditimg_unittest.cpp diff --git a/BUILD.gn b/BUILD.gn index 4b8f2223da..87ee80f182 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1569,6 +1569,7 @@ test("pdfium_unittests") { "core/fxcrt/fx_extension_unittest.cpp", "core/fxcrt/fx_system_unittest.cpp", "fpdfsdk/fpdfdoc_unittest.cpp", + "fpdfsdk/fpdfeditimg_unittest.cpp", ] deps = [ ":pdfium", diff --git a/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp b/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp index a85591ceef..0e4bab1ae1 100644 --- a/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp +++ b/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp @@ -99,12 +99,12 @@ void CPDF_PageContentGenerator::ProcessImage(CFX_ByteTextBuf& buf, uint32_t dwSavedObjNum = pStream->GetObjNum(); CFX_ByteString name = RealizeResource(pStream, "XObject"); if (dwSavedObjNum == 0) { - pImage->Release(); - pImageObj->m_pImage = m_pDocument->GetPageData()->GetImage(pStream); + pImageObj->SetUnownedImage(m_pDocument->GetPageData()->GetImage(pStream)); } buf << "/" << PDF_NameEncode(name) << " Do Q\n"; } } + void CPDF_PageContentGenerator::ProcessForm(CFX_ByteTextBuf& buf, const uint8_t* data, uint32_t size, diff --git a/core/fpdfapi/fpdf_page/cpdf_image.cpp b/core/fpdfapi/fpdf_page/cpdf_image.cpp index 63e006fc06..af361eb9f6 100644 --- a/core/fpdfapi/fpdf_page/cpdf_image.cpp +++ b/core/fpdfapi/fpdf_page/cpdf_image.cpp @@ -6,6 +6,10 @@ #include "core/fpdfapi/fpdf_page/include/cpdf_image.h" +#include +#include +#include + #include "core/fpdfapi/fpdf_page/include/cpdf_page.h" #include "core/fpdfapi/fpdf_page/pageint.h" #include "core/fpdfapi/fpdf_parser/cpdf_boolean.h" @@ -19,14 +23,35 @@ #include "core/fxge/include/fx_dib.h" CPDF_Image::CPDF_Image(CPDF_Document* pDoc) + : CPDF_Image(pDoc, nullptr, false) {} + +CPDF_Image::CPDF_Image(CPDF_Document* pDoc, CPDF_Stream* pStream, bool bInline) : m_pDIBSource(nullptr), m_pMask(nullptr), m_MatteColor(0), - m_pStream(nullptr), - m_bInline(FALSE), + m_pStream(pStream), + m_bInline(bInline), m_pInlineDict(nullptr), + m_Height(0), + m_Width(0), + m_bIsMask(false), + m_bInterpolate(false), m_pDocument(pDoc), - m_pOC(nullptr) {} + m_pOC(nullptr) { + if (!pStream) + return; + + CPDF_Dictionary* pDict = pStream->GetDict(); + if (m_bInline) + m_pInlineDict = ToDictionary(pDict->Clone()); + + m_pOC = pDict->GetDictBy("OC"); + m_bIsMask = + !pDict->KeyExist("ColorSpace") || pDict->GetIntegerBy("ImageMask"); + m_bInterpolate = !!pDict->GetIntegerBy("Interpolate"); + m_Height = pDict->GetIntegerBy("Height"); + m_Width = pDict->GetIntegerBy("Width"); +} CPDF_Image::~CPDF_Image() { if (m_bInline) { @@ -37,43 +62,18 @@ CPDF_Image::~CPDF_Image() { } } -void CPDF_Image::Release() { - if (m_bInline || (m_pStream && m_pStream->GetObjNum() == 0)) - delete this; -} - CPDF_Image* CPDF_Image::Clone() { if (m_pStream->GetObjNum()) return m_pDocument->GetPageData()->GetImage(m_pStream); - CPDF_Image* pImage = new CPDF_Image(m_pDocument); - pImage->LoadImageF(ToStream(m_pStream->Clone()), m_bInline); + CPDF_Image* pImage = + new CPDF_Image(m_pDocument, ToStream(m_pStream->Clone()), m_bInline); if (m_bInline) pImage->SetInlineDict(ToDictionary(m_pInlineDict->Clone(TRUE))); return pImage; } -FX_BOOL CPDF_Image::LoadImageF(CPDF_Stream* pStream, FX_BOOL bInline) { - m_pStream = pStream; - if (m_bInline && m_pInlineDict) { - m_pInlineDict->Release(); - m_pInlineDict = nullptr; - } - m_bInline = bInline; - CPDF_Dictionary* pDict = pStream->GetDict(); - if (m_bInline) { - m_pInlineDict = ToDictionary(pDict->Clone()); - } - m_pOC = pDict->GetDictBy("OC"); - m_bIsMask = - !pDict->KeyExist("ColorSpace") || pDict->GetIntegerBy("ImageMask"); - m_bInterpolate = pDict->GetIntegerBy("Interpolate"); - m_Height = pDict->GetIntegerBy("Height"); - m_Width = pDict->GetIntegerBy("Width"); - return TRUE; -} - CPDF_Dictionary* CPDF_Image::InitJPEG(uint8_t* pData, uint32_t size) { int32_t width; int32_t height; @@ -120,36 +120,23 @@ CPDF_Dictionary* CPDF_Image::InitJPEG(uint8_t* pData, uint32_t size) { return pDict; } -void CPDF_Image::SetJpegImage(uint8_t* pData, uint32_t size) { - CPDF_Dictionary* pDict = InitJPEG(pData, size); - if (!pDict) { - return; - } - m_pStream->InitStream(pData, size, pDict); -} - void CPDF_Image::SetJpegImage(IFX_FileRead* pFile) { uint32_t size = (uint32_t)pFile->GetSize(); - if (!size) { + if (!size) return; - } - uint32_t dwEstimateSize = size; - if (dwEstimateSize > 8192) { - dwEstimateSize = 8192; - } - uint8_t* pData = FX_Alloc(uint8_t, dwEstimateSize); - pFile->ReadBlock(pData, 0, dwEstimateSize); - CPDF_Dictionary* pDict = InitJPEG(pData, dwEstimateSize); - FX_Free(pData); + + uint32_t dwEstimateSize = std::min(size, 8192U); + std::vector data(dwEstimateSize); + pFile->ReadBlock(data.data(), 0, dwEstimateSize); + CPDF_Dictionary* pDict = InitJPEG(data.data(), dwEstimateSize); if (!pDict && size > dwEstimateSize) { - pData = FX_Alloc(uint8_t, size); - pFile->ReadBlock(pData, 0, size); - pDict = InitJPEG(pData, size); - FX_Free(pData); + data.resize(size); + pFile->ReadBlock(data.data(), 0, size); + pDict = InitJPEG(data.data(), size); } - if (!pDict) { + if (!pDict) return; - } + m_pStream->InitStreamFromFile(pFile, pDict); } diff --git a/core/fpdfapi/fpdf_page/cpdf_imageobject.cpp b/core/fpdfapi/fpdf_page/cpdf_imageobject.cpp index e2defa8054..1fa14805aa 100644 --- a/core/fpdfapi/fpdf_page/cpdf_imageobject.cpp +++ b/core/fpdfapi/fpdf_page/cpdf_imageobject.cpp @@ -6,22 +6,17 @@ #include "core/fpdfapi/fpdf_page/include/cpdf_imageobject.h" +#include + #include "core/fpdfapi/fpdf_page/include/cpdf_image.h" #include "core/fpdfapi/fpdf_page/pageint.h" #include "core/fpdfapi/fpdf_parser/include/cpdf_document.h" -CPDF_ImageObject::CPDF_ImageObject() : m_pImage(nullptr) {} +CPDF_ImageObject::CPDF_ImageObject() + : m_pImage(nullptr), m_pImageOwned(false) {} CPDF_ImageObject::~CPDF_ImageObject() { - if (!m_pImage) { - return; - } - if (m_pImage->IsInline() || - (m_pImage->GetStream() && m_pImage->GetStream()->GetObjNum() == 0)) { - delete m_pImage; - } else { - m_pImage->GetDocument()->GetPageData()->ReleaseImage(m_pImage->GetStream()); - } + Release(); } CPDF_ImageObject* CPDF_ImageObject::Clone() const { @@ -59,3 +54,31 @@ void CPDF_ImageObject::CalcBoundingBox() { m_Right = m_Top = 1.0f; m_Matrix.TransformRect(m_Left, m_Right, m_Top, m_Bottom); } + +void CPDF_ImageObject::SetOwnedImage(std::unique_ptr pImage) { + Release(); + m_pImage = pImage.release(); + m_pImageOwned = true; +} + +void CPDF_ImageObject::SetUnownedImage(CPDF_Image* pImage) { + Release(); + m_pImage = pImage; + m_pImageOwned = false; +} + +void CPDF_ImageObject::Release() { + if (m_pImageOwned) { + delete m_pImage; + m_pImage = nullptr; + m_pImageOwned = false; + return; + } + + if (!m_pImage) + return; + + CPDF_DocPageData* pPageData = m_pImage->GetDocument()->GetPageData(); + pPageData->ReleaseImage(m_pImage->GetStream()); + m_pImage = nullptr; +} diff --git a/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp b/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp index f8d7575318..fa242ffb85 100644 --- a/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp +++ b/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp @@ -406,10 +406,8 @@ CPDF_Image* CPDF_DocPageData::GetImage(CPDF_Object* pImageStream) { if (it != m_ImageMap.end()) return it->second->AddRef(); - CPDF_Image* pImage = new CPDF_Image(m_pPDFDoc); - pImage->LoadImageF(pImageStream->AsStream(), false); - - CPDF_CountedImage* pCountedImage = new CPDF_CountedImage(pImage); + CPDF_CountedImage* pCountedImage = new CPDF_CountedImage( + new CPDF_Image(m_pPDFDoc, pImageStream->AsStream(), false)); m_ImageMap[dwImageObjNum] = pCountedImage; return pCountedImage->AddRef(); } diff --git a/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp b/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp index 483a6b1fd6..991e4b1e9f 100644 --- a/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp +++ b/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp @@ -785,13 +785,13 @@ CPDF_ImageObject* CPDF_StreamContentParser::AddImage(CPDF_Stream* pStream, std::unique_ptr pImageObj(new CPDF_ImageObject); if (pImage) { - pImageObj->m_pImage = - m_pDocument->GetPageData()->GetImage(pImage->GetStream()); + pImageObj->SetUnownedImage( + m_pDocument->GetPageData()->GetImage(pImage->GetStream())); } else if (pStream->GetObjNum()) { - pImageObj->m_pImage = m_pDocument->LoadImageF(pStream); + pImageObj->SetUnownedImage(m_pDocument->LoadImageF(pStream)); } else { - pImageObj->m_pImage = new CPDF_Image(m_pDocument); - pImageObj->m_pImage->LoadImageF(pStream, bInline); + pImageObj->SetOwnedImage( + WrapUnique(new CPDF_Image(m_pDocument, pStream, bInline))); } SetGraphicStates(pImageObj.get(), pImageObj->GetImage()->IsMask(), FALSE, FALSE); diff --git a/core/fpdfapi/fpdf_page/include/cpdf_image.h b/core/fpdfapi/fpdf_page/include/cpdf_image.h index f475e53478..ef48a04f95 100644 --- a/core/fpdfapi/fpdf_page/include/cpdf_image.h +++ b/core/fpdfapi/fpdf_page/include/cpdf_image.h @@ -28,12 +28,9 @@ class IFX_Pause; class CPDF_Image { public: explicit CPDF_Image(CPDF_Document* pDoc); + CPDF_Image(CPDF_Document* pDoc, CPDF_Stream* pStream, bool bInline); ~CPDF_Image(); - FX_BOOL LoadImageF(CPDF_Stream* pImageStream, FX_BOOL bInline); - - void Release(); - CPDF_Image* Clone(); CPDF_Dictionary* GetInlineDict() const { return m_pInlineDict; } @@ -47,9 +44,9 @@ class CPDF_Image { int32_t GetPixelHeight() const { return m_Height; } int32_t GetPixelWidth() const { return m_Width; } - FX_BOOL IsInline() { return m_bInline; } - FX_BOOL IsMask() const { return m_bIsMask; } - FX_BOOL IsInterpol() const { return m_bInterpolate; } + bool IsInline() const { return m_bInline; } + bool IsMask() const { return m_bIsMask; } + bool IsInterpol() const { return m_bInterpolate; } CFX_DIBSource* LoadDIBSource(CFX_DIBSource** ppMask = nullptr, uint32_t* pMatteColor = nullptr, @@ -59,7 +56,6 @@ class CPDF_Image { void SetInlineDict(CPDF_Dictionary* pDict) { m_pInlineDict = pDict; } void SetImage(const CFX_DIBitmap* pDIBitmap, int32_t iCompress); - void SetJpegImage(uint8_t* pImageData, uint32_t size); void SetJpegImage(IFX_FileRead* pFile); void ResetCache(CPDF_Page* pPage, const CFX_DIBitmap* pDIBitmap); @@ -81,13 +77,13 @@ class CPDF_Image { CPDF_Dictionary* InitJPEG(uint8_t* pData, uint32_t size); CPDF_Stream* m_pStream; - FX_BOOL m_bInline; + const bool m_bInline; CPDF_Dictionary* m_pInlineDict; int32_t m_Height; int32_t m_Width; - FX_BOOL m_bIsMask; - FX_BOOL m_bInterpolate; - CPDF_Document* m_pDocument; + bool m_bIsMask; + bool m_bInterpolate; + CPDF_Document* const m_pDocument; CPDF_Dictionary* m_pOC; }; diff --git a/core/fpdfapi/fpdf_page/include/cpdf_imageobject.h b/core/fpdfapi/fpdf_page/include/cpdf_imageobject.h index b2bffa707c..fdbd43c4a9 100644 --- a/core/fpdfapi/fpdf_page/include/cpdf_imageobject.h +++ b/core/fpdfapi/fpdf_page/include/cpdf_imageobject.h @@ -7,6 +7,8 @@ #ifndef CORE_FPDFAPI_FPDF_PAGE_INCLUDE_CPDF_IMAGEOBJECT_H_ #define CORE_FPDFAPI_FPDF_PAGE_INCLUDE_CPDF_IMAGEOBJECT_H_ +#include + #include "core/fpdfapi/fpdf_page/include/cpdf_pageobject.h" #include "core/fxcrt/include/fx_coordinates.h" @@ -27,9 +29,16 @@ class CPDF_ImageObject : public CPDF_PageObject { void CalcBoundingBox(); CPDF_Image* GetImage() const { return m_pImage; } + void SetOwnedImage(std::unique_ptr pImage); + void SetUnownedImage(CPDF_Image* pImage); - CPDF_Image* m_pImage; CFX_Matrix m_Matrix; + + private: + void Release(); + + CPDF_Image* m_pImage; + bool m_pImageOwned; }; #endif // CORE_FPDFAPI_FPDF_PAGE_INCLUDE_CPDF_IMAGEOBJECT_H_ diff --git a/fpdfsdk/fpdfeditimg.cpp b/fpdfsdk/fpdfeditimg.cpp index cd28301977..da3d33eb05 100644 --- a/fpdfsdk/fpdfeditimg.cpp +++ b/fpdfsdk/fpdfeditimg.cpp @@ -19,7 +19,7 @@ FPDFPageObj_NewImgeObj(FPDF_DOCUMENT document) { return nullptr; CPDF_ImageObject* pImageObj = new CPDF_ImageObject; - pImageObj->m_pImage = new CPDF_Image(pDoc); + pImageObj->SetOwnedImage(WrapUnique(new CPDF_Image(pDoc))); return pImageObj; } diff --git a/fpdfsdk/fpdfeditimg_unittest.cpp b/fpdfsdk/fpdfeditimg_unittest.cpp new file mode 100644 index 0000000000..abe0ffe25c --- /dev/null +++ b/fpdfsdk/fpdfeditimg_unittest.cpp @@ -0,0 +1,52 @@ +// Copyright 2016 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 "public/fpdf_edit.h" + +#include "core/fpdfapi/include/cpdf_modulemgr.h" +#include "testing/gtest/include/gtest/gtest.h" + +class PDFEditTest : public testing::Test { + void SetUp() override { + CPDF_ModuleMgr* module_mgr = CPDF_ModuleMgr::Get(); + module_mgr->InitPageModule(); + } + + void TearDown() override { CPDF_ModuleMgr::Destroy(); } +}; + +TEST_F(PDFEditTest, InsertObjectWithInvalidPage) { + FPDF_DOCUMENT doc = FPDF_CreateNewDocument(); + FPDF_PAGE page = FPDFPage_New(doc, 0, 100, 100); + EXPECT_EQ(0, FPDFPage_CountObject(page)); + + FPDFPage_InsertObject(nullptr, nullptr); + EXPECT_EQ(0, FPDFPage_CountObject(page)); + + FPDFPage_InsertObject(page, nullptr); + EXPECT_EQ(0, FPDFPage_CountObject(page)); + + FPDF_PAGEOBJECT page_image = FPDFPageObj_NewImgeObj(doc); + FPDFPage_InsertObject(nullptr, page_image); + EXPECT_EQ(0, FPDFPage_CountObject(page)); + + FPDF_ClosePage(page); + FPDF_CloseDocument(doc); +} + +// https://bugs.chromium.org/p/pdfium/issues/detail?id=545 +// TODO(thestig): Add another test case that also calls +// FPDFPage_GenerateContent(). +TEST_F(PDFEditTest, NewImgeObj) { + FPDF_DOCUMENT doc = FPDF_CreateNewDocument(); + FPDF_PAGE page = FPDFPage_New(doc, 0, 100, 100); + EXPECT_EQ(0, FPDFPage_CountObject(page)); + + FPDF_PAGEOBJECT page_image = FPDFPageObj_NewImgeObj(doc); + FPDFPage_InsertObject(page, page_image); + EXPECT_EQ(1, FPDFPage_CountObject(page)); + + FPDF_ClosePage(page); + FPDF_CloseDocument(doc); +} diff --git a/pdfium.gyp b/pdfium.gyp index 584de12a7b..9b9e8cac40 100644 --- a/pdfium.gyp +++ b/pdfium.gyp @@ -944,6 +944,7 @@ 'core/fxcrt/fx_extension_unittest.cpp', 'core/fxcrt/fx_system_unittest.cpp', 'fpdfsdk/fpdfdoc_unittest.cpp', + 'fpdfsdk/fpdfeditimg_unittest.cpp', 'testing/fx_string_testhelpers.h', 'testing/fx_string_testhelpers.cpp', ], -- cgit v1.2.3