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

core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp

Index: core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
diff --git a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
index 1637655e68d9f44642a9c478ce37521371158bf0..e4d20413a6240d9c1dacc64bcdec5fb196cf01b1 100644
--- a/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
+++ b/core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp
@@ -314,9 +314,6 @@ int	CPDF_DIBSource::StartLoadDIBSource(CPDF_Document* pDoc, const CPDF_Stream* p
     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;
@@ -567,8 +564,12 @@ int CPDF_DIBSource::CreateDecoder()
     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();
@@ -578,7 +579,7 @@ int CPDF_DIBSource::CreateDecoder()
     } 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();
@@ -617,29 +618,29 @@ int CPDF_DIBSource::CreateDecoder()
     } 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()
 {