Fix a leak with FPDFPageObj_NewImgeObj().
BUG=pdfium:545
Review-Url: https://codereview.chromium.org/2194393002
diff --git a/BUILD.gn b/BUILD.gn
index 4b8f222..87ee80f 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -1569,6 +1569,7 @@
"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 a85591c..0e4bab1 100644
--- a/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp
+++ b/core/fpdfapi/fpdf_edit/cpdf_pagecontentgenerator.cpp
@@ -99,12 +99,12 @@
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 63e006f..af361eb 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 <algorithm>
+#include <memory>
+#include <vector>
+
#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 @@
}
}
-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 @@
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<uint8_t> 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 e2defa8..1fa1480 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 <memory>
+
#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 @@
m_Right = m_Top = 1.0f;
m_Matrix.TransformRect(m_Left, m_Right, m_Top, m_Bottom);
}
+
+void CPDF_ImageObject::SetOwnedImage(std::unique_ptr<CPDF_Image> 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 f8d7575..fa242ff 100644
--- a/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp
+++ b/core/fpdfapi/fpdf_page/fpdf_page_doc.cpp
@@ -406,10 +406,8 @@
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 483a6b1..991e4b1 100644
--- a/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp
+++ b/core/fpdfapi/fpdf_page/fpdf_page_parser.cpp
@@ -785,13 +785,13 @@
std::unique_ptr<CPDF_ImageObject> 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 f475e53..ef48a04 100644
--- a/core/fpdfapi/fpdf_page/include/cpdf_image.h
+++ b/core/fpdfapi/fpdf_page/include/cpdf_image.h
@@ -28,12 +28,9 @@
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 @@
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 @@
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 @@
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 b2bffa7..fdbd43c 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 <memory>
+
#include "core/fpdfapi/fpdf_page/include/cpdf_pageobject.h"
#include "core/fxcrt/include/fx_coordinates.h"
@@ -27,9 +29,16 @@
void CalcBoundingBox();
CPDF_Image* GetImage() const { return m_pImage; }
+ void SetOwnedImage(std::unique_ptr<CPDF_Image> pImage);
+ void SetUnownedImage(CPDF_Image* pImage);
+
+ CFX_Matrix m_Matrix;
+
+ private:
+ void Release();
CPDF_Image* m_pImage;
- CFX_Matrix m_Matrix;
+ bool m_pImageOwned;
};
#endif // CORE_FPDFAPI_FPDF_PAGE_INCLUDE_CPDF_IMAGEOBJECT_H_
diff --git a/fpdfsdk/fpdfeditimg.cpp b/fpdfsdk/fpdfeditimg.cpp
index cd28301..da3d33e 100644
--- a/fpdfsdk/fpdfeditimg.cpp
+++ b/fpdfsdk/fpdfeditimg.cpp
@@ -19,7 +19,7 @@
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 0000000..abe0ffe
--- /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 584de12..9b9e8ca 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',
],
