From 33fe4880611288f659bbfae75e14865002a04d97 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 04:20:41 +0000 Subject: [PATCH] fix(cos): an incremental save of a file with bytes before its header counts from the header Some files carry bytes before `%PDF-`, and their offsets count from the header rather than from the first byte. We read those, retrying a section relative to the header when the absolute offset misses, but the incremental writer then appended a section whose offsets and `startxref` were absolute while its `/Prev` kept the header-relative value it was given. qpdf reported "xref not found" and reconstructed the file, so a save that reported success left a damaged-looking update behind (#2364, found with KillerPDF's reserve3.pdf and a Subject edit). The reader now records on each `Revision` whether it was found counting from the header, and the writer counts the new section the same way as the one its `/Prev` points to. A file whose offsets already include the prefix keeps absolute offsets, which the second test pins so the fix cannot overcorrect. Seen red with the writer change reverted: the appended section's `startxref` missed its `xref`. Fixes storytold/pdfcraft#2364 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DsT1y8xBvkXeBxEUQqdDtj --- crates/cos/src/document.rs | 25 ++++++++++----- crates/cos/src/writer.rs | 6 +++- crates/cos/tests/writer.rs | 63 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 9 deletions(-) diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index a0b1f56ee..1c82e500d 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -307,6 +307,10 @@ pub struct Revision { pub xref_offset: u64, /// `true` if the section is a cross-reference stream. pub is_stream: bool, + /// `true` if `xref_offset` counts from the `%PDF-` header rather than from the start of the + /// file. The two differ only in a file with bytes before its header, and an update appended + /// to such a file must count the same way as the section it points back to. + pub header_relative: bool, } #[derive(Clone, Debug)] @@ -817,6 +821,11 @@ impl Document { &self.revisions } + /// Position of the `%PDF-` header in the file: 0 unless bytes come before it. + pub fn header_offset(&self) -> usize { + self.header_offset + } + /// Where each revision of the file ends (oldest first): the byte after the `%%EOF` (and its /// end-of-line) that closes its cross-reference section. `data[..end]` is that revision as it /// was saved. A linearized file's first-page section belongs to the revision it precedes, so @@ -1345,13 +1354,13 @@ impl Document { log.push(format!("cross-reference chain loops at offset {off}; stopped")); break; } - let (primary, sect_trailer, is_stream) = match self.read_compact_section(off as usize) { - Ok(Some(v)) => v, + let ((primary, sect_trailer, is_stream), header_relative) = match self.read_compact_section(off as usize) { + Ok(Some(v)) => (v, false), Ok(None) => return Ok(None), Err(e) if self.header_offset > 0 => { log.push(format!("xref at {off} unreadable ({e}); retrying relative to the header")); match self.read_compact_section(off as usize + self.header_offset)? { - Some(v) => v, + Some(v) => (v, true), None => return Ok(None), } } @@ -1359,7 +1368,7 @@ impl Document { // materialized reader logs it and the file is rebuilt from object headers (#1656). Err(e) => return Err(e), }; - revisions.push(Revision { xref_offset: off, is_stream }); + revisions.push(Revision { xref_offset: off, is_stream, header_relative }); let Some(rows) = retained_rows.checked_add(primary.row_count).filter(|rows| *rows <= MAX_COMPACT_XREF_ROWS) else { return Ok(None) }; let Some(bytes) = retained_bytes.checked_add(primary.retained_bytes()).filter(|bytes| *bytes <= MAX_COMPACT_XREF_BYTES) else { return Ok(None); @@ -1636,13 +1645,13 @@ impl Document { let mut sect = BTreeMap::new(); // Object numbers whose entry in this section was set aside ([`Self::xref_oddities`]). let mut odd = Vec::new(); - let (sect_trailer, is_stream) = match self.read_section(off as usize, &mut sect, &mut odd) { - Ok(v) => v, + let ((sect_trailer, is_stream), header_relative) = match self.read_section(off as usize, &mut sect, &mut odd) { + Ok(v) => (v, false), Err(e) if self.header_offset > 0 => { log.push(format!("xref at {off} unreadable ({e}); retrying relative to the header")); sect.clear(); odd.clear(); - self.read_section(off as usize + self.header_offset, &mut sect, &mut odd)? + (self.read_section(off as usize + self.header_offset, &mut sect, &mut odd)?, true) } Err(e) => { // An unreadable older section loses every object only it locates (the page @@ -1653,7 +1662,7 @@ impl Document { return Err(e); } }; - revisions.push(Revision { xref_offset: off, is_stream }); + revisions.push(Revision { xref_offset: off, is_stream, header_relative }); // Hybrid files (§7.5.8.4): the /XRefStm entries supplement the table and replace the // free entries it gives objects only a cross-reference stream can locate. if let Some(x) = sect_trailer.int(b"XRefStm").filter(|x| *x >= 0) { diff --git a/crates/cos/src/writer.rs b/crates/cos/src/writer.rs index f8d8fd3d8..69442ccea 100644 --- a/crates/cos/src/writer.rs +++ b/crates/cos/src/writer.rs @@ -193,7 +193,11 @@ pub fn write_incremental(doc: &Document, opts: &SaveOptions) -> Result, // allocated once at its exact size: growing a buffer that holds a large original past a // guessed capacity would copy the whole file again and keep up to as much again unused. let newline = !matches!(original.last(), Some(b'\n' | b'\r')); - let base = (original.len() + usize::from(newline)) as u64; + // A file with bytes before its `%PDF-` header may count its offsets from the header. The new + // section then has to as well: readers take `/Prev` and `startxref` in one convention, and a + // mix sends them into reconstruction (#2364). + let shift = if doc.revisions().last().is_some_and(|r| r.header_relative) { doc.header_offset() } else { 0 }; + let base = (original.len() + usize::from(newline)).saturating_sub(shift) as u64; let mut out = Vec::new(); let mut offsets: BTreeMap = BTreeMap::new(); // num → (offset, gen, in use) for (num, generation, obj) in doc.overlay_entries() { diff --git a/crates/cos/tests/writer.rs b/crates/cos/tests/writer.rs index ec18b13f3..b7a54aa16 100644 --- a/crates/cos/tests/writer.rs +++ b/crates/cos/tests/writer.rs @@ -392,3 +392,66 @@ fn full_save_preserves_unknown_trailer_data() { assert_eq!(trailer_note(&back), trailer_note(&doc)); assert_eq!(back.trailer().get(b"Version"), doc.trailer().get(b"Version")); } + +/// The `startxref` value at the end of `bytes`. +fn last_startxref(bytes: &[u8]) -> usize { + let at = bytes.windows(9).rposition(|w| w == b"startxref").unwrap(); + String::from_utf8_lossy(&bytes[at + 9..]).split_whitespace().next().unwrap().parse().unwrap() +} + +#[test] +fn incremental_saves_of_a_file_with_bytes_before_its_header_count_from_the_header() { + // Offsets written before the prefix was added count from `%PDF-`, as in the KillerPDF file + // of #2364; the appended section must count the same way as the `/Prev` it points back to. + let prefix = vec![b'#'; 128]; + let as_stream = write_full(&Document::open(Arc::new(fixture())).unwrap(), &SaveOptions::default()).unwrap(); + for (name, body, marker) in [("table", classic_table(), &b"xref"[..]), ("stream", as_stream, &b" 0 obj"[..])] { + let input = [prefix.as_slice(), &body].concat(); + let mut doc = Document::open(Arc::new(input.clone())).unwrap(); + assert!(doc.revisions().last().unwrap().header_relative, "{name}"); + doc.update_dict(ObjRef::new(1, 0), |d| d.set(b"Lang".to_vec(), Object::String(pdfcraft_cos::PdfString::literal(b"en".to_vec())))).unwrap(); + let saved = write_incremental(&doc, &SaveOptions::default()).unwrap(); + assert!(saved.starts_with(&input), "{name}"); + + let xref = last_startxref(&saved) + prefix.len(); + let section = saved.get(xref..).unwrap_or_default(); + assert!(section.windows(marker.len()).position(|w| w == marker).is_some_and(|p| p < 16), "{name}: startxref misses its section"); + let back = Document::open(Arc::new(saved.clone())).unwrap(); + assert_eq!(back.revisions().len(), doc.revisions().len() + 1, "{name}"); + assert!(back.revisions().iter().all(|r| r.header_relative), "{name}: one convention across the chain"); + let edited = back.get(ObjRef::new(1, 0)); + assert_eq!(edited.as_dict().unwrap().get(b"Lang").and_then(|o| o.as_string()).map(|s| s.bytes.as_slice()), Some(b"en".as_slice())); + assert!(back.xref_entry_is_exact(1), "{name}"); + assert_qpdf_clean(&saved); + } +} + +#[test] +fn incremental_saves_of_a_file_whose_offsets_count_from_its_first_byte_keep_counting_so() { + // The other convention: the same file, but its offsets include the bytes before the header. + let prefix = vec![b'#'; 128]; + let mut input = [prefix.as_slice(), &classic_table()].concat(); + let xref = input.windows(6).rposition(|w| w == b"\nxref\n").unwrap() + 1; + let table: String = String::from_utf8_lossy(&input[xref..]) + .lines() + .map(|line| match line.strip_suffix(" 00000 n ").and_then(|o| o.parse::().ok()) { + Some(o) => format!("{:010} 00000 n \n", o + prefix.len()), + None if line.parse::().is_ok() => format!("{xref}\n"), + None => format!("{line}\n"), + }) + .collect(); + input.truncate(xref); + input.extend_from_slice(table.as_bytes()); + + let mut doc = Document::open(Arc::new(input.clone())).unwrap(); + assert!(doc.repair_log().is_empty(), "{:?}", doc.repair_log()); + assert!(!doc.revisions().last().unwrap().header_relative); + doc.update_dict(ObjRef::new(1, 0), |d| d.set(b"Lang".to_vec(), Object::String(pdfcraft_cos::PdfString::literal(b"en".to_vec())))).unwrap(); + let saved = write_incremental(&doc, &SaveOptions::default()).unwrap(); + assert!(saved.get(last_startxref(&saved)..).unwrap_or_default().starts_with(b"xref")); + let back = Document::open(Arc::new(saved.clone())).unwrap(); + assert!(back.repair_log().is_empty(), "{:?}", back.repair_log()); + assert_eq!(back.revisions().len(), 2); + assert!(back.xref_entry_is_exact(1)); + assert_qpdf_clean(&saved); +}