Refuse to overwrite a document OverWrite() cannot read (mirrors mpdf/mpdf#747) - #42
Open
jakejackson1 wants to merge 1 commit into
Open
Refuse to overwrite a document OverWrite() cannot read (mirrors mpdf/mpdf#747)#42jakejackson1 wants to merge 1 commit into
jakejackson1 wants to merge 1 commit into
Conversation
) OverWrite() finds the cross-reference table, the page tree and the startxref with three patterns and then reads the captures without checking any of them matched. Given a document written with CRLF line endings - what the reporter had - none of them match. On PHP 8 that is "Undefined array key 1", then key 2, then a null handed to preg_match_all(), then two more; and then the method carries on and writes a cross-reference table built out of nothing. What comes back is a document with none of the text replaced and a table readers report as damaged and repair. Ghostscript says "xref table was repaired". Check each match and throw, naming the file and what could not be found. Also guard the per-object read inside the loop and the xref entry the offset bookkeeping needs, so a page tree pointing outside the table stops rather than producing offsets that do not add up. The documents this method can read are unaffected: text is still replaced on every page, compressed and not, and the table it writes still points at itself. All twenty snapshots are unchanged. mpdf#747 instead accepts \R as the line ending. Reading CRLF is not enough on its own - the method writes LF back and its offset arithmetic does not model the difference, so the table would still be wrong. Refusing is the outcome that leaves the caller with something they can act on. That PR's rewrite of the object loop is also not usable: it names two capture groups "length" in one pattern, which will not compile, and writes "enobj" and an undefined $newLen into the objects it rebuilds. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Add snapshot test to verify it can override |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This mirrors the intent of mpdf/mpdf#747, so the fork carries a fix while the upstream PR sits open. It closes mpdf/mpdf#626. The upstream patch itself is not usable — see below.
OverWrite()finds the cross-reference table, the page tree and thestartxrefwith three patterns, and reads the captures without checking that any of them matched:Given a document written with CRLF line endings — what the reporter had — none of them match. On PHP 8 that is
Undefined array key 1, then key 2, then a null handed topreg_match_all(), then two more from the other two sites. That is theUndefined Offset: 1 - mPDF.php Line 29198in the issue, and the screenshots under it.Then the method carries on. It writes a new cross-reference table out of the empty
$xref, and astartxrefcomputed from an empty$m. What comes back is a document with none of the text replaced and a table Ghostscript reports asxref table was repaired. A reader that does not repair gets nothing.Try it
Before: five PHP warnings, and 1,460 bytes of PDF with
MAIN HEADINGstill in it and a broken cross-reference table.After:
Mpdf\MpdfException: Cannot overwrite "crlf.pdf": no cross-reference table of the kind mPDF writes was found in it.Test plan
tests/Mpdf/OverWriteTest.php, six cases, each building its source document with mPDF and cleaning up after itself.testTextIsReplacedOnEveryPageandtestTextIsReplacedInACompressedDocument— the working path, uncompressed and compressed. Both pass ongravitypdf; they are the controls that nothing this method could already do has been taken away.testTheCrossReferenceTableStillPointsAtItself— reads thestartxrefback out of the result and checks the bytes at that offset arexref\n0. Passes on both.testADocumentWrittenWithOtherLineEndingsIsRefused— makes a CRLF copy of an mPDF document. Ongravitypdfthis returns a damaged file; here it throws.testADocumentFromSomewhereElseIsRefused—tests/data/pdfs/compressed-xref.pdf, a fixture already in the repository. Also returns something ongravitypdf.testRefusingRaisesNothingOfItsOwn— installs an error handler and asserts nothing at all was raised on the way to the exception. This is the one that holds the issue's actual report. Ongravitypdfthe handler collects five messages.OverWrite()rewrites a file that already exists and is not part of rendering; all twenty snapshot documents regenerated on this branch and ongravitypdfare byte-identical once the creation date and the random/IDare normalised.composer test— 1128 tests, 2652 assertions, up from 1122/2643.composer csclean.phpstanoutput identical togravitypdf.More info — why refusing rather than reading CRLF, and the state of the upstream patch
Why not just accept \R
mpdf/mpdf#747changes the four patterns to accept any line ending:Reading CRLF is not enough on its own. The method writes LF back —
$newstrand$newxrefare both built with"\n"— and its bookkeeping models the size change aswhich counts the stream and the length digits and nothing else. Every line ending it converted would move an offset the table names, uncounted, and the result would be the same repaired-on-open document by a different route. Refusing is the outcome that leaves the caller with something they can act on.
OverWrite()only ever worked on documents mPDF wrote itself — it needs<</Length N>>\nstream\nexactly as mPDF writes it — and mPDF writes LF.The rest of the upstream patch
Its rewrite of the object loop cannot run:
length, which PCRE refuses to compile without(?J);stream, which the body then reads;enobj, notendobj;{$newLen}, which is not a variable in scope —$newlenis.Its test extends
\PHPUnit_Framework_TestCaseand callsSetImportUse(), neither of which exists any more.The other guards
Two reads inside the loop are guarded here as well. The per-object
preg_matchresult is checked before$m[2]is read, and$xref[$obj + 1]is checked before the offset change is recorded against it — a page tree naming an object the table does not carry now stops, rather than rewriting the document with offsets that do not add up.