Do not clone in CPDF_ImageCacheEntry::GetCachedBitmap

core/fpdfapi/render/fpdf_render_cache.cpp

 // Copyright 2014 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.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "core/fpdfapi/render/cpdf_pagerendercache.h"
 
 #include "core/fpdfapi/page/cpdf_page.h"
 #include "core/fpdfapi/page/pageint.h"
@@ skipping 165 matching lines @@
       m_pCurBitmap(nullptr),
       m_pCurMask(nullptr),
       m_MatteColor(0),
       m_pRenderStatus(nullptr),
       m_pDocument(pDoc),
       m_pStream(pStream),
       m_pCachedBitmap(nullptr),
       m_pCachedMask(nullptr),
       m_dwCacheSize(0) {}
 CPDF_ImageCacheEntry::~CPDF_ImageCacheEntry() {
   delete m_pCachedBitmap;

[Tom Sepez, 2016/11/29 17:00:53]

Can we make these two unique_ptr's first? 
Similarly, we should make any method called "clone" return an unique_ptr.

Then it's easier to reason about whether we're passing ownership or not.  I
suspect the clone()s may have been used to prevent this from freeing things it
didn't own.

[npm, 2016/11/29 17:05:31]

Sure I can do that. But in this particular case, you can see the cloned objects
are deleted right about cloning. So that's not the reason for the cloning.

[npm, 2016/11/29 17:06:27]

s/about/after

   delete m_pCachedMask;
 }
 void CPDF_ImageCacheEntry::Reset(const CFX_DIBitmap* pBitmap) {
   delete m_pCachedBitmap;
   m_pCachedBitmap = nullptr;
   if (pBitmap) {
     m_pCachedBitmap = pBitmap->Clone();
   }
   CalcSize();
 }
@@ skipping 29 matching lines @@
   CPDF_DIBSource* pSrc = new CPDF_DIBSource;
   CPDF_DIBSource* pMaskSrc = nullptr;
   if (!pSrc->Load(m_pDocument, m_pStream, &pMaskSrc, &MatteColor,
                   pRenderStatus->m_pFormResource, pPageResources, bStdCS,
                   GroupFamily, bLoadMask)) {
     delete pSrc;
     pBitmap = nullptr;
     return false;
   }
   m_MatteColor = MatteColor;
-  if (pSrc->GetPitch() * pSrc->GetHeight() < FPDF_HUGE_IMAGE_SIZE) {
-    m_pCachedBitmap = pSrc->Clone();
-    delete pSrc;
-  } else {
-    m_pCachedBitmap = pSrc;
-  }
-  if (pMaskSrc) {
-    m_pCachedMask = pMaskSrc->Clone();
-    delete pMaskSrc;
-  }
+  m_pCachedBitmap = pSrc;
+  if (pMaskSrc)
+    m_pCachedMask = pMaskSrc;
 
   pBitmap = m_pCachedBitmap;
   pMask = m_pCachedMask;
   CalcSize();
   return false;
 }
+
 CFX_DIBSource* CPDF_ImageCacheEntry::DetachBitmap() {
   CFX_DIBSource* pDIBSource = m_pCurBitmap;
   m_pCurBitmap = nullptr;
   return pDIBSource;
 }
 CFX_DIBSource* CPDF_ImageCacheEntry::DetachMask() {
   CFX_DIBSource* pDIBSource = m_pCurMask;
   m_pCurMask = nullptr;
   return pDIBSource;
 }
@@ skipping 55 matching lines @@
     m_pCurBitmap = nullptr;
     return 0;
   }
   ContinueGetCachedBitmap();
   return 0;
 }
 void CPDF_ImageCacheEntry::CalcSize() {
   m_dwCacheSize = FPDF_ImageCache_EstimateImageSize(m_pCachedBitmap) +
                   FPDF_ImageCache_EstimateImageSize(m_pCachedMask);
 }