Fix potentially massive memory leak in CPDF_DIBSource::LoadJpxBitmap().

core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.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 "../../../../third_party/base/nonstd_unique_ptr.h"
 #include "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fpdfapi/fpdf_pageobj.h"
 #include "../../../include/fpdfapi/fpdf_render.h"
 #include "../../../include/fxcodec/fx_codec.h"
 #include "../../../include/fxcrt/fx_safe_types.h"
 #include "../../../include/fxge/fx_ge.h"
 #include "../fpdf_page/pageint.h"
 #include "render_int.h"
 
 namespace {
@@ skipping 31 matching lines @@
 
 FX_SAFE_DWORD CalculatePitch32(int bpp, int width)
 {
     FX_SAFE_DWORD pitch = bpp;
     pitch *= width;
     pitch += 31;
     pitch /= 8;
     return pitch;
 }
 
+// Wrapper class to hold objects allocated in CPDF_DIBSource::LoadJpxBitmap(),
+// because nonstd::unique_ptr does not support custom deleters yet.
+class JpxBitMapContext
+{
+  public:
+    explicit JpxBitMapContext(ICodec_JpxModule* jpx_module)
+        : jpx_module_(jpx_module),
+          ctx_(nullptr),
+          output_offsets_(nullptr) {}
+
+    ~JpxBitMapContext() {
+        FX_Free(output_offsets_);
+        jpx_module_->DestroyDecoder(ctx_);
+    }
+
+    // Takes ownership of |ctx|.
+    void set_context(void* ctx) {
+        ctx_ = ctx;
+    }
+
+    void* context() {
+        return ctx_;
+    }
+
+    // Takes ownership of |output_offsets|.
+    void set_output_offsets(FX_LPBYTE output_offsets) {

[Tom Sepez, 2015/06/06 19:05:04]

nit: prefer void* to LPBYTE in new code.

[Lei Zhang, 2015/06/08 19:35:20]

LPBYTE is unsigned char*.

+      output_offsets_ = output_offsets;
+    }
+
+    FX_LPBYTE output_offsets() {
+      return output_offsets_;
+    }
+
+  private:
+    ICodec_JpxModule* jpx_module_;  // Weak pointer.
+    void* ctx_;  // Decoder context, owned.
+    FX_LPBYTE output_offsets_;  // Output offets for decoding, owned.
+
+    // Disallow evil constructors
+    JpxBitMapContext(const JpxBitMapContext&);
+    void operator=(const JpxBitMapContext&);
+};
+
 }  // namespace
 
 CFX_DIBSource* CPDF_Image::LoadDIBSource(CFX_DIBSource** ppMask, FX_DWORD* pMatteColor, FX_BOOL bStdCS, FX_DWORD GroupFamily, FX_BOOL bLoadMask) const
 {
     CPDF_DIBSource* pSource = new CPDF_DIBSource;
     if (pSource->Load(m_pDocument, m_pStream, (CPDF_DIBSource**)ppMask, pMatteColor, NULL, NULL, bStdCS, GroupFamily, bLoadMask)) {
         return pSource;
     }
     delete pSource;
     return NULL;
@@ skipping 554 matching lines @@
         return 0;
     }
     if (provided_pitch.ValueOrDie() < requested_pitch.ValueOrDie()) {
         return 0;
     }
     return 1;
 }
 void CPDF_DIBSource::LoadJpxBitmap()
 {
     ICodec_JpxModule* pJpxModule = CPDF_ModuleMgr::Get()->GetJpxModule();
-    if (pJpxModule == NULL) {
+    if (!pJpxModule)
         return;
-    }
-    FX_LPVOID ctx = pJpxModule->CreateDecoder(m_pStreamAcc->GetData(), m_pStreamAcc->GetSize(), m_pColorSpace != NULL);
-    if (ctx == NULL) {
+
+    nonstd::unique_ptr<JpxBitMapContext> context(
+        new JpxBitMapContext(pJpxModule));
+    context->set_context(pJpxModule->CreateDecoder(m_pStreamAcc->GetData(),
+                                                   m_pStreamAcc->GetSize(),
+                                                   m_pColorSpace != nullptr));
+    if (!context->context())
         return;
-    }
-    FX_DWORD width = 0, height = 0, codestream_nComps = 0, image_nComps = 0;
-    pJpxModule->GetImageInfo(ctx, width, height, codestream_nComps, image_nComps);
-    if ((int)width < m_Width || (int)height < m_Height) {
-        pJpxModule->DestroyDecoder(ctx);
+
+    FX_DWORD width = 0;
+    FX_DWORD height = 0;
+    FX_DWORD codestream_nComps = 0;
+    FX_DWORD image_nComps = 0;
+    pJpxModule->GetImageInfo(context->context(), width, height,
+                             codestream_nComps, image_nComps);
+    if ((int)width < m_Width || (int)height < m_Height)
         return;
-    }
+
     int output_nComps;
-    FX_BOOL bTranslateColor, bSwapRGB = FALSE;
+    FX_BOOL bTranslateColor;
+    FX_BOOL bSwapRGB = FALSE;
     if (m_pColorSpace) {
-        if (codestream_nComps != (FX_DWORD)m_pColorSpace->CountComponents()) {
+        if (codestream_nComps != (FX_DWORD)m_pColorSpace->CountComponents())
             return;
-        }
         output_nComps = codestream_nComps;
         bTranslateColor = FALSE;
         if (m_pColorSpace == CPDF_ColorSpace::GetStockCS(PDFCS_DEVICERGB)) {
             bSwapRGB = TRUE;
-            m_pColorSpace = NULL;
+            m_pColorSpace = nullptr;
         }
     } else {
         bTranslateColor = TRUE;
         if (image_nComps) {
             output_nComps = image_nComps;
         } else {
             output_nComps = codestream_nComps;
         }
         if (output_nComps == 3) {
             bSwapRGB = TRUE;
@@ skipping 10 matching lines @@
         format = FXDIB_Rgb;
     } else if (output_nComps == 4) {
         format = FXDIB_Rgb32;
     } else {
         width = (width * output_nComps + 2) / 3;
         format = FXDIB_Rgb;
     }
     m_pCachedBitmap = new CFX_DIBitmap;
     if (!m_pCachedBitmap->Create(width, height, format)) {
         delete m_pCachedBitmap;
-        m_pCachedBitmap = NULL;
+        m_pCachedBitmap = nullptr;
         return;
     }
     m_pCachedBitmap->Clear(0xFFFFFFFF);
-    FX_LPBYTE output_offsets = FX_Alloc(FX_BYTE, output_nComps);
-    for (int i = 0; i < output_nComps; i ++) {
-        output_offsets[i] = i;
+    context->set_output_offsets(FX_Alloc(FX_BYTE, output_nComps));
+    for (int i = 0; i < output_nComps; ++i)
+        context->output_offsets()[i] = i;
+    if (bSwapRGB) {
+        context->output_offsets()[0] = 2;
+        context->output_offsets()[2] = 0;
     }
-    if (bSwapRGB) {
-        output_offsets[0] = 2;
-        output_offsets[2] = 0;
-    }
-    if (!pJpxModule->Decode(ctx, m_pCachedBitmap->GetBuffer(), m_pCachedBitmap->GetPitch(), bTranslateColor, output_offsets)) {
+    if (!pJpxModule->Decode(context->context(),
+                            m_pCachedBitmap->GetBuffer(),
+                            m_pCachedBitmap->GetPitch(),
+                            bTranslateColor,
+                            context->output_offsets())) {
         delete m_pCachedBitmap;

[Tom Sepez, 2015/06/06 19:05:04]

nit: can m_pCachedBitmap be a unique_ptr?

[Lei Zhang, 2015/06/08 19:35:20]

Done.

-        m_pCachedBitmap = NULL;
+        m_pCachedBitmap = nullptr;
         return;

[Tom Sepez, 2015/06/06 19:05:04]

Nit: presumably this is the return that leaks, right? maybe mention this in the
CL description ... "leak when JpxModule::Decode() fails" or some such.

[Lei Zhang, 2015/06/08 19:35:20]

The failures are on line 700 above. "if (codestream_nComps !=
(FX_DWORD)m_pColorSpace->CountComponents())"

     }
-    FX_Free(output_offsets);
-    pJpxModule->DestroyDecoder(ctx);
-    if (m_pColorSpace && m_pColorSpace->GetFamily() == PDFCS_INDEXED && m_bpc < 8) {
+    if (m_pColorSpace &&
+        m_pColorSpace->GetFamily() == PDFCS_INDEXED &&
+        m_bpc < 8) {
         int scale = 8 - m_bpc;
-        for (FX_DWORD row = 0; row < height; row ++) {
+        for (FX_DWORD row = 0; row < height; ++row) {
             FX_LPBYTE scanline = (FX_LPBYTE)m_pCachedBitmap->GetScanline(row);
-            for (FX_DWORD col = 0; col < width; col ++) {
+            for (FX_DWORD col = 0; col < width; ++col) {
                 *scanline = (*scanline) >> scale;
-                scanline++;
+                ++scanline;
             }
         }
     }
     m_bpc = 8;
 }
 CPDF_DIBSource* CPDF_DIBSource::LoadMask(FX_DWORD& MatteColor)
 {
     MatteColor = 0xffffffff;
     CPDF_Stream* pSoftMask = m_pDict->GetStream(FX_BSTRC("SMask"));
     if (pSoftMask) {
@@ skipping 782 matching lines @@
 {
     return ((CPDF_ProgressiveImageLoaderHandle*)LoadHandle)->Continue(pPause);
 }
 CPDF_ImageLoader::~CPDF_ImageLoader()
 {
     if (!m_bCached) {
         delete m_pBitmap;
         delete m_pMask;
     }
 }