Add a simple check for V4 cross reference table

Add a simple check for V4 cross reference table

Some unknown software generates cross reference table with the
claimed object numbers are all off by one. Add a simple verification
function to detect this scenario, thus have a chance to rebuild
the correct cross reference table.

To avoid unnecessary checks and potiential performance hit, we only
check for the very first non free entry.

BUG=602650
Committed: https://pdfium.googlesource.com/pdfium/+/90c964fb069bfa9934493481f8ccfefef9c3b3fe

[Wei Li, 2016-04-21 19:07:17]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-04-21 19:40:30]

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

[Wei Li, 2016-04-21 19:40:31]

PTAL, thanks

[dsinclair, 2016-04-21 19:47:08]

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

[dsinclair, 2016-04-21 19:47:09]

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:324: for (const auto& it :
m_ObjectInfo) {
Is this going to cause a perf regression if you have a large xref table?

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:326: if (it.second.pos != 0) {
nit: if (it.second_pos == 0)
       continue;

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:335: // something wrong with the cross
reference table.
nit: something is wrong

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:393: if (i == 0 && !VerifyCrossRefV4())
Why only verify the first one and not all of them? Is the assumption that if the
first is correct they'll all be correct?

[Wei Li, 2016-04-21 20:56:45]

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:324: for (const auto& it :
m_ObjectInfo) {
On 2016/04/21 19:47:09, dsinclair wrote:
> Is this going to cause a perf regression if you have a large xref table?

No since this only runs for one entry, not the whole table.

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:326: if (it.second.pos != 0) {
On 2016/04/21 19:47:09, dsinclair wrote:
> nit: if (it.second_pos == 0)
>        continue;

Done.

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:335: // something wrong with the cross
reference table.
On 2016/04/21 19:47:09, dsinclair wrote:
> nit: something is wrong

Done.

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:393: if (i == 0 && !VerifyCrossRefV4())
On 2016/04/21 19:47:09, dsinclair wrote:
> Why only verify the first one and not all of them? Is the assumption that if
the
> first is correct they'll all be correct?

This is related to your perf regression question. We want to avoid penalty for
well-behaved pdfs.

[Wei Li, 2016-04-21 20:58:36]

Patchset #2 (id:30001) has been deleted

[dsinclair, 2016-04-21 21:02:54]

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
core/fpdfapi/fpdf_parser/cpdf_parser.cpp:393: if (i == 0 && !VerifyCrossRefV4())
On 2016/04/21 20:56:44, Wei Li wrote:
> On 2016/04/21 19:47:09, dsinclair wrote:
> > Why only verify the first one and not all of them? Is the assumption that if
> the
> > first is correct they'll all be correct?
> 
> This is related to your perf regression question. We want to avoid penalty for
> well-behaved pdfs.


Right, I guess my question is, is there the potential that we're going to get an
issue where the first xref table is correct but subsequent ones are off-by-one?
So, if the bad writer extends a file and adds a new xref table, will we end up
crashing in that case?

[Wei Li, 2016-04-21 21:02:56]

Description was changed from

==========
Add a simple check for V4 cross reference table

Some unknown software generates cross reference table with the
claimed object numbers are all off by one. Add a simple verification
function to detect this scenario, thus have a chance to rebuild
the correct cross reference table.

BUG=602650
==========

to

==========
Add a simple check for V4 cross reference table

Some unknown software generates cross reference table with the
claimed object numbers are all off by one. Add a simple verification
function to detect this scenario, thus have a chance to rebuild
the correct cross reference table.

To avoid unnecessary checks and potiential performance hit, we only
check for the very first non free entry.

BUG=602650
==========

[Wei Li, 2016-04-21 21:10:35]

On 2016/04/21 21:02:54, dsinclair wrote:
>
https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
> File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):
> 
>
https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
> core/fpdfapi/fpdf_parser/cpdf_parser.cpp:393: if (i == 0 &&
!VerifyCrossRefV4())
> On 2016/04/21 20:56:44, Wei Li wrote:
> > On 2016/04/21 19:47:09, dsinclair wrote:
> > > Why only verify the first one and not all of them? Is the assumption that
if
> > the
> > > first is correct they'll all be correct?
> > 
> > This is related to your perf regression question. We want to avoid penalty
for
> > well-behaved pdfs.
> 
> 
> Right, I guess my question is, is there the potential that we're going to get
an
> issue where the first xref table is correct but subsequent ones are
off-by-one?
> So, if the bad writer extends a file and adds a new xref table, will we end up
> crashing in that case?

No, we will be able to detect that and handle correctly. However, if it was
edited
three times with xref tables such good--bad--good. Then this would not detect
it. 

PDFium's goal is not to handle arbitrary non-standard PDF, we just need some 
tolerance to some old/buggy software.

Any better alternatives?

[dsinclair, 2016-04-21 23:50:40]

On 2016/04/21 21:10:35, Wei Li wrote:
> On 2016/04/21 21:02:54, dsinclair wrote:
> >
>
https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
> > File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1910063004/diff/2/core/fpdfapi/fpdf_parser/cp...
> > core/fpdfapi/fpdf_parser/cpdf_parser.cpp:393: if (i == 0 &&
> !VerifyCrossRefV4())
> > On 2016/04/21 20:56:44, Wei Li wrote:
> > > On 2016/04/21 19:47:09, dsinclair wrote:
> > > > Why only verify the first one and not all of them? Is the assumption
that
> if
> > > the
> > > > first is correct they'll all be correct?
> > > 
> > > This is related to your perf regression question. We want to avoid penalty
> for
> > > well-behaved pdfs.
> > 
> > 
> > Right, I guess my question is, is there the potential that we're going to
get
> an
> > issue where the first xref table is correct but subsequent ones are
> off-by-one?
> > So, if the bad writer extends a file and adds a new xref table, will we end
up
> > crashing in that case?
> 
> No, we will be able to detect that and handle correctly. However, if it was
> edited
> three times with xref tables such good--bad--good. Then this would not detect
> it. 
> 
> PDFium's goal is not to handle arbitrary non-standard PDF, we just need some 
> tolerance to some old/buggy software.
> 
> Any better alternatives?

Nope, just curious how it will act in the face of bad data.

lgtm

[Wei Li, 2016-04-22 22:10:12]

The CQ bit was checked by weili@chromium.org

[commit-bot: I haz the power, 2016-04-22 22:10:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1910063004/50001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1910063004/50001

[commit-bot: I haz the power, 2016-04-22 22:25:18]

Description was changed from

==========
Add a simple check for V4 cross reference table

Some unknown software generates cross reference table with the
claimed object numbers are all off by one. Add a simple verification
function to detect this scenario, thus have a chance to rebuild
the correct cross reference table.

To avoid unnecessary checks and potiential performance hit, we only
check for the very first non free entry.

BUG=602650
==========

to

==========
Add a simple check for V4 cross reference table

Some unknown software generates cross reference table with the
claimed object numbers are all off by one. Add a simple verification
function to detect this scenario, thus have a chance to rebuild
the correct cross reference table.

To avoid unnecessary checks and potiential performance hit, we only
check for the very first non free entry.

BUG=602650

Committed:
https://pdfium.googlesource.com/pdfium/+/90c964fb069bfa9934493481f8ccfefef9c3...
==========

[commit-bot: I haz the power, 2016-04-22 22:25:19]

Committed patchset #2 (id:50001) as
https://pdfium.googlesource.com/pdfium/+/90c964fb069bfa9934493481f8ccfefef9c3...