Relax colorspace checks in CPDF_DIBSource::CreateDecoder().

Relax colorspace checks in CPDF_DIBSource::CreateDecoder().

Previously the log just made sure the colorspace and the image have
exact matches for the number of colorspace components. Now, for some
colorspace types, check the type and make sure the number of
components meets or exceeds what is required by the spec.

Also do some refactoring.

BUG=chromium:650230
Committed: https://pdfium.googlesource.com/pdfium/+/dc40c40f40ed2415605da32f688091a57f53c9e6

[Lei Zhang, 2016-11-09 03:11:22]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 03:11:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-11-09 03:15:39]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 03:15:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 04:30:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 04:30:36]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-11-09 05:21:49]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2016-11-09 17:26:38]

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
File core/fpdfapi/render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:543: if (decoder.IsEmpty()) {
nit: no {}.

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:546: if (m_bDoBpcCheck && m_bpc ==
0) {
nit: no {}

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:549: const uint8_t* src_data =
m_pStreamAcc->GetData();
nit: can these move down some?

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:596: switch (m_Family) {
Do we want to bound the components here to the expected number?  I wonder if
there's code dowstream that does indexing by component into some fixed sized
array of the expected component size ...

[Lei Zhang, 2016-11-09 22:23:49]

Fixed nits first.

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
File core/fpdfapi/render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:543: if (decoder.IsEmpty()) {
On 2016/11/09 17:26:37, Tom Sepez wrote:
> nit: no {}.

Done.

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:546: if (m_bDoBpcCheck && m_bpc ==
0) {
On 2016/11/09 17:26:38, Tom Sepez wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:549: const uint8_t* src_data =
m_pStreamAcc->GetData();
On 2016/11/09 17:26:37, Tom Sepez wrote:
> nit: can these move down some?

Done.

[Lei Zhang, 2016-11-09 22:27:56]

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
File core/fpdfapi/render/fpdf_render_loadimage.cpp (right):

https://codereview.chromium.org/2486123002/diff/20001/core/fpdfapi/render/fpd...
core/fpdfapi/render/fpdf_render_loadimage.cpp:596: switch (m_Family) {
On 2016/11/09 17:26:37, Tom Sepez wrote:
> Do we want to bound the components here to the expected number?  I wonder if
> there's code dowstream that does indexing by component into some fixed sized
> array of the expected component size ...

I don't know what is correct for all the cases. I'm trying to make sure for some
easy to understand ones, the colorspace has at least enough components to
accommodate what's in the image. If there is a mismatch, previously the image
would not render at all, and now it would render, possibly poorly.

I'll try more PDFs outside of our public corpus to see if there are some that
have colorspaces.

In the elephant example, it's family is PDFCS_ICCBASED, the colorspace has 3
components, and the image only has 1.

[Tom Sepez, 2016-11-11 18:20:34]

Ok, let's go with this LGTM.

[Lei Zhang, 2016-11-11 19:33:50]

I looked through many PDFs but could not find another sample that had this
problem. I believe this to be safe, but we can be even more conservative and
only relax the check for the known problematic sample. WDYT?

[Tom Sepez, 2016-11-11 19:49:02]

On 2016/11/11 19:33:50, Lei Zhang (slow) wrote:
> I looked through many PDFs but could not find another sample that had this
> problem. I believe this to be safe, but we can be even more conservative and
> only relax the check for the known problematic sample. WDYT?

Let's go with this as-is ++ LGTM

[Lei Zhang, 2016-11-11 19:52:57]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-11-11 19:53:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 20:06:50]

Description was changed from

==========
Relax colorspace checks in CPDF_DIBSource::CreateDecoder().

Previously the log just made sure the colorspace and the image have
exact matches for the number of colorspace components. Now, for some
colorspace types, check the type and make sure the number of
components meets or exceeds what is required by the spec.

Also do some refactoring.

BUG=chromium:650230
==========

to

==========
Relax colorspace checks in CPDF_DIBSource::CreateDecoder().

Previously the log just made sure the colorspace and the image have
exact matches for the number of colorspace components. Now, for some
colorspace types, check the type and make sure the number of
components meets or exceeds what is required by the spec.

Also do some refactoring.

BUG=chromium:650230

Committed:
https://pdfium.googlesource.com/pdfium/+/dc40c40f40ed2415605da32f688091a57f53...
==========

[commit-bot: I haz the power, 2016-11-11 20:06:51]

Committed patchset #4 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/dc40c40f40ed2415605da32f688091a57f53...