From ecffce69eeb3f102206f142f38f3ee4e66058933 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sun, 18 Oct 2026 22:47:28 +0800 Subject: [PATCH] fix(engine): Save writes a repaired file in full even without edits (#1937) (#2029) * fix(engine): Save writes a repaired file in full even without edits (#1937) A damaged file is repaired on open (its cross-reference index rebuilt from object headers, as Document Properties reports), but Save wrote it back as it was: `Session::save_bytes` returned the bytes as opened whenever nothing was edited, and a repair is not an edit. The "repaired" file on disk was still the damaged one. `save_bytes` now writes a reconstructed document in full even with no edits. `Document::reconstructed()` in cos names the condition (no revision chain: rebuilt, or built from scratch), and `write_incremental` uses it too, so the writer and the engine share one definition. Tests: `engine::tests::a_repaired_file_is_rewritten_by_save_even_without_edits` (table offsets shifted; no table at all) and `automation::saving_a_repaired_file_puts_the_repair_on_disk` (doc_open, doc_info, doc_save over the same path, reopen). Both fail with the condition removed: the saved bytes equal the damaged file. parity: core.repair-broken-files cites them and notes that a repair still does not count as an unsaved change. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz * fix(engine): a signed repaired file keeps its bytes on Save without edits (#1937) Review follow-up: writing a reconstructed file in full on Save puts the repair on disk, but for a signed file it would also invalidate its signatures without warning. Save now skips the early return only when the document is reconstructed and not signed. A regression test signs a document, appends a `startxref` that points nowhere (the signed bytes stay intact, the cross-reference data must be rebuilt) and checks that Save without edits returns those bytes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz --------- Co-authored-by: Claude Fable 5.1 --- crates/automation/tests/automation.rs | 24 +++++++++++ crates/cos/src/document.rs | 8 ++++ crates/cos/src/writer.rs | 2 +- crates/engine/src/lib.rs | 7 +++- crates/engine/src/tests.rs | 57 +++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +- 6 files changed, 98 insertions(+), 4 deletions(-) diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index 30658fdfd7..683b626862 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -214,6 +214,30 @@ fn page_transitions_are_set_and_removed_through_the_tools() { ); } +/// #1937 through the tools: a damaged file is repaired on open (`doc_info` lists the repair) and +/// `doc_save` over it writes the rebuilt file, so opening it again needs no repair. +#[test] +fn saving_a_repaired_file_puts_the_repair_on_disk() { + let dir = workdir("repaired-save"); + let good = fixture(2); + let mut broken = b"%PDF-1.7\n%shifted\n".to_vec(); + broken.extend_from_slice(&good[b"%PDF-1.7\n".len()..]); + std::fs::write(dir.join("broken.pdf"), &broken).unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "broken.pdf" }))["doc"].as_u64().unwrap(); + let info = ok(&mut a, "doc_info", json!({ "doc": doc })); + assert!(info["repairs"].as_array().unwrap().iter().any(|r| r.as_str().unwrap_or("").contains("rebuilt")), "{}", info["repairs"]); + let saved = ok(&mut a, "doc_save", json!({ "doc": doc })); + assert_eq!(saved["incremental"], false, "{saved}"); + let bytes = std::fs::read(dir.join("broken.pdf")).unwrap(); + assert_ne!(bytes, broken, "the damaged bytes were written back"); + let reopened = pdfcraft_cos::Document::open(std::sync::Arc::new(bytes)).unwrap(); + assert!(reopened.repair_log().is_empty(), "{:?}", reopened.repair_log()); + let info = ok(&mut a, "doc_info", json!({ "doc": doc })); + assert_eq!(info["repairs"].as_array().unwrap().len(), 0, "{}", info["repairs"]); + assert_eq!(info["pages"].as_array().unwrap().len(), 2); +} + #[test] fn autotag_preview_proposes_text_without_changing_the_pdf() { let dir = workdir("autotag-preview"); diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index e943555905..a8a561e7d5 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -857,6 +857,14 @@ impl Document { &self.revisions } + /// There is no cross-reference chain to append to: the index was rebuilt from object + /// headers because the file's own was unusable (see [`Self::repair_log`]), or the document + /// was built from scratch ([`Self::new_empty`]). A save writes such a document in full, even + /// when nothing was edited: for a repaired file that is what puts the repair on disk. + pub fn reconstructed(&self) -> bool { + self.revisions.is_empty() + } + /// 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 diff --git a/crates/cos/src/writer.rs b/crates/cos/src/writer.rs index f8d8fd3d80..ecd5dc2bc8 100644 --- a/crates/cos/src/writer.rs +++ b/crates/cos/src/writer.rs @@ -183,7 +183,7 @@ pub fn write_incremental(doc: &Document, opts: &SaveOptions) -> Result, } // Reconstructed files have no chain to append to; added or removed encryption must // rewrite every object; redaction must not leave the old revision behind. All need a full save. - if doc.revisions().is_empty() || doc.encryption_changed() || doc.full_save_required() { + if doc.reconstructed() || doc.encryption_changed() || doc.full_save_required() { return write_full(doc, opts); } let mut doc = doc.clone(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 147fc634ec..07ba5503ca 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -4078,11 +4078,16 @@ impl Session { /// The bytes to write for Save: an incremental update of the file as opened/last saved, /// with `/ModDate` stamped. Call `mark_saved` after writing them successfully. + /// + /// A file whose cross-reference data had to be rebuilt is written in full even when nothing + /// was edited: its bytes as opened are the damaged file, and this save is how the repair the + /// user was told about reaches the disk (#1937). Not a signed one: a full rewrite would + /// break its signatures, so with no edits it keeps its bytes, damage and all. pub fn save_bytes(&self, id: DocId) -> Result>, EditError> { let doc = self.get(id).ok_or(EditError::NoDocument)?; let Some(editor) = doc.editor.as_ref() else { return Ok(doc.bytes.clone()) }; let on_saved = Arc::ptr_eq(editor.cos.bytes(), &editor.saved); - if !editor.cos.is_modified() && on_saved { + if !editor.cos.is_modified() && on_saved && (!editor.cos.reconstructed() || doc.is_signed()) { return Ok(editor.saved.clone()); } let opts = SaveOptions { mod_date: self.now().map(pdfcraft_cos::pdf_date), ..SaveOptions::default() }; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1c473b7807..74f4e51c5e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -519,6 +519,63 @@ fn save_is_incremental_stamps_mod_date_and_rebases() { assert_eq!(reopened.revisions().len(), 3); } +/// #1937: a file whose cross-reference data had to be rebuilt saved as its original bytes when +/// nothing was edited, so the damage Document Properties reported as repaired was still in the +/// file. Save writes the rebuilt file in full; reopened, it needs no repair. +#[test] +fn a_repaired_file_is_rewritten_by_save_even_without_edits() { + let good = fixture(2); + // Every offset in the table is off by the inserted line: the chain reads but leads nowhere. + let mut shifted = b"%PDF-1.7\n%shifted\n".to_vec(); + shifted.extend_from_slice(&good[b"%PDF-1.7\n".len()..]); + // No cross-reference table at all. + let mut lost = good.clone(); + lost.truncate(good.windows(9).rposition(|w| w == b"startxref").unwrap()); + lost.extend_from_slice(b"startxref\n999999\n%%EOF\n"); + for (name, damaged) in [("shifted", shifted), ("lost", lost)] { + let mut s = Session::new().with_clock(|| 1_700_000_000); + let id = s.open("broken.pdf", Some("/tmp/broken.pdf".into()), Arc::new(damaged.clone()), None).unwrap(); + let doc = s.get(id).unwrap(); + assert!(doc.repair_log().iter().any(|l| l.contains("rebuilt cross-reference index")), "{name}: {:?}", doc.repair_log()); + assert_eq!(doc.info.pages.len(), 2, "{name}"); + assert!(!doc.dirty, "{name}: a repair is not an edit"); + let saved = s.save_bytes(id).unwrap(); + assert_ne!(&saved[..], &damaged[..], "{name}: the damaged bytes are not what Save writes"); + let reopened = pdfcraft_cos::Document::open(saved.clone()).unwrap(); + assert!(reopened.repair_log().is_empty(), "{name}: {:?}", reopened.repair_log()); + assert_eq!(reopened.revisions().len(), 1, "{name}"); + s.mark_saved(id, saved.clone(), None).unwrap(); + let doc = s.get(id).unwrap(); + assert!(doc.repair_log().is_empty(), "{name}: {:?}", doc.repair_log()); + assert_eq!(doc.info.pages.len(), 2, "{name}"); + // Sound now: saving again without edits writes the same bytes. + assert_eq!(s.save_bytes(id).unwrap(), saved, "{name}"); + } +} + +/// A damaged file that is signed keeps its bytes on a Save without edits: rewriting it in full +/// to put the repair on disk would invalidate its signatures without a word. +#[test] +fn a_signed_repaired_file_keeps_its_bytes_when_saved_without_edits() { + let p12 = std::fs::read(concat!(env!("CARGO_MANIFEST_DIR"), "/../sign/tests/data/ec-p256.p12")).unwrap(); + let digital_id = sign::pkcs12::open(&p12, "test").unwrap(); + // After the test certificate's start date (2026-10-02). + let mut s = Session::new().with_clock(|| 1_800_000_000); + let id = s.open("fixture.pdf", None, Arc::new(fixture(1)), None).unwrap(); + let opts = SignOptions { page: 0, rect: Some([20.0, 20.0, 180.0, 60.0]), ..SignOptions::default() }; + let signed = s.sign(id, &digital_id, opts).unwrap(); + // A trailing `startxref` that points nowhere: the signed revision's bytes are untouched, but + // the file's cross-reference data has to be rebuilt. + let mut damaged = signed.to_vec(); + damaged.extend_from_slice(b"\nstartxref\n999999\n%%EOF\n"); + let mut t = Session::new().with_clock(|| 1_800_000_000); + let tid = t.open("damaged.pdf", Some("/tmp/damaged.pdf".into()), Arc::new(damaged.clone()), None).unwrap(); + let doc = t.get(tid).unwrap(); + assert!(doc.repair_log().iter().any(|l| l.contains("rebuilt cross-reference index")), "{:?}", doc.repair_log()); + assert!(doc.is_signed()); + assert_eq!(&t.save_bytes(tid).unwrap()[..], &damaged[..], "a signed file is not rewritten without edits"); +} + #[test] fn undo_still_works_after_save() { let (mut s, id) = session_with(2); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 3e2b2efaef..deb0d9471d 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -46,8 +46,8 @@ milestone = "M1" status = "shipped" commands = ["file.open"] tools = ["doc_open"] -evidence = ["crates/cos/src/document.rs::damaged_xref_is_reconstructed", "crates/cos/src/parser.rs::indirect_stream_with_wrong_length_recovers", "crates/cos/src/document.rs::self_referential_length_does_not_overflow"] -notes = "" +evidence = ["crates/cos/src/document.rs::damaged_xref_is_reconstructed", "crates/cos/src/parser.rs::indirect_stream_with_wrong_length_recovers", "crates/cos/src/document.rs::self_referential_length_does_not_overflow", "crates/engine/src/tests.rs::a_repaired_file_is_rewritten_by_save_even_without_edits", "crates/automation/tests/automation.rs::saving_a_repaired_file_puts_the_repair_on_disk", "crates/engine/src/tests.rs::a_signed_repaired_file_keeps_its_bytes_when_saved_without_edits"] +notes = "Save and doc_save write a reconstructed file in full even when nothing was edited, so the repair reaches the disk and the saved file opens without one (#1937); a signed one keeps its bytes, so its signatures stay valid. A repair does not mark the document as having unsaved changes, so closing it does not offer to save the repaired file" [[feature]] id = "core.malformed-input-safety"