fix: recover from a corrupted startxref pointer (#230)
* fix: recover from a corrupted startxref pointer Fixes #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. * fix: validate xref table shape and scan in a single reverse pass Addresses cubic-dev-ai's review of #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 #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. * fix: reject xref subsection count runs with trailing garbage 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> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Abimael Martell <1450169+abimaelmartell@users.noreply.github.com>
This commit is contained in:
MADENIYOU
co-authored by
Claude Sonnet 5
Abimael Martell
Claude Sonnet 5
Abimael Martell
parent
371de80b14
commit
585d36e6a6
+163
@@ -3553,6 +3553,7 @@ fn repair_pdf_container_candidates(buf: &[u8]) -> Vec<Vec<u8>> {
|
||||
let mut candidates = Vec::new();
|
||||
|
||||
add_repair_candidate(&mut candidates, append_missing_eof_marker(buf), buf);
|
||||
add_repair_candidate(&mut candidates, recover_startxref_pointer(buf), buf);
|
||||
|
||||
let stripped = strip_leading_pdf_container_bytes(buf);
|
||||
if let Some(stripped_buf) = stripped.as_deref() {
|
||||
@@ -3562,11 +3563,112 @@ fn repair_pdf_container_candidates(buf: &[u8]) -> Vec<Vec<u8>> {
|
||||
append_missing_eof_marker(stripped_buf),
|
||||
buf,
|
||||
);
|
||||
add_repair_candidate(
|
||||
&mut candidates,
|
||||
recover_startxref_pointer(stripped_buf),
|
||||
buf,
|
||||
);
|
||||
}
|
||||
|
||||
candidates
|
||||
}
|
||||
|
||||
/// Some PDF writers emit a `startxref` pointer that doesn't actually point
|
||||
/// at the cross-reference table — a single corrupted byte in the offset is
|
||||
/// enough. lopdf trusts that pointer outright and fails to load rather than
|
||||
/// searching for the real table, unlike pypdf/pdfium which both recover by
|
||||
/// locating it directly. This finds the real (classic, non-stream) `xref`
|
||||
/// table by scanning for the keyword — validating that a plausible
|
||||
/// subsection header follows, not just any standalone "xref" token, since
|
||||
/// this crate processes untrusted input and a coincidental match inside
|
||||
/// unrelated stream/string content must not get "repaired" against a bogus
|
||||
/// offset (lopdf would then load successfully against garbage instead of
|
||||
/// returning a clean error) — and appends a corrected trailing
|
||||
/// `startxref`/`%%EOF` block. lopdf's own `get_xref_start` always uses the
|
||||
/// *last* `%%EOF` in the final 512 bytes of the buffer, so ours
|
||||
/// transparently supersedes the broken one without needing to touch
|
||||
/// anything already in the file.
|
||||
///
|
||||
/// Doesn't cover cross-reference *streams* (`N 0 obj << /Type /XRef ...`,
|
||||
/// used by some PDF 1.5+ writers instead of a classic table) — recovering
|
||||
/// those needs the containing object's number, not just a byte offset.
|
||||
fn recover_startxref_pointer(buf: &[u8]) -> Option<Vec<u8>> {
|
||||
let xref_pos = find_last_valid_xref_table_start(buf)?;
|
||||
|
||||
let mut repaired = Vec::with_capacity(buf.len() + 32);
|
||||
repaired.extend_from_slice(buf);
|
||||
if !repaired.ends_with(b"\n") {
|
||||
repaired.push(b'\n');
|
||||
}
|
||||
repaired.extend_from_slice(format!("startxref\n{xref_pos}\n%%EOF\n").as_bytes());
|
||||
Some(repaired)
|
||||
}
|
||||
|
||||
/// Finds the last standalone `xref` token in `buf` that is immediately
|
||||
/// followed by a plausible classic cross-reference subsection header
|
||||
/// (`<start-id> <count>`, e.g. "0 6") — the shape every real classic xref
|
||||
/// table starts with. A single reverse byte scan: O(n) even on a
|
||||
/// pathological buffer with many non-matching or non-standalone "xref"
|
||||
/// occurrences, unlike repeatedly re-searching a shrinking prefix.
|
||||
fn find_last_valid_xref_table_start(buf: &[u8]) -> Option<usize> {
|
||||
const KEYWORD: &[u8] = b"xref";
|
||||
if buf.len() < KEYWORD.len() {
|
||||
return None;
|
||||
}
|
||||
let mut pos = buf.len() - KEYWORD.len();
|
||||
loop {
|
||||
if &buf[pos..pos + KEYWORD.len()] == KEYWORD {
|
||||
let before_ok = pos == 0 || buf[pos - 1].is_ascii_whitespace();
|
||||
let after_ok = buf
|
||||
.get(pos + KEYWORD.len())
|
||||
.is_none_or(|c| c.is_ascii_whitespace());
|
||||
if before_ok && after_ok && looks_like_xref_subsection_header(buf, pos + KEYWORD.len())
|
||||
{
|
||||
return Some(pos);
|
||||
}
|
||||
}
|
||||
if pos == 0 {
|
||||
return None;
|
||||
}
|
||||
pos -= 1;
|
||||
}
|
||||
}
|
||||
|
||||
/// Checks that `buf[pos..]` starts (after whitespace) with two
|
||||
/// whitespace-separated runs of ASCII digits — `<start-id> <count>`, the
|
||||
/// first subsection header of a classic PDF cross-reference table.
|
||||
fn looks_like_xref_subsection_header(buf: &[u8], pos: usize) -> bool {
|
||||
fn skip_ws(buf: &[u8], mut pos: usize) -> usize {
|
||||
while buf.get(pos).is_some_and(u8::is_ascii_whitespace) {
|
||||
pos += 1;
|
||||
}
|
||||
pos
|
||||
}
|
||||
fn skip_digits(buf: &[u8], mut pos: usize) -> usize {
|
||||
while buf.get(pos).is_some_and(u8::is_ascii_digit) {
|
||||
pos += 1;
|
||||
}
|
||||
pos
|
||||
}
|
||||
|
||||
let pos = skip_ws(buf, pos);
|
||||
let after_first_digits = skip_digits(buf, pos);
|
||||
if after_first_digits == pos {
|
||||
return false; // no start-id
|
||||
}
|
||||
let sep = skip_ws(buf, after_first_digits);
|
||||
if sep == after_first_digits {
|
||||
return false; // start-id and count must be whitespace-separated
|
||||
}
|
||||
let after_count = skip_digits(buf, sep);
|
||||
if after_count == sep {
|
||||
return false; // no count
|
||||
}
|
||||
// The count run must end at whitespace/buffer-end, not run into trailing
|
||||
// garbage (e.g. a coincidental "xref\n0 6garbage" in stream content).
|
||||
buf.get(after_count).is_none_or(u8::is_ascii_whitespace)
|
||||
}
|
||||
|
||||
fn add_repair_candidate(
|
||||
candidates: &mut Vec<Vec<u8>>,
|
||||
candidate: Option<Vec<u8>>,
|
||||
@@ -7099,4 +7201,65 @@ mod tests {
|
||||
// Pre-filled cell was not touched.
|
||||
assert_eq!(cells[1].text, "Pre-filled");
|
||||
}
|
||||
|
||||
// -- recover_startxref_pointer / find_last_valid_xref_table_start ------
|
||||
//
|
||||
// Direct unit tests on the byte-level scan, addressing review feedback
|
||||
// on #230: a coincidental standalone "xref" token that isn't actually
|
||||
// followed by a subsection header (start-id + count) must not be
|
||||
// treated as a real table — accepting it would let lopdf "succeed"
|
||||
// against a bogus offset and silently return garbled/empty content
|
||||
// instead of a clean error.
|
||||
|
||||
#[test]
|
||||
fn find_xref_rejects_standalone_token_without_subsection_header() {
|
||||
// "xref" appears as a real standalone word, but nothing that looks
|
||||
// like "<start-id> <count>" follows it.
|
||||
let buf = b"Please refer to the xref appendix for details.";
|
||||
assert_eq!(find_last_valid_xref_table_start(buf), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_xref_accepts_real_classic_table_header() {
|
||||
let buf = b"garbage\nxref\n0 6\n0000000000 65535 f \n%%EOF";
|
||||
let pos = find_last_valid_xref_table_start(buf).expect("should find the real table");
|
||||
assert_eq!(&buf[pos..pos + 4], b"xref");
|
||||
assert_eq!(&buf[pos..], b"xref\n0 6\n0000000000 65535 f \n%%EOF");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_xref_skips_coincidental_match_and_finds_real_table_before_it() {
|
||||
// A coincidental "xref" (no subsection header) appears *after* the
|
||||
// real table in the buffer — the scan must not stop at the first
|
||||
// (rightmost) standalone token it finds; it must keep looking
|
||||
// backward until one actually validates.
|
||||
let buf = b"xref\n0 3\n0000000000 65535 f \ntrailer\nsee the xref\n";
|
||||
let pos = find_last_valid_xref_table_start(buf).expect("should find the real table");
|
||||
assert_eq!(pos, 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_xref_rejects_substring_of_startxref() {
|
||||
// "xref" is a substring of "startxref" but isn't a standalone
|
||||
// token there (not preceded by whitespace) — must not match, even
|
||||
// though a number immediately follows it.
|
||||
let buf = b"startxref\n1234\n%%EOF";
|
||||
assert_eq!(find_last_valid_xref_table_start(buf), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_xref_rejects_count_run_with_trailing_garbage() {
|
||||
// "xref\n0 6garbage" has the right shape (digits, whitespace,
|
||||
// digits) but the count run doesn't end at whitespace/EOF — it
|
||||
// runs straight into non-digit garbage, so this must not be
|
||||
// accepted as a real subsection header.
|
||||
let buf = b"xref\n0 6garbage\n%%EOF";
|
||||
assert_eq!(find_last_valid_xref_table_start(buf), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recover_startxref_pointer_returns_none_without_a_valid_table() {
|
||||
let buf = b"Please refer to the xref appendix for details.";
|
||||
assert!(recover_startxref_pointer(buf).is_none());
|
||||
}
|
||||
}
|
||||
|
||||
+68
@@ -0,0 +1,68 @@
|
||||
%PDF-1.3
|
||||
%“Œ‹ž ReportLab Generated PDF document (opensource)
|
||||
1 0 obj
|
||||
<<
|
||||
/F1 2 0 R
|
||||
>>
|
||||
endobj
|
||||
2 0 obj
|
||||
<<
|
||||
/BaseFont /Helvetica /Encoding /WinAnsiEncoding /Name /F1 /Subtype /Type1 /Type /Font
|
||||
>>
|
||||
endobj
|
||||
3 0 obj
|
||||
<<
|
||||
/Contents 7 0 R /MediaBox [ 0 0 612 792 ] /Parent 6 0 R /Resources <<
|
||||
/Font 1 0 R /ProcSet [ /PDF /Text /ImageB /ImageC /ImageI ]
|
||||
>> /Rotate 0 /Trans <<
|
||||
|
||||
>>
|
||||
/Type /Page
|
||||
>>
|
||||
endobj
|
||||
4 0 obj
|
||||
<<
|
||||
/PageMode /UseNone /Pages 6 0 R /Type /Catalog
|
||||
>>
|
||||
endobj
|
||||
5 0 obj
|
||||
<<
|
||||
/Author (anonymous) /CreationDate (D:20260803112923+00'00') /Creator (anonymous) /Keywords () /ModDate (D:20260803112923+00'00') /Producer (ReportLab PDF Library - \(opensource\))
|
||||
/Subject (unspecified) /Title (untitled) /Trapped /False
|
||||
>>
|
||||
endobj
|
||||
6 0 obj
|
||||
<<
|
||||
/Count 1 /Kids [ 3 0 R ] /Type /Pages
|
||||
>>
|
||||
endobj
|
||||
7 0 obj
|
||||
<<
|
||||
/Filter [ /ASCII85Decode /FlateDecode ] /Length 202
|
||||
>>
|
||||
stream
|
||||
GarW05mr9@&;9NOME,dW.,B;'jAjYq0S4Z`*D9aMA;]$5J)A/$3lESen1?F)ZJsa4$4&N%-%cs)#qW5EVhhbPiRDrAV>MC%.spto@CU"ZdipR'TtFiMR_%m*Hm$N%qL7a"ckkp9T/s[N2"Og377mP*M^akb2XQZ@'l*qT(9bVtDb5+)S&Q.#%E)<]Ao`TSk2AE'/E\fn~>endstream
|
||||
endobj
|
||||
xref
|
||||
0 8
|
||||
0000000000 65535 f
|
||||
0000000061 00000 n
|
||||
0000000092 00000 n
|
||||
0000000199 00000 n
|
||||
0000000392 00000 n
|
||||
0000000460 00000 n
|
||||
0000000721 00000 n
|
||||
0000000780 00000 n
|
||||
trailer
|
||||
<<
|
||||
/ID
|
||||
[<6d7ea1213c5974c78613d5d2a08423b5><6d7ea1213c5974c78613d5d2a08423b5>]
|
||||
% ReportLab generated PDF document -- digest (opensource)
|
||||
|
||||
/Info 5 0 R
|
||||
/Root 4 0 R
|
||||
/Size 8
|
||||
>>
|
||||
startxref
|
||||
9072
|
||||
%%EOF
|
||||
@@ -3996,6 +3996,29 @@ fn pdf_options_debug_redacts_password() {
|
||||
assert!(dbg.contains("REDACTED"), "expected redaction marker: {dbg}");
|
||||
}
|
||||
|
||||
/// Regression for #228: a `startxref` pointer corrupted to point at the
|
||||
/// wrong byte offset (a single flipped digit — a real, common writer bug)
|
||||
/// must not make the whole file unprocessable. The real classic xref table
|
||||
/// is still present and findable by scanning for the `xref` keyword; both
|
||||
/// pypdf and pdfium recover the same way. Before this fix, every entry
|
||||
/// point raised "Invalid PDF structure" on a file whose object data was
|
||||
/// otherwise completely intact.
|
||||
#[test]
|
||||
fn test_process_pdf_recovers_corrupted_startxref_pointer() {
|
||||
let result = process_pdf_with_options(
|
||||
"tests/fixtures/broken_startxref_pointer.pdf",
|
||||
PdfOptions::new(),
|
||||
)
|
||||
.expect("a corrupted startxref pointer should be recoverable, like pypdf/pdfium");
|
||||
|
||||
assert_eq!(result.page_count, 1);
|
||||
let md = result.markdown.unwrap_or_default();
|
||||
assert!(
|
||||
md.contains("Order Detail Report by Account") && md.contains("WIDGET ASSEMBLY"),
|
||||
"recovered document should extract its real text, got: {md:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression for #227: `extract_pages_markdown`'s per-page `needs_ocr`
|
||||
/// must agree with `classify_pdf`/`detect_pdf_type` on the same page. The
|
||||
/// fixture is a full-page raster "scan" with a single line of genuine
|
||||
|
||||
Reference in New Issue
Block a user