Fix a leak in CPDF_Type3Font::LoadChar().

core/src/fpdfapi/fpdf_font/fpdf_font.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 "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fpdfapi/fpdf_page.h"
 #include "../../../include/fpdfapi/fpdf_pageobj.h"
 #include "../../../include/fpdfapi/fpdf_resource.h"
@@ skipping 1625 matching lines @@
       }
     }
     if (bGotOne) {
       return;
     }
   }
   for (int charcode = 0; charcode < 256; charcode++) {
     m_GlyphIndex[charcode] = charcode;
   }
 }
-CPDF_Type3Font::CPDF_Type3Font() : CPDF_SimpleFont(PDFFONT_TYPE3) {
-  m_pPageResources = NULL;
-  FXSYS_memset(m_CharWidthL, 0, sizeof m_CharWidthL);
+
+CPDF_Type3Font::CPDF_Type3Font()
+    : CPDF_SimpleFont(PDFFONT_TYPE3),
+      m_pCharProcs(nullptr),
+      m_pPageResources(nullptr),
+      m_pFontResources(nullptr) {
+  FXSYS_memset(m_CharWidthL, 0, sizeof(m_CharWidthL));
 }
+
 CPDF_Type3Font::~CPDF_Type3Font() {
-  FX_POSITION pos = m_CacheMap.GetStartPosition();
-  while (pos) {
-    void* key;
-    void* value;
-    m_CacheMap.GetNextAssoc(pos, key, value);
-    delete (CPDF_Type3Char*)value;
-  }
-  m_CacheMap.RemoveAll();
-  pos = m_DeletedMap.GetStartPosition();
-  while (pos) {
-    void* key;
-    void* value;
-    m_DeletedMap.GetNextAssoc(pos, key, value);
-    delete (CPDF_Type3Char*)key;
-  }
+  for (auto it : m_CacheMap)
+    delete it.second;
 }
+
 FX_BOOL CPDF_Type3Font::_Load() {
   m_pFontResources = m_pFontDict->GetDict(FX_BSTRC("Resources"));
   CPDF_Array* pMatrix = m_pFontDict->GetArray(FX_BSTRC("FontMatrix"));
   FX_FLOAT xscale = 1.0f, yscale = 1.0f;
   if (pMatrix) {
     m_FontMatrix = pMatrix->GetMatrix();
     xscale = m_FontMatrix.a;
     yscale = m_FontMatrix.d;
   }
   CPDF_Array* pBBox = m_pFontDict->GetArray(FX_BSTRC("FontBBox"));
@@ skipping 30 matching lines @@
           m_Encoding.m_Unicodes[i] = i;
         }
       }
     }
   }
   return TRUE;
 }
 void CPDF_Type3Font::CheckType3FontMetrics() {
   CheckFontMetrics();
 }
