Check object type for objects used in the dictory of 'Index'

Check object type for objects used in the dictory of 'Index'

BUG=387970
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/12a9940905825e028bebc0bf8d3eddcc65b1933d

[jun_fang, 2014-09-07 05:49:43]

jun_fang@foxitsoftware.com changed reviewers:
+ bo_xu@foxitsoftware.com, tsepez@chromium.org

[jun_fang, 2014-09-07 05:49:44]

Hi guys, please review this change. Thanks!

[Tom Sepez, 2014-09-08 17:56:18]

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1038: if (pObj &&
(pObj->GetType() == PDFOBJ_NUMBER)) {
nit: no need for inner parenthesis here.

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1040: }
This looks like one of those arrays which needs to have an even number of
elements, because it track pairs, where the even elements mean one thing and the
odd elements mean another.

Ideally, this would be an array of std::pair<int,int> so as to make the meaning
obvious and avoid the possibility of an odd number of elts.  But let's not
change this now.  Instead we need to ensure we don't mess up the ordering here
by sticking in a single element.

Maybe the easiest way to do this it to iterate i += 2 in the for loop, and
process element i and i + i in the body, not inserting either unless both are
good.

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1043: }
nit: I worry about a change down the road where IndexArray might have previous
elements in it before we hit the loop above.  Maybe we want to count the number
of pair insertions above.

[jun_fang, 2014-09-09 18:51:25]

please review it again. Thanks!

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/551503003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1038: if (pObj &&
(pObj->GetType() == PDFOBJ_NUMBER)) {
On 2014/09/08 17:56:18, Tom Sepez wrote:
> nit: no need for inner parenthesis here.

For fuzz tests, elements in the index array can be names or any chars other than
numbers. The element shall be a number according to PDF standard. I will add a
check to break the loop when a non number is detected.

[Tom Sepez, 2014-09-09 23:53:46]

https://codereview.chromium.org/551503003/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/551503003/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1037: CPDF_Object* pObj =
pArray->GetElement(i);
Sorry, I'd be happier if you did this two at at time and not inserting either
rather than breaking and removing the last one.

[jun_fang, 2014-09-10 21:57:35]

On 2014/09/09 23:53:46, Tom Sepez wrote:
>
https://codereview.chromium.org/551503003/diff/20001/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/551503003/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1037: CPDF_Object* pObj =
> pArray->GetElement(i);
> Sorry, I'd be happier if you did this two at at time and not inserting either
> rather than breaking and removing the last one.

I updated the code in patch 5. Please review it.

[Tom Sepez, 2014-09-10 22:58:55]

Very nice. LGTM after fixing one nit.

https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:13: using namespace std;
nit: I'm glad you're using STL. Generally, in chromium, we don't like "using
std" because the word "vector" by itself could mean any one of a number of
home-brew vector implementations, whereas std::vector is definitive.

[Tom Sepez, 2014-09-10 23:05:30]

https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):

https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:12: #include <limits.h>
#include <utility> to be explicit about the declaration of std::pair (before
<vector> to keep it alphabetical).

[jun_fang, 2014-09-10 23:11:59]

On 2014/09/10 23:05:30, Tom Sepez wrote:
>
https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):
> 
>
https://codereview.chromium.org/551503003/diff/40002/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:12: #include <limits.h>
> #include <utility> to be explicit about the declaration of std::pair (before
> <vector> to keep it alphabetical).

Updated in patch 6.

[jun_fang, 2014-09-10 23:19:47]

Committed patchset #6 (id:80001) manually as 12a9940 (presubmit successful).