fix: recover from a corrupted startxref pointer - #230
Conversation
Fixes firecrawl#228. A PDF whose startxref pointer has been corrupted to point at the wrong byte offset — a single flipped digit, which is what damaged writers emit in the wild — was entirely unprocessable: every entry point (classify_pdf, extract_pages_markdown, process_pdf) raised "Invalid PDF structure", even though the file's object data, real xref table, and trailer were all completely intact just past the wrong pointer. Both pypdf and pdfium recover from this by locating the real table directly instead of trusting the pointer; lopdf doesn't. Added a new repair candidate (alongside the existing missing-%%EOF-marker and stripped-leading-bytes repairs in repair_pdf_container_candidates): scan the buffer for the real, standalone `xref` keyword and append a corrected trailing `startxref`/`%%EOF` block. lopdf's own get_xref_start always reads the *last* `%%EOF` in the final 512 bytes of the buffer and the `startxref` value immediately before it, so the appended block transparently supersedes the corrupted one already in the file — no in-place byte surgery on content the original writer produced. Scoped to classic (non-stream) xref tables, matching the reported repro and the common case; a corrupted pointer into a cross-reference *stream* (`N 0 obj << /Type /XRef ...>>`, some PDF 1.5+ writers) would need the containing object's number, not just a byte offset — out of scope here. Verified against the issue's exact repro (a valid one-page PDF with a single corrupted byte in its startxref offset): before this fix, process_pdf/classify_pdf/extract_pages_markdown all raised "Invalid PDF structure"; after, both the page count and the real extracted text ("Order Detail Report by Account", "WIDGET ASSEMBLY", the dollar amount) come back correctly. New regression test added. Full suite (859 tests, 1 new) passes; cargo clippy --all-targets -- -D warnings unchanged at 28 pre-existing/unrelated errors.
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Addresses cubic-dev-ai's review of firecrawl#230. - P2 (correctness/safety): the recovery candidate trusted the last standalone "xref" token unconditionally, without confirming it's actually a cross-reference table. A coincidental "xref" substring inside unrelated content — a stream, a string, uncompressed metadata — could get "repaired" against a bogus offset, letting lopdf load successfully against garbage instead of returning a clean error: a real failure turned into silent data corruption on the fallback path. Added looks_like_xref_subsection_header, which confirms a plausible classic xref subsection header (`<start-id> <count>`, e.g. "0 6" — the shape every real classic table starts with) actually follows the candidate token before accepting it. find_last_valid_xref_table_start now walks backward from the end of the buffer until it finds a token that both stands alone *and* validates, rather than accepting the first (rightmost) standalone match unconditionally. - P2 (performance): the old scan re-invoked `buf[..search_end].windows(4).rposition(...)` on a shrinking prefix every time a candidate token failed the boundary check, which is quadratic on a pathological buffer with many non-standalone "xref" occurrences. Rewrote as a single reverse byte-index walk — O(n) regardless of how many false candidates it has to reject along the way. Added direct unit tests on the byte-level scan (more precise than constructing adversarial full PDFs, and the coincidental-match scenario can't be represented in an integration-test fixture anyway since reportlab compresses page content by default): a coincidental standalone "xref" with no subsection header is rejected; a real classic table is found; a coincidental match positioned *after* the real table in the buffer doesn't shadow it; "xref" as a substring of "startxref" still doesn't match. The original firecrawl#228 repro (corrupted startxref pointer, real table otherwise intact) is unaffected — verified manually in addition to the existing integration test. Full suite (863 tests, 5 new) passes; cargo clippy --all-targets -- -D warnings unchanged at 28 pre-existing/unrelated errors.
|
Both confirmed and fixed. P2 — trusting any standalone "xref" unconditionally: Added `looks_like_xref_subsection_header`, which confirms a plausible classic xref subsection header (` `, e.g. "0 6" — the shape every real table starts with) actually follows the candidate token before accepting it. The scan now walks backward until it finds a token that both stands alone and validates, instead of accepting the first (rightmost) standalone match unconditionally. P2 — quadratic worst case: Rewrote as a single reverse byte-index walk — O(n) regardless of how many false candidates get rejected along the way, instead of re-invoking Added direct unit tests on the byte-level scan rather than trying to construct an adversarial full PDF for the coincidental-match case — that scenario can't actually be represented in a PDF-level integration test anyway, since reportlab (and most real writers) compress page content by default, so a literal "xref" word in visible text never appears as raw bytes in the file. Tests cover: a coincidental standalone "xref" with no subsection header is rejected; a real classic table is found; a coincidental match positioned after the real table doesn't shadow it; "xref" as a substring of "startxref" still doesn't match. Full suite: 863 tests passing (5 new), clippy unchanged at the same 28 pre-existing/unrelated baseline. The original #228 repro is unaffected — reverified manually alongside the existing integration test. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
looks_like_xref_subsection_header validated that a count run of digits followed the whitespace separator, but never checked what came after it. A coincidental "xref\n0 6garbage" in stream/literal content would still validate as a real subsection header shape and get repaired against a bogus offset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Confirmed and fixed in the latest commit. P2 — count run not bounded: Added |
Fixes #228.
Summary
A PDF whose
startxrefpointer has been corrupted to point at the wrong byte offset — a single flipped digit, which is what damaged writers emit in the wild — was entirely unprocessable: every entry point (classify_pdf,extract_pages_markdown,process_pdf) raisedInvalid PDF structure, even though the file's object data, real xref table, and trailer were all completely intact just past the wrong pointer. Bothpypdfandpdfiumrecover from this by locating the real table directly instead of trusting the pointer;lopdfdoesn't.Fix
Added a new repair candidate alongside the existing missing-
%%EOF-marker and stripped-leading-bytes repairs already inrepair_pdf_container_candidates: scan the buffer for the real, standalonexrefkeyword and append a corrected trailingstartxref/%%EOFblock.lopdf's ownget_xref_startalways reads the last%%EOFin the final 512 bytes of the buffer and thestartxrefvalue immediately before it, so the appended block transparently supersedes the corrupted one already in the file — no in-place byte surgery on content the original writer produced.Scoped to classic (non-stream) xref tables, matching the reported repro and the common case. A corrupted pointer into a cross-reference stream (
N 0 obj << /Type /XRef ...>>, used by some PDF 1.5+ writers instead) would need the containing object's number, not just a byte offset — out of scope here, happy to follow up separately if useful.Verification
Reproduced the issue's exact repro (a valid one-page PDF, then corrupt the
startxrefoffset to point at the wrong byte):process_pdf/classify_pdf/extract_pages_markdownInvalid PDF structureon all threeAdded a regression test using a fixture built the same way as the issue's repro script.
Testing
cargo test: 859 tests pass (1 new)cargo clippy --all-targets -- -D warnings: unchanged at 28 pre-existing/unrelated errors versusmainNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Recover PDFs with a corrupted
startxrefby scanning for the real classicxreftable and appending a correct trailer. The scan validates the table header (including count termination) and runs in a single reverse pass, fixing “Invalid PDF structure” errors inprocess_pdf,classify_pdf, andextract_pages_markdown.xref(with a valid<start-id> <count>header) and appendsstartxref/%%EOF, ensuringlopdfuses the corrected trailer.startxref, or a count with trailing garbage likexref\n0 6garbage) to avoid false positives.pypdf/pdfium.Written for commit 4a03110. Summary will update on new commits.