Fix JPX image rendering that regressed due to several security fixes.

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 296 matching lines @@
     m_Width = m_pDict->GetInteger(FX_BSTRC("Width"));
     m_Height = m_pDict->GetInteger(FX_BSTRC("Height"));
     if (m_Width <= 0 || m_Height <= 0 || m_Width > 0x01ffff || m_Height > 0x01ffff) {
         return 0;
     }
     m_GroupFamily = GroupFamily;
     m_bLoadMask = bLoadMask;
     if (!LoadColorInfo(m_pStream->GetObjNum() != 0 ? NULL : pFormResources, pPageResources)) {
         return 0;
     }
-    if (m_bpc == 0 || m_nComponents == 0) {

[Lei Zhang, 2015/01/30 06:43:58]

If you look in the CL that introduced this:

https://codereview.chromium.org/419693003/diff/40001/core/src/fpdfapi/fpdf_re...

You can see the previous code tolerated the case when these variables are 0.

Perhaps I should remove this check in CPDF_DIBSource::Load() above as well?

[Tom Sepez, 2015/01/30 17:25:29]

From the original review, I asked the question and got the answer:
You can find |FX_DWORD src_pitch = m_bpc;| at line 178 of original code. if
|m_bpc| is 0, it can't load any data at line 198 in the original code because
|m_Height * src_pitch| is 0.

bpc: bits per component
components are used in color space. For example, RGB color space has 3
components. 
They must be larger than 0 (>0).

Maybe we want to bounce this off Jun again.

[Lei Zhang, 2015/01/30 23:00:07]

I disagree with that assessment. If you follow the calls where |src_pitch| is
used:

-> CPDF_StreamAcc::LoadAllData()
--> PDF_DataDecode()
---> FPDFAPI_FlateOrLZWDecode()
----> CCodec_FlateModule::FlateOrLZWDecode()
-----> FlateUncompress()

In FlateUncompress(), the variable that correlates with |src_pitch| is
|orig_size|, and the first thing the function does is to handle the 0 case and
use some default values.
For the test PDF from the bug report, |m_bpc| starts off as 0 in
CPDF_DIBSource::StartLoadDIBSource(). Then we call:

-> CPDF_DIBSource::CreateDecoder()
--> CPDF_DIBSource::LoadJpxBitmap()

and LoadJpxBitmap() sets |m_bpc| to 8 at the end on success.

So either we need to make an exception here, or someone should have set |m_bpc|
earlier. Otherwise, I don't see how JPX decoding can work. I don't know where
the bug is exactly, so I just picked one direction.

[jun_fang, 2015/01/31 04:29:22]

According to PDF standard, BitsPerComponent(bpc) is required in image dictionary
expect for image masks and images that use the JPXDecode filter). That means
BitsPerComponent(bpc) is optional for JPX images. It's incorrect to check |m_bpc
== 0| here without exceptions. It needs to do some changes here. 

-        return 0;
-    }
     FX_SAFE_DWORD src_pitch = m_bpc;
     src_pitch *= m_nComponents;
     src_pitch *= m_Width;
     src_pitch += 7;
     src_pitch /= 8;
     src_pitch *= m_Height;
     if (!src_pitch.IsValid()) {
         return 0;
     }
     m_pStreamAcc = FX_NEW CPDF_StreamAcc;
@@ skipping 230 matching lines @@
 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()
 {
     const CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
     if (decoder.IsEmpty()) {
         return 1;
     }
-    if (m_bpc == 0) {
-        return 0;
+    if (decoder != FX_BSTRC("CCITTFaxDecode") &&

[Lei Zhang, 2015/01/30 06:43:58]

I don't think |m_bpc| matters for these decoders, but I'm not 100% sure.

[jun_fang, 2015/01/31 04:29:22]

It's no need to check bpc for only JPX images and image masks. Other image
formats, we still need to check.   

+        decoder != FX_BSTRC("JPXDecode") &&
+        decoder != FX_BSTRC("JBIG2Decode")) {
+        if (m_bpc == 0) {
+            return 0;
+        }
     }
     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) {
+        if (!m_pDecoder) {
             FX_BOOL bTransform = FALSE;
             int comps, bpc;
             ICodec_JpegModule* pJpegModule = CPDF_ModuleMgr::Get()->GetJpegModule();
             if (pJpegModule->LoadInfo(src_data, src_size, m_Width, m_Height, comps, bpc, bTransform)) {
                 if (m_nComponents != comps) {
                     FX_Free(m_pCompData);
                     m_nComponents = comps;
                     if (m_Family == PDFCS_LAB && m_nComponents != 3) {
                         m_pCompData = NULL;
                         return 0;
@@ skipping 18 matching lines @@
         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) {
-        FX_SAFE_DWORD requested_pitch = m_bpc;
-        requested_pitch *= m_nComponents;
-        requested_pitch *= m_Width;
-        requested_pitch += 7;
-        requested_pitch /= 8;
-        if (!requested_pitch.IsValid()) {
-            return 0;
-        }
-        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 0;
-        }
-        if (provided_pitch.ValueOrDie() < requested_pitch.ValueOrDie()) {
-            return 0;
-        }
-        return 1;
+    if (!m_pDecoder)

[Lei Zhang, 2015/01/30 06:43:58]

This is just to put all the "return 0" cases before the "return 1" case for
consistency.

+        return 0;
+
+    FX_SAFE_DWORD requested_pitch = m_bpc;
+    requested_pitch *= m_nComponents;
+    requested_pitch *= m_Width;
+    requested_pitch += 7;
+    requested_pitch /= 8;
+    if (!requested_pitch.IsValid()) {
+        return 0;
     }
-    return 0;
+    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 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) {
         return;
     }
     FX_LPVOID ctx = pJpxModule->CreateDecoder(m_pStreamAcc->GetData(), m_pStreamAcc->GetSize(), m_pColorSpace != NULL);
     if (ctx == NULL) {
         return;
@@ skipping 914 matching lines @@
     if (!m_bCached) {
         if (m_pBitmap) {
             delete m_pBitmap;
             m_pBitmap = NULL;
         }
         if (m_pMask) {
             delete m_pMask;
         }
     }
 }