Add unit tests for ascii85 and hex decoders.

Add unit tests for ascii85 and hex decoders.

R=thestig@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/0db900952c2aa76db801c9198923ce1b3d7c017d

[Wei Li, 2016-02-04 04:35:57]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-02-04 04:37:25]

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

[Wei Li, 2016-02-04 04:37:25]

PTAL, thanks

[Lei Zhang, 2016-02-05 01:25:34]

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (left):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:73: if (zcount > UINT_MAX /
4) {
Why remove this?

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:122: dest_buf = nullptr;
Move this line into the 0 src_size handler?

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:128: while (i < src_size &&
src_buf[i++] != '>') {
Is |i| going to be incremented by 1 more in the new code vs the original code?

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:39:
pdfium::StrFuncTestData* ptr = &flate_encode_cases[i];
Maybe go with const pdfium::StrFuncTestData& ? Ditto below with other tests.

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_unittest.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_unittest.cpp:35:
EXPECT_EQ(ptr->expected_size, result_size);
ASSERT_EQ() or we may get out of bound access below. Ditto on line 70.

[Wei Li, 2016-02-06 00:11:44]

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (left):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:73: if (zcount > UINT_MAX /
4) {
On 2016/02/05 01:25:33, Lei Zhang wrote:
> Why remove this?

This one is redundant as the next statement should cover this.

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:122: dest_buf = nullptr;
On 2016/02/05 01:25:33, Lei Zhang wrote:
> Move this line into the 0 src_size handler?

Done.

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:128: while (i < src_size &&
src_buf[i++] != '>') {
On 2016/02/05 01:25:33, Lei Zhang wrote:
> Is |i| going to be incremented by 1 more in the new code vs the original code?

Done.

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_embeddertest.cpp:39:
pdfium::StrFuncTestData* ptr = &flate_encode_cases[i];
On 2016/02/05 01:25:34, Lei Zhang wrote:
> Maybe go with const pdfium::StrFuncTestData& ? Ditto below with other tests.

Done.

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_unittest.cpp (right):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode_unittest.cpp:35:
EXPECT_EQ(ptr->expected_size, result_size);
On 2016/02/05 01:25:34, Lei Zhang wrote:
> ASSERT_EQ() or we may get out of bound access below. Ditto on line 70.

Done.

[Lei Zhang, 2016-02-06 00:17:04]

lgtm with comment below addressed:

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (left):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:73: if (zcount > UINT_MAX /
4) {
On 2016/02/06 00:11:44, Wei Li wrote:
> On 2016/02/05 01:25:33, Lei Zhang wrote:
> > Why remove this?
> 
> This one is redundant as the next statement should cover this. 

Sorry, it's not. If |zcount| is very large, zcount * 4 on the next line would
suffer from an integer overflow, and the comparison would be invalid. Please add
this back.

[Wei Li, 2016-02-06 01:16:27]

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (left):

https://codereview.chromium.org/1666663004/diff/20001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:73: if (zcount > UINT_MAX /
4) {
On 2016/02/06 00:17:04, Lei Zhang wrote:
> On 2016/02/06 00:11:44, Wei Li wrote:
> > On 2016/02/05 01:25:33, Lei Zhang wrote:
> > > Why remove this?
> > 
> > This one is redundant as the next statement should cover this. 
> 
> Sorry, it's not. If |zcount| is very large, zcount * 4 on the next line would
> suffer from an integer overflow, and the comparison would be invalid. Please
add
> this back.

In that case, this should fix it.

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

https://codereview.chromium.org/1666663004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1666663004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:76: dest_buf =
FX_Alloc(uint8_t, zcount * 4 + (pos - zcount));
I looked at this just now - isn't this just: zcount * 3 + pos? In which case,
the overflow check can be simplified to:

zcount > (UINT_MAX - pos) / 3

[Wei Li, 2016-02-06 02:18:59]

https://codereview.chromium.org/1666663004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp (right):

https://codereview.chromium.org/1666663004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_decode.cpp:76: dest_buf =
FX_Alloc(uint8_t, zcount * 4 + (pos - zcount));
On 2016/02/06 01:22:02, Lei Zhang wrote:
> I looked at this just now - isn't this just: zcount * 3 + pos? In which case,
> the overflow check can be simplified to:
> 
> zcount > (UINT_MAX - pos) / 3

Now let's make it right :)

[Wei Li, 2016-02-06 02:22:20]

Patchset #4 (id:80001) has been deleted

[Wei Li, 2016-02-09 19:38:57]

Description was changed from

==========
Add unit tests for ascii85 and hex decoders.
==========

to

==========
Add unit tests for ascii85 and hex decoders.

R=thestig@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/0db900952c2aa76db801c9198923ce1b3d7c...
==========

[Wei Li, 2016-02-09 19:38:58]

Committed patchset #6 (id:140001) manually as
0db900952c2aa76db801c9198923ce1b3d7c017d (presubmit successful).