Handle another integer overflow in ReadPageHintTable().

Handle another integer overflow in ReadPageHintTable().

Return false instead of crashing.

BUG=641882
Committed: https://pdfium.googlesource.com/pdfium/+/8d3ca14840a027c3dd1e2c943795d057dbb91454

[Lei Zhang, 2016-09-01 02:56:08]

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

[commit-bot: I haz the power, 2016-09-01 02:56:18]

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-09-01 03:16:01]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2016-09-01 03:22:06]

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

[commit-bot: I haz the power, 2016-09-01 03:22:13]

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-09-01 03:23:05]

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

[commit-bot: I haz the power, 2016-09-01 03:23:13]

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-09-01 03:51:13]

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

[commit-bot: I haz the power, 2016-09-01 03:51:14]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-01 03:54:42]

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

[dsinclair, 2016-09-01 13:39:24]

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

[dsinclair, 2016-09-01 13:39:25]

lgtm

[Tom Sepez, 2016-09-01 16:08:04]

LGTM w/question and nit.

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp (right):

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:54: int nStreamOffset =
ReadPrimaryHintStreamOffset();
nit: can these two be int32_t's since presumably the data is coming from 4 bytes
in a stream somewhere?

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:60:
!pdfium::base::IsValueInRangeForNumericType<FX_FILESIZE>(nStreamLen)) {
Ok, but I thought FX_FILESIZE was signed so we could return -1 as an error code
... did you have a situation where this could trip?

[Oliver Chang, 2016-09-01 16:08:22]

lgtm, so the symbolization was completely off?

[Lei Zhang, 2016-09-01 17:19:41]

On 2016/09/01 16:08:22, Oliver Chang wrote:
> lgtm, so the symbolization was completely off?

It got to the right function. We are just off by ~120 lines.

[Lei Zhang, 2016-09-01 17:49:11]

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

[commit-bot: I haz the power, 2016-09-01 17:49:17]

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-09-01 17:49:19]

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp (right):

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:54: int nStreamOffset =
ReadPrimaryHintStreamOffset();
On 2016/09/01 16:08:04, Tom Sepez wrote:
> nit: can these two be int32_t's since presumably the data is coming from 4
bytes
> in a stream somewhere?

No, they are from CPDF_Number::GetInteger() and not 4 byte entry in the header.

https://codereview.chromium.org/2300903002/diff/40001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_hint_tables.cpp:60:
!pdfium::base::IsValueInRangeForNumericType<FX_FILESIZE>(nStreamLen)) {
On 2016/09/01 16:08:04, Tom Sepez wrote:
> Ok, but I thought FX_FILESIZE was signed so we could return -1 as an error
code
> ... did you have a situation where this could trip?

It may not be obvious if an int fits inside a FX_FILESIZE.

As is FX_FILESIZE can either be int32_t or off_t, so this probably won't trip.
If it does ever trip, I'd like it to trip here early and return false, rather
than trip with a checked_cast on line 76.

[commit-bot: I haz the power, 2016-09-01 18:40:23]

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

[commit-bot: I haz the power, 2016-09-01 18:40:24]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-01 18:46:59]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-09-01 18:46:59]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
ochang@chromium.org, dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/2300903002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-01 18:47:05]

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-01 18:47:19]

Description was changed from

==========
Handle another integer overflow in ReadPageHintTable().

Return false instead of crashing.

BUG=641882
==========

to

==========
Handle another integer overflow in ReadPageHintTable().

Return false instead of crashing.

BUG=641882

Committed:
https://pdfium.googlesource.com/pdfium/+/8d3ca14840a027c3dd1e2c943795d057dbb9...
==========

[commit-bot: I haz the power, 2016-09-01 18:47:20]

Committed patchset #3 (id:60001) as
https://pdfium.googlesource.com/pdfium/+/8d3ca14840a027c3dd1e2c943795d057dbb9...