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

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

BUG=453723
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/254360730190cc6d6e3de325ee101948b78c1e32

[Lei Zhang, 2015-01-30 06:43:57]

thestig@chromium.org changed reviewers:
+ jun_fang@foxitsoftware.com, tsepez@chromium.org

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

Warning: I don't really understand this code, so please review carefully.

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (left):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:317: if (m_bpc == 0 ||
m_nComponents == 0) {
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?

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:567: if (decoder !=
FX_BSTRC("CCITTFaxDecode") &&
I don't think |m_bpc| matters for these decoders, but I'm not 100% sure.

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:621: if (!m_pDecoder)
This is just to put all the "return 0" cases before the "return 1" case for
consistency.

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/render_int.h (left):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/render_int.h:424: FX_BOOL				CreateDecoder();
Funny how a "bool" can return 2. :\

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

Do you have sample images for use in tests?

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (left):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:317: if (m_bpc == 0 ||
m_nComponents == 0) {
From the original review, I asked the question and got the answer:

> We continue execution here in the old code in this case. Why do we want to
> short-circuit the calls below? Is this the comparison from line 221 of the old
> file moved up here?

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]

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (left):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:317: if (m_bpc == 0 ||
m_nComponents == 0) {
On 2015/01/30 17:25:29, Tom Sepez wrote:
> From the original review, I asked the question and got the answer:
> 
> > We continue execution here in the old code in this case. Why do we want to
> > short-circuit the calls below? Is this the comparison from line 221 of the
old
> > file moved up here?
> 
> 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.

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.

> 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.

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.

[Tom Sepez, 2015-01-30 23:08:25]

Feels like the right thing to do is to set m_bpc earlier, then we wouldn't need
any of the special cases below, no?

[Lei Zhang, 2015-01-31 01:47:51]

On 2015/01/30 23:08:25, Tom Sepez wrote:
> Feels like the right thing to do is to set m_bpc earlier, then we wouldn't
need
> any of the special cases below, no?

Maybe, the code here is tricky though. What's happening is we bail out early in
CPDF_DIBSource::LoadColorInfo() for the JPX cases, so |m_bpc| and
|m_nComponents| never gets set and stays at 0. CPDF_DIBSource::LoadJpxBitmap(),
which we are currently not calling, is also strange because it can read |m_bpc|
before finally setting it to 8. LoadJpxBitmap() also only sets |m_nComponents|
in some cases.

[Lei Zhang, 2015-01-31 02:12:33]

Jun, BTW, do you have test cases for JPX images? I feel we should not be in this
situation as testing should have caught this regression much earlier.

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

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (left):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:317: if (m_bpc == 0 ||
m_nComponents == 0) {
On 2015/01/30 23:00:07, Lei Zhang wrote:
> On 2015/01/30 17:25:29, Tom Sepez wrote:
> > From the original review, I asked the question and got the answer:
> > 
> > > We continue execution here in the old code in this case. Why do we want to
> > > short-circuit the calls below? Is this the comparison from line 221 of the
> old
> > > file moved up here?
> > 
> > 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.
> 
> 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.
> 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/1/core/src/fpdfapi/fpdf_render...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:567: if (decoder !=
FX_BSTRC("CCITTFaxDecode") &&
On 2015/01/30 06:43:58, Lei Zhang wrote:
> I don't think |m_bpc| matters for these decoders, but I'm not 100% sure.

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

[Lei Zhang, 2015-02-07 08:39:11]

I created one possible solution in patch set 3, and another in patch set 4.
Patch set 4 is a bit more extensive and tries to fix the same problem for
CCITTFax.

https://codereview.chromium.org/892553002/diff/60001/core/src/fpdfapi/fpdf_re...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/60001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:217: m_bpc = 1;
I don't have a test image, but I don't see how this can work given the |m_bpc|
check on line 201. So I modified LoadColorInfo() to skip the check for
"CCITTFaxDecode" as well.

In patch set 1, I suspected we can skip the check for JPX, CCITTFax, and JBIG2.
I didn't make the change for JBIG2, but I still suspect we need to.

[Tom Sepez, 2015-02-08 17:33:04]

On 2015/02/07 08:39:11, Lei Zhang wrote:
Let me give you a LGTM for either one.  I'd suggest landing #3 initially, then
landing #4 as a follow-on CL in case Jun comes up with a reason to roll it back
down the road.

[Lei Zhang, 2015-02-10 02:51:13]

New patchsets have been uploaded after l-g-t-m from tsepez@chromium.org

[Lei Zhang, 2015-02-10 02:53:03]

On 2015/02/08 17:33:04, Tom Sepez wrote:
> On 2015/02/07 08:39:11, Lei Zhang wrote:
> Let me give you a LGTM for either one.  I'd suggest landing #3 initially, then
> landing #4 as a follow-on CL in case Jun comes up with a reason to roll it
back
> down the road.

I'll wait for Jun to take a look. Meanwhile, I'll go look for "CITTFaxDecode"
and JBIG2 test cases.

[jun_fang, 2015-02-11 16:55:24]

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:334: const
CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
ValidateDictParam() is used to correct parameters in dictionary. I think that we
can move this part into ValidateDictParam().

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:456: filter ==
FX_BSTRC("CCITTFaxDecode")) {
Only JPX may not have "ColorSpace". It's no need to add "CCITTFaxDecode" here.

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:463:
pArray->GetString(pArray->GetCount() - 1) == FX_BSTRC("CCITTFaxDecode")) {
Only JPX may not have "ColorSpace". It's no need to add "CCITTFaxDecode" here.

[Lei Zhang, 2015-02-13 02:15:37]

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:334: const
CFX_ByteString& decoder = m_pStreamAcc->GetImageDecoder();
On 2015/02/11 16:55:24, jun_fang wrote:
> ValidateDictParam() is used to correct parameters in dictionary. I think that
we
> can move this part into ValidateDictParam().

No we can't. CreateDecoder will fail first because |m_bpc| is 0, and we'll
return on line 342, so we'll never get to ValidateDictParam(). Notice how
CPDF_DIBSource::Load() above which is very similar to this function already
makes an exception for CCITTFaxDecode on line 215.

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:456: filter ==
FX_BSTRC("CCITTFaxDecode")) {
On 2015/02/11 16:55:24, jun_fang wrote:
> Only JPX may not have "ColorSpace". It's no need to add "CCITTFaxDecode" here.

Can you suggest some other way for CCITTFaxDecode to pass the |m_bpc| /
|m_nComponents| check right after LoadColorInfo() returns? Without this, they
will fail the check because we don't set |m_bpc| for CCITTFaxDecode until later.

[Lei Zhang, 2015-02-13 02:51:07]

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/892553002/diff/80001/core/src/fpdfapi/fpdf_re...
core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:456: filter ==
FX_BSTRC("CCITTFaxDecode")) {
On 2015/02/13 02:15:37, Lei Zhang wrote:
> On 2015/02/11 16:55:24, jun_fang wrote:
> > Only JPX may not have "ColorSpace". It's no need to add "CCITTFaxDecode"
here.
> 
> Can you suggest some other way for CCITTFaxDecode to pass the |m_bpc| /
> |m_nComponents| check right after LoadColorInfo() returns? Without this, they
> will fail the check because we don't set |m_bpc| for CCITTFaxDecode until
later.

Oh, nevermind. I understand what you are saying.

[Lei Zhang, 2015-02-13 03:00:23]

On 2015/02/13 02:51:07, Lei Zhang wrote:
> Oh, nevermind. I understand what you are saying.

See patch set 6.

[Jun Fang, 2015-02-13 19:34:35]

On 2015/02/13 03:00:23, Lei Zhang wrote:
> On 2015/02/13 02:51:07, Lei Zhang wrote:
> > Oh, nevermind. I understand what you are saying.
> 
> See patch set 6.

LGTM. Thanks!

[Lei Zhang, 2015-02-13 20:02:46]

Committed patchset #6 (id:100001) manually as
254360730190cc6d6e3de325ee101948b78c1e32 (presubmit successful).