Fix compare between signed and unsigned values in CPDF_ImageRenderer::StartDIBSource.

Fix compare between signed and unsigned values in CPDF_ImageRenderer::StartDIBSource.

Correct the compare logic in CPDF_ImageRenderer::StartDIBSource() by using size_t instead of int.

BUG=chromium:645036
R=ochang@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/2f8568ef91156d2deb8411c427fbb52f880ccc34

[Lei Zhang, 2016-09-09 22:14:25]

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

[Lei Zhang, 2016-09-09 22:14:26]

Maybe what you really want is FX_SAFE_SIZE_T or one of its friends.

[Ke Liu, 2016-09-12 12:24:01]

On 2016/09/09 22:14:26, Lei Zhang (OOO) wrote:
> Maybe what you really want is FX_SAFE_SIZE_T or one of its friends.

Thanks. I changed to FX_SAFE_SIZE_T here.

FX_SAFE_SIZE_T will detect RANGE_UNDERFLOW {value_=0xffff4740
validity_=RANGE_UNDERFLOW (0x00000001) }.
So function ValueOrDie() would throw an exception in Debug mode but Release mode
is quiet.

[Oliver Chang, 2016-09-12 16:21:33]

https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
File core/fpdfapi/fpdf_render/fpdf_render_image.cpp (right):

https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_image.cpp:765: FX_SAFE_SIZE_T image_size =
m_pDIBSource->GetBPP() / 8 * m_pDIBSource->GetWidth() *
This is incorrect, as any integer errors would've already occurred by the time
you assign to image_size.

You need to do something like:

FX_SAFE_SIZE_T image_size = m_pDIBSource->GetBPP();
image_size /= 8;
image_size *= m_pDIBSource->GetWidth();
...

https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
core/fpdfapi/fpdf_render/fpdf_render_image.cpp:767: if (image_size.ValueOrDie()
> FPDF_HUGE_IMAGE_SIZE &&
Usually we don't want to crash if image_size is not valid. The convention is to
do something like:

if (!image_size.IsValid())
  return FALSE;

[Ke Liu, 2016-09-13 01:46:17]

On 2016/09/12 16:21:33, Oliver Chang wrote:
>
https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
> File core/fpdfapi/fpdf_render/fpdf_render_image.cpp (right):
> 
>
https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
> core/fpdfapi/fpdf_render/fpdf_render_image.cpp:765: FX_SAFE_SIZE_T image_size
=
> m_pDIBSource->GetBPP() / 8 * m_pDIBSource->GetWidth() *
> This is incorrect, as any integer errors would've already occurred by the time
> you assign to image_size.
> 
> You need to do something like:
> 
> FX_SAFE_SIZE_T image_size = m_pDIBSource->GetBPP();
> image_size /= 8;
> image_size *= m_pDIBSource->GetWidth();
> ...
> 
>
https://codereview.chromium.org/2323663002/diff/20001/core/fpdfapi/fpdf_rende...
> core/fpdfapi/fpdf_render/fpdf_render_image.cpp:767: if
(image_size.ValueOrDie()
> > FPDF_HUGE_IMAGE_SIZE &&
> Usually we don't want to crash if image_size is not valid. The convention is
to
> do something like:
> 
> if (!image_size.IsValid())
>   return FALSE;

Thank you Oliver. Actually some overflow checks has been made before we reach
here. But it's always good to check it. I've updated the code and it won't die
now.

[Oliver Chang, 2016-09-14 20:03:40]

lgtm, thanks

[Ke Liu, 2016-09-19 13:53:11]

The CQ bit was checked by stackexploit@gmail.com

[commit-bot: I haz the power, 2016-09-19 13:53:18]

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-09-19 14:05:53]

Description was changed from

==========
Fix compare between signed and unsigned values in
CPDF_ImageRenderer::StartDIBSource.

Correct the compare logic in CPDF_ImageRenderer::StartDIBSource() by using
size_t instead of int.

BUG=chromium:645036
R=ochang@chromium.org
==========

to

==========
Fix compare between signed and unsigned values in
CPDF_ImageRenderer::StartDIBSource.

Correct the compare logic in CPDF_ImageRenderer::StartDIBSource() by using
size_t instead of int.

BUG=chromium:645036
R=ochang@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/2f8568ef91156d2deb8411c427fbb52f880c...
==========

[commit-bot: I haz the power, 2016-09-19 14:05:54]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/2f8568ef91156d2deb8411c427fbb52f880c...