+
 CPDF_Type3Char* CPDF_Type3Font::LoadChar(FX_DWORD charcode, int level) {
   if (level >= _FPDF_MAX_TYPE3_FORM_LEVEL_)
     return nullptr;
 
-  CPDF_Type3Char* pChar = nullptr;
-  if (m_CacheMap.Lookup((void*)(uintptr_t)charcode, (void*&)pChar)) {
-    if (pChar->m_bPageRequired && m_pPageResources) {
-      delete pChar;
-      m_CacheMap.RemoveKey((void*)(uintptr_t)charcode);
-      return LoadChar(charcode, level + 1);
+  {

[Tom Sepez, 2015/11/08 04:46:29]

Why { here?  Can't you just re-assign this at 1748?

[Lei Zhang, 2015/11/09 20:27:26]

They are unrelated so I didn't want to reuse the same variable. Let me see if I
can remove some more dead code and make this go away.

+    auto it = m_CacheMap.find(charcode);
+    if (it != m_CacheMap.end()) {
+      CPDF_Type3Char* pCachedChar = it->second;
+      if (pCachedChar->m_bPageRequired && m_pPageResources) {
+        delete pCachedChar;
+        m_CacheMap.erase(it);
+        return LoadChar(charcode, level + 1);
+      }
+      return pCachedChar;
     }
-    return pChar;
   }
+
   const FX_CHAR* name =
       GetAdobeCharName(m_BaseEncoding, m_pCharNames, charcode);
   if (!name)
     return nullptr;
 
   CPDF_Stream* pStream =
       ToStream(m_pCharProcs ? m_pCharProcs->GetElementValue(name) : nullptr);
   if (!pStream)
     return nullptr;
 
-  pChar = new CPDF_Type3Char;
-  pChar->m_pForm = new CPDF_Form(
+  nonstd::unique_ptr<CPDF_Type3Char> pNewChar(new CPDF_Type3Char);
+  pNewChar->m_pForm = new CPDF_Form(

[Tom Sepez, 2015/11/08 04:46:29]

Pity the constructor doesn't set m_pForm.

[Lei Zhang, 2015/11/09 20:27:26]

Done.

       m_pDocument, m_pFontResources ? m_pFontResources : m_pPageResources,
       pStream, nullptr);
-  pChar->m_pForm->ParseContent(nullptr, nullptr, pChar, nullptr, level + 1);
+
+  // This can trigger recursion into this method. The content of |m_CacheMap|
+  // can change as a result. Thus after it returns, check the cache again for
+  // a cache hit.
+  pNewChar->m_pForm->ParseContent(nullptr, nullptr, pNewChar.get(), nullptr,
+                                  level + 1);
+  auto it = m_CacheMap.find(charcode);
+  if (it != m_CacheMap.end())
+    return it->second;
+
   FX_FLOAT scale = m_FontMatrix.GetXUnit();
-  pChar->m_Width = (int32_t)(pChar->m_Width * scale + 0.5f);
-  FX_RECT& rcBBox = pChar->m_BBox;
+  pNewChar->m_Width = (int32_t)(pNewChar->m_Width * scale + 0.5f);
+  FX_RECT& rcBBox = pNewChar->m_BBox;
   CFX_FloatRect char_rect(
       (FX_FLOAT)rcBBox.left / 1000.0f, (FX_FLOAT)rcBBox.bottom / 1000.0f,
       (FX_FLOAT)rcBBox.right / 1000.0f, (FX_FLOAT)rcBBox.top / 1000.0f);
-  if (rcBBox.right <= rcBBox.left || rcBBox.bottom >= rcBBox.top) {
-    char_rect = pChar->m_pForm->CalcBoundingBox();
-  }
+  if (rcBBox.right <= rcBBox.left || rcBBox.bottom >= rcBBox.top)
+    char_rect = pNewChar->m_pForm->CalcBoundingBox();
+
   char_rect.Transform(&m_FontMatrix);
   rcBBox.left = FXSYS_round(char_rect.left * 1000);
   rcBBox.right = FXSYS_round(char_rect.right * 1000);
   rcBBox.top = FXSYS_round(char_rect.top * 1000);
   rcBBox.bottom = FXSYS_round(char_rect.bottom * 1000);
-  m_CacheMap.SetAt((void*)(uintptr_t)charcode, pChar);
-  if (pChar->m_pForm->CountObjects() == 0) {
-    delete pChar->m_pForm;
-    pChar->m_pForm = nullptr;
+
+  FXSYS_assert(m_CacheMap.find(charcode) == m_CacheMap.end());
+  CPDF_Type3Char* pCachedChar = pNewChar.release();
+  m_CacheMap[charcode] = pCachedChar;
+  if (pCachedChar->m_pForm->CountObjects() == 0) {
+    delete pCachedChar->m_pForm;
+    pCachedChar->m_pForm = nullptr;
   }
-  return pChar;
+  return pCachedChar;
 }
+
 int CPDF_Type3Font::GetCharWidthF(FX_DWORD charcode, int level) {
-  if (charcode > 0xff) {
+  if (charcode > FX_ArraySize(m_CharWidthL) - 1)

[Tom Sepez, 2015/11/08 04:46:29]

or just >= and lose the -1.

[Lei Zhang, 2015/11/09 20:27:26]

Done.

     charcode = 0;
-  }
-  if (m_CharWidthL[charcode]) {
+
+  if (m_CharWidthL[charcode])
     return m_CharWidthL[charcode];
-  }
-  CPDF_Type3Char* pChar = LoadChar(charcode, level);
-  if (pChar == NULL) {
-    return 0;
-  }
-  return pChar->m_Width;
+
+  const CPDF_Type3Char* pChar = LoadChar(charcode, level);
+  return pChar ? pChar->m_Width : 0;
 }
+
 void CPDF_Type3Font::GetCharBBox(FX_DWORD charcode, FX_RECT& rect, int level) {
-  CPDF_Type3Char* pChar = LoadChar(charcode, level);
-  if (pChar == NULL) {
+  const CPDF_Type3Char* pChar = LoadChar(charcode, level);
+  if (!pChar) {
     rect.left = rect.right = rect.top = rect.bottom = 0;

[Tom Sepez, 2015/11/08 04:46:29]

nit: one per line.

[Lei Zhang, 2015/11/09 20:27:26]

Done.

     return;
   }
   rect = pChar->m_BBox;
 }
+
 CPDF_Type3Char::CPDF_Type3Char() {
   m_pForm = NULL;
   m_pBitmap = NULL;
   m_bPageRequired = FALSE;
   m_bColored = FALSE;
 }
 CPDF_Type3Char::~CPDF_Type3Char() {
   delete m_pForm;
   delete m_pBitmap;
 }