Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

if |m_Family| was RGB, the code assumed there were 3 components, which
may not be the case.

BUG=chromium:602046
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/6a3521f049b35c801f124f1573718021a785ff6b

[Oliver Chang, 2016-04-12 00:37:19]

Description was changed from

==========
Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

if |m_Family| was RGB, the code assumed there were 3 components, which
may not be the case.

BUG=chromium:602046
==========

to

==========
Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

if |m_Family| was RGB, the code assumed there were 3 components, which
may not be the case.

BUG=chromium:602046
R=tsepez@chromium.org
==========

[Oliver Chang, 2016-04-12 00:37:27]

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

[Tom Sepez, 2016-04-12 00:39:27]

Can we also add the test?

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

https://codereview.chromium.org/1877033003/diff/1/core/fpdfapi/fpdf_render/fp...
core/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:921: m_nComponents == 3) {
Do we just want to return in error if m_nComponents is not 3 for these families?
 Or do we get good output if we continue?

[Oliver Chang, 2016-04-12 00:48:59]

I can only reproduce this on a chromium checkout, probably because chromium uses
libjpeg_turbo while standalone uses libjpeg. Not sure how much work is needed/if
it's possible to create a test that works for standalone.

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

https://codereview.chromium.org/1877033003/diff/1/core/fpdfapi/fpdf_render/fp...
core/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:921: m_nComponents == 3) {
On 2016/04/12 00:39:27, Tom Sepez wrote:
> Do we just want to return in error if m_nComponents is not 3 for these
families?
>  Or do we get good output if we continue?

I'm not sure if we get good output, but it doesn't look like the rest of the
code in this function makes similarly incorrect assumptions. Would it be better
to just return early to be safe?

[Tom Sepez, 2016-04-12 16:30:57]

> I'm not sure if we get good output, but it doesn't look like the rest of the
> code in this function makes similarly incorrect assumptions. Would it be
better
> to just return early to be safe?
SGTM

[Oliver Chang, 2016-04-12 16:36:22]

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

https://codereview.chromium.org/1877033003/diff/1/core/fpdfapi/fpdf_render/fp...
core/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:921: m_nComponents == 3) {
On 2016/04/12 00:48:59, Oliver Chang wrote:
> On 2016/04/12 00:39:27, Tom Sepez wrote:
> > Do we just want to return in error if m_nComponents is not 3 for these
> families?
> >  Or do we get good output if we continue?
> 
> I'm not sure if we get good output, but it doesn't look like the rest of the
> code in this function makes similarly incorrect assumptions. Would it be
better
> to just return early to be safe? 

Done.

[Oliver Chang, 2016-04-12 18:41:26]

On 2016/04/12 16:36:22, Oliver Chang wrote:
>
https://codereview.chromium.org/1877033003/diff/1/core/fpdfapi/fpdf_render/fp...
> File core/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right):
> 
>
https://codereview.chromium.org/1877033003/diff/1/core/fpdfapi/fpdf_render/fp...
> core/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:921: m_nComponents == 3) {
> On 2016/04/12 00:48:59, Oliver Chang wrote:
> > On 2016/04/12 00:39:27, Tom Sepez wrote:
> > > Do we just want to return in error if m_nComponents is not 3 for these
> > families?
> > >  Or do we get good output if we continue?
> > 
> > I'm not sure if we get good output, but it doesn't look like the rest of the
> > code in this function makes similarly incorrect assumptions. Would it be
> better
> > to just return early to be safe? 
> 
> Done.

Tom, any other comments apart from test? (I'd like to get this in with the next
roll if possible). As mentioned, the testcase provided by the reporter doesn't
reproduce with standalone pdfium_test -- I suspect libjpeg errors out earlier in
this case vs libjpeg_turbo. Not sure what I can do here...

[Tom Sepez, 2016-04-12 20:06:40]

lgtm

[Oliver Chang, 2016-04-12 20:10:57]

The CQ bit was checked by ochang@chromium.org

[commit-bot: I haz the power, 2016-04-12 20:11:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877033003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877033003/20001

[commit-bot: I haz the power, 2016-04-12 20:31:39]

Description was changed from

==========
Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

if |m_Family| was RGB, the code assumed there were 3 components, which
may not be the case.

BUG=chromium:602046
R=tsepez@chromium.org
==========

to

==========
Prevent an OOB access in CPDF_DIBSource::TranslateScanline24bpp

if |m_Family| was RGB, the code assumed there were 3 components, which
may not be the case.

BUG=chromium:602046
R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/6a3521f049b35c801f124f1573718021a785...
==========

[commit-bot: I haz the power, 2016-04-12 20:31:40]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/6a3521f049b35c801f124f1573718021a785...