Fix parsing of object numbers > 16,777,216.

Fix parsing of object numbers > 16,777,216.

Currently, there is a check that an object number is <= 0x1000000. If that
check fails, we end up putting the parser into a bad state and fail to load
documents. The object does not need to be in the XRef table, or referenced
from the document, just be in the document.

This Cl removes the size check and updates the various atoi calls to use a
uint32_t instead of an int32_t so we don't end up getting strange values when
converting from a string.

BUG=455199
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/42fb301abcf6b9f6a580f3d30defeadedf5d7ebd

[dsinclair, 2016-03-02 19:32:23]

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

[dsinclair, 2016-03-02 19:32:23]

PTAL.

With this CL we can parse and render the fuzz-95 document in the attached bug.
(Without png rendering pdfium_test takes ~17s to process the file).

[Tom Sepez, 2016-03-02 20:23:53]

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc.cpp:41: 
Are you sure you want to enable this for unsigned types? Try something like the
following:

if (std::numeric_limits<t>::is_signed && str == '-')

[Tom Sepez, 2016-03-02 20:56:38]

https://codereview.chromium.org/1755273002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:111:
EXPECT_TRUE(OpenDocument("bug_455199.pdf"));
Does this take the 17 seconds alluded to in the comments? At some point we'll
start to worry about these runtimes.

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:7: 
note: might be fun to cover the other FXYS_*toi() functions while you're at it.

[dsinclair, 2016-03-02 21:02:06]

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc.cpp:41: 
On 2016/03/02 20:23:52, Tom Sepez wrote:
> Are you sure you want to enable this for unsigned types? Try something like
the
> following:
> 
> if (std::numeric_limits<t>::is_signed && str == '-') 

Awesome, thanks. Didn't know about is_signed. That's a lot nicer.

[dsinclair, 2016-03-02 21:27:29]

https://codereview.chromium.org/1755273002/diff/1/core/src/fpdfapi/fpdf_parse...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fpdfapi/fpdf_parse...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:111:
EXPECT_TRUE(OpenDocument("bug_455199.pdf"));
On 2016/03/02 20:56:38, Tom Sepez wrote:
> Does this take the 17 seconds alluded to in the comments? At some point we'll
> start to worry about these runtimes.

No, this test is fast. I pared it down from 3k objects to 7. The 17s is for the
original file, which, originally would either fail to load.

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/1/core/src/fxcrt/fx_basic_gcc...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:7: 
On 2016/03/02 20:56:38, Tom Sepez wrote:
> note: might be fun to cover the other FXYS_*toi() functions while you're at
it.

Done.

[Tom Sepez, 2016-03-02 21:46:23]

Good, but let's take one more crack at it.

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:14: }
Note that "-2147483648" is legit but can't be parsed at present by our code.
Maybe add a test to show this along with a comment. Also, I'd add an"
2147483648" which isn't legit plus an overlong number of digits, and a string
with a non digit like "9x9".

This applies to all the tests.

[Tom Sepez, 2016-03-02 21:47:38]

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:15: 
Oh, and anything that takes a string needs to be called on "" as the first test
cast.

[Tom Sepez, 2016-03-02 21:48:37]

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:1: // Copyright 2014 PDFium Authors.
All rights reserved.
nit: 2016.  Ha!

[Tom Sepez, 2016-03-02 21:53:37]

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:48: T val = FXSYS_toDecimalDigitWide(*str);
If you really want some fun, try overloading FXSYS_toDecimalDigit() and create
an overloaded FXSYS_isDecimalDigit() to replace the iswdigit call above,
then fold this into the above template like

template <typename IntType, typename CharType> ...

[dsinclair, 2016-03-02 22:09:18]

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc.cpp:48: T val = FXSYS_toDecimalDigitWide(*str);
On 2016/03/02 21:53:36, Tom Sepez wrote:
> If you really want some fun, try overloading FXSYS_toDecimalDigit() and create
> an overloaded FXSYS_isDecimalDigit() to replace the iswdigit call above,
> then fold this into the above template like
> 
> template <typename IntType, typename CharType> ...


Will do in a separate CL.
https://bugs.chromium.org/p/pdfium/issues/detail?id=423

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
File core/src/fxcrt/fx_basic_gcc_unittest.cpp (right):

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:1: // Copyright 2014 PDFium Authors.
All rights reserved.
On 2016/03/02 21:48:37, Tom Sepez wrote:
> nit: 2016.  Ha!

Doh, caught in the copy-paste act.

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:14: }
On 2016/03/02 21:46:23, Tom Sepez wrote:
> Note that "-2147483648" is legit but can't be parsed at present by our code.
> Maybe add a test to show this along with a comment. Also, I'd add an"
> 2147483648" which isn't legit plus an overlong number of digits, and a string
> with a non digit like "9x9".
> 
> This applies to all the tests.

Done, although the results kinda suck. I don't think we can change them without
auditing all the call sites.

https://codereview.chromium.org/1755273002/diff/60001/core/src/fxcrt/fx_basic...
core/src/fxcrt/fx_basic_gcc_unittest.cpp:15: 
On 2016/03/02 21:47:38, Tom Sepez wrote:
> Oh, and anything that takes a string needs to be called on "" as the first
test
> cast.

Done.

[Tom Sepez, 2016-03-02 22:56:56]

lgtm

[dsinclair, 2016-03-03 13:59:26]

Description was changed from

==========
Fix parsing of object numbers > 16,777,216.

Currently, there is a check that an object number is <= 0x1000000. If that
check fails, we end up putting the parser into a bad state and fail to load
documents. The object does not need to be in the XRef table, or referenced
from the document, just be in the document.

This Cl removes the size check and updates the various atoi calls to use a
uint32_t instead of an int32_t so we don't end up getting strange values when
converting from a string.

BUG=455199
==========

to

==========
Fix parsing of object numbers > 16,777,216.

Currently, there is a check that an object number is <= 0x1000000. If that
check fails, we end up putting the parser into a bad state and fail to load
documents. The object does not need to be in the XRef table, or referenced
from the document, just be in the document.

This Cl removes the size check and updates the various atoi calls to use a
uint32_t instead of an int32_t so we don't end up getting strange values when
converting from a string.

BUG=455199
R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/42fb301abcf6b9f6a580f3d30defeadedf5d...
==========

[dsinclair, 2016-03-03 13:59:28]

Committed patchset #6 (id:100001) manually as
42fb301abcf6b9f6a580f3d30defeadedf5d7ebd (presubmit successful).