Fix a leak with FPDFPageObj_NewImgeObj().

BUG=pdfium:545

Review-Url: https://codereview.chromium.org/2194393002

11 files changed, 156 insertions, 89 deletions

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 <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 @@ 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<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 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 <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 @@ 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<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 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<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 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 <memory>
+
 #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<CPDF_Image> 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',
       ],