Fix #297 : fix corruption in startxref or xref table

[pubpub-zz]

Proposal for #297

Issue is due to corrupted startxref pointer and/or xref table
Fixed proposed if incorrect pointer look for all objs to rebuild an xref table

Tests updated

[codecov-commenter]

Codecov Report

Merging #788 (2852248) into main (e7fe707) will increase coverage by 0.09%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##             main     #788      +/-   ##
==========================================
+ Coverage   75.21%   75.31%   +0.09%     
==========================================
  Files          11       12       +1     
  Lines        3514     3609      +95     
  Branches      809      835      +26     
==========================================
+ Hits         2643     2718      +75     
- Misses        658      670      +12     
- Partials      213      221       +8     

Impacted Files	Coverage Δ
PyPDF2/pdf.py	81.72% <95.34%> (-0.11%)	⬇️
PyPDF2/merger.py	68.07% <0.00%> (-4.87%)	⬇️
PyPDF2/generic.py	67.84% <0.00%> (ø)
PyPDF2/__init__.py	100.00% <0.00%> (ø)
PyPDF2/papersizes.py	100.00% <0.00%> (ø)
PyPDF2/xmp.py	50.27% <0.00%> (+7.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7fe707...2852248. Read the comment docs.

[MartinThoma]

#297

[MartinThoma]

This deactivates the test. At least for strict=False I would expect reader.extractText() to give something reasonable.

The rest of the PR might be fine, but it does not fix issue297. I will not merge this part.

[pubpub-zz]

@MartinThoma
Thanks for your review. I do not master pytest. Test reviewed to check both Strict True and False.

[MartinThoma]

I understand :-)

This might help to capture both cases:

@pytest.mark.parametrize(
    "set_strict, [True, False]
)
def test_issue297(set_strict):
    .... # the test: you pass set_strict to the PdfFileReader and you only do 'with pytest.raises' if set_strict is True

[pubpub-zz]

@MartinThoma ,
The expected result not being the same I've included both in the test manually isn't this good for you ?

def test_issue297():
    path = os.path.join(RESOURCE_ROOT, "issue-297.pdf")
    with pytest.raises(PdfReadWarning) as exc:
        print(exc)
        reader = PdfFileReader(path,strict=True)
        reader.getPage(0)
    assert ( exc.value.args[0].find("startxref") > 0)
    reader = PdfFileReader(path,strict=False)
    reader.getPage(0)

[MartinThoma]

@pubpub-zz Thank your for pointing it out - yes, that is ok :-) The parametrize option is better as it shows clearer where an issue is (if there was one) + it avoids a bit of code duplication. But leave it as is - that is just a nitpick; I'll polish that in another commit. Thank you 🤗

I'll not merge it for now. In my roof apartment one of the windows just broke out ... I'm still shocked + cannot concentrate. I'll likely do review it again tomorrow when I can focus again 😅

[pubpub-zz]

@MartinThoma, good luck with your Window(...s 10 or 11 ? That is the most common window which is breaking down 🤣)

[MartinThoma]

Good morning!

There seems to be something off with test_read_unknown_zero_pages. You've commented, that this is necessary due to the double-percentage in the beginning (%%PDF-1.7). However, this is a part of the percentage formatting. The actual formatted byte-string looks like this:

%PDF-1.7
1 0 obj << /Count 1 /Kids [4 0 R] /Type /Pages >> endobj
2 0 obj << >> endobj
3 0 obj << >> endobj
4 0 obj << /Contents 3 0 R /CropBox [0.0 0.0 2550.0 3508.0] /MediaBox [0.0 0.0 2550.0 3508.0] /Parent 1 0 R /Resources << /Font << >> >> /Rotate 0 /Type /Page >> endobj
5 0 obj << /Pages 0 0 R /Type /Catalog >> endobj
xref 1 5
0000000010 00000 n
0000000067 00000 n
0000000088 00000 n
0000000109 00000 n
0000000278 00000 n
trailer << /Root 5 1 R /Size 6 >>
startxref 327
%%EOF'

As you can see, the byte string starts with %PDF-1.7 and not %%PDF-1.7.

Can you help me understand why the -1 is now necessary?

[MartinThoma]

(minor comment, feel free to ignore: Running black over just the test is fine: black Tests/test_reader.py. Please don't run it over the complete codebase as this would add to many changes, but I already had it running over the Tests. For this reason it will just beautify your additions)

[pubpub-zz]

As you can see, the byte string starts with %PDF-1.7 and not %%PDF-1.7.

Can you help me understand why the -1 is now necessary?

due to the formatting, the original formatting string includes a double '%%' that is converted to a single during formating. the point is that the .find(b"xref") applies the search on the original function. therefore the 327 points to the r and not the x of the xref.
the original code was not checking for the xref not reporting any error being tolerant to 'invalid' pdf data.
with the new code, strict=true will reject the file - not compliant with the spec - but with string=false, the xref table will be rebuilt, being able to accept the file
@MartinThoma , Do you think it would be better to accept this pointer error as it used to be without rebuilding the xref table?

[MartinThoma]

the .find(b"xref") applies the search on the original

Thank you - that was what I missed 🙏 👍 Thank you for fixing the tests!

Do you think it would be better to accept this pointer error as it used to be without rebuilding the xref table?

I fear that we might now reject / fail to parse a lot of PDFs that we could parse beforehand. Fixing the xref table if we can is pretty nice - and raising an exception if it's broken as well 👍
The part that I'm worried about is broken PDFs, that we could still parse before.

[pubpub-zz]

I fear that we might now reject / fail to parse a lot of PDFs that we could parse beforehand. Fixing the xref table if we can is pretty nice - and raising an exception if it's broken as well 👍
The part that I'm worried about is broken PDFs, that we could still parse before.

So did I. I've extended to test to cover both nominal case and incorrect pointer. A loop has been detected and fixed in such cases.

[MartinThoma]

Thank you very much for all the time and effort you've put into this! I appreciate it!

[MartinThoma]

@pubpub-zz I've created a follow-up PR that improves this IMHO a little bit. Nothing substantial, but from a maintainers perspective important. I'll add the reasoning in a second: #830