Add GetValidBpc() check in CPDF_DIBSource::CreateDecoder()

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 "../../../include/fxge/fx_ge.h"
 #include "../../../include/fxcodec/fx_codec.h"
 #include "../../../include/fpdfapi/fpdf_module.h"
 #include "../../../include/fpdfapi/fpdf_render.h"
@@ skipping 540 matching lines @@
                 }
             m_bColorKey = TRUE;
         }
     }
     return TRUE;
 }
 ICodec_ScanlineDecoder* FPDFAPI_CreateFaxDecoder(FX_LPCBYTE src_buf, FX_DWORD src_size, int width, int height,
         const CPDF_Dictionary* pParams);
 ICodec_ScanlineDecoder* FPDFAPI_CreateFlateDecoder(FX_LPCBYTE src_buf, FX_DWORD src_size, int width, int height,
         int nComps, int bpc, const CPDF_Dictionary* pParams);
 int CPDF_DIBSource::CreateDecoder()

[Tom Sepez, 2014/08/25 17:20:11]

Just curious, what does the return value from this function mean? success vs.
failure? Would this be happier as an FX_BOOL, returning TRUE/FALSE rather than
0/1?

[Bo Xu, 2014/08/25 18:16:43]

I am not entirely sure for now. This function can return 0, 1 or 2. When decoder
== "JBIG2Decode", it's possible to return 2. From callers of this function, it
seems 1 and 2 means successful. One such caller is at line 345. I can check with
the devs to confirm.

I also change some of the return value of 1 in my previous patch to 0.

 {
+    FX_DWORD bpc = GetValidBpc();

[Tom Sepez, 2014/08/25 17:20:11]

Do we need to check for the bpc == 0 case and return an error?
Also, we can move this call down below the decoder.IsEmpty() call to avoid doing
any work when this short-ciruits?

[Bo Xu, 2014/08/25 18:16:43]

Done.

     const CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
     if (decoder.IsEmpty()) {
         return 1;
     }
     FX_LPCBYTE src_data = m_pStreamAcc->GetData();
     FX_DWORD src_size = m_pStreamAcc->GetSize();
     const CPDF_Dictionary* pParams = m_pStreamAcc->GetImageParam();
     if (decoder == FX_BSTRC("CCITTFaxDecode")) {
         m_pDecoder = FPDFAPI_CreateFaxDecoder(src_data, src_size, m_Width, m_Height, pParams);
     } else if (decoder == FX_BSTRC("DCTDecode")) {
         m_pDecoder = CPDF_ModuleMgr::Get()->GetJpegModule()->CreateDecoder(src_data, src_size, m_Width, m_Height,
                      m_nComponents, pParams ? pParams->GetInteger(FX_BSTR("ColorTransform"), 1) : 1);
         if (NULL == m_pDecoder) {
             FX_BOOL bTransform = FALSE;
-            int comps, bpc;
+            int comps;
             ICodec_JpegModule* pJpegModule = CPDF_ModuleMgr::Get()->GetJpegModule();
-            if (pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps, bpc, bTransform)) {
+            if (pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps, (FX_INT32&)bpc, bTransform)) {

[Tom Sepez, 2014/08/25 17:20:11]

This cast looks suspicious. Maybe bpc should be an FX_INT32? Alternatively,
maybe LoadInfo() changes to take an FX_DWORD reference? Casting
pointers/reference types like this can be a source of subtle bugs (when the
underlying sizes don't match, etc.).

If you absolutely can't get around it, you'll need to make a temporary
  FX_INT32 bpc_as_int = bpc;
  sts =  ...( ..., bpc_as_int, ...)
  bpc = bpc_as_int;
To be strictly legal.

[Bo Xu, 2014/08/25 18:16:43]

Now I use the temporal bpc to get around this.

                 m_nComponents = comps;
                 m_bpc = bpc;
                 m_pDecoder = CPDF_ModuleMgr::Get()->GetJpegModule()->CreateDecoder(src_data, src_size, m_Width, m_Height,
                              m_nComponents, bTransform);
             }
         }
     } else if (decoder == FX_BSTRC("FlateDecode")) {
-        m_pDecoder = FPDFAPI_CreateFlateDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, m_bpc, pParams);
+        m_pDecoder = FPDFAPI_CreateFlateDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, bpc, pParams);
     } else if (decoder == FX_BSTRC("JPXDecode")) {
         LoadJpxBitmap();
         return m_pCachedBitmap != NULL ? 1 : 0;
     } else if (decoder == FX_BSTRC("JBIG2Decode")) {
         m_pCachedBitmap = FX_NEW CFX_DIBitmap;
         if (!m_pCachedBitmap->Create(m_Width, m_Height, m_bImageMask ? FXDIB_1bppMask : FXDIB_1bppRgb)) {
             delete m_pCachedBitmap;
             m_pCachedBitmap = NULL;
             return 0;
         }
         m_Status = 1;
         return 2;
     } else if (decoder == FX_BSTRC("RunLengthDecode")) {
         m_pDecoder = CPDF_ModuleMgr::Get()->GetCodecModule()->GetBasicModule()->CreateRunLengthDecoder(src_data, src_size, m_Width, m_Height, m_nComponents, m_bpc);
     }
     if (m_pDecoder) {
-        int requested_pitch = (m_Width * m_nComponents * m_bpc + 7) / 8;
-        int provided_pitch = (m_pDecoder->GetWidth() * m_pDecoder->CountComps() * m_pDecoder->GetBPC() + 7) / 8;
-        if (provided_pitch < requested_pitch) {
+        FX_SAFE_DWORD requested_pitch = bpc;
+        requested_pitch *= m_nComponents;
+        requested_pitch *= m_Width;
+        requested_pitch += 7;
+        requested_pitch /= 8;
+        if (!requested_pitch.IsValid()) {
+            return 1;
+        }
+        FX_SAFE_DWORD provided_pitch = m_pDecoder->GetBPC();
+        provided_pitch *= m_pDecoder->CountComps();
+        provided_pitch *= m_pDecoder->GetWidth();
+        provided_pitch += 7;
+        provided_pitch /= 8;
+        if (!provided_pitch.IsValid()) {
+            return 1;
+        }
+        if (provided_pitch.ValueOrDie() < requested_pitch.ValueOrDie()) {
             return 0;
         }
         return 1;
     }
     return 0;
 }
 void CPDF_DIBSource::LoadJpxBitmap()
 {
     ICodec_JpxModule* pJpxModule = CPDF_ModuleMgr::Get()->GetJpxModule();
     if (pJpxModule == NULL) {
@@ skipping 909 matching lines @@
     if (!m_bCached) {
         if (m_pBitmap) {
             delete m_pBitmap;
             m_pBitmap = NULL;
         }
         if (m_pMask) {
             delete m_pMask;
         }
     }
 }