diff --git a/crates/organize/src/import.rs b/crates/organize/src/import.rs index 588539866..80f90fb2a 100644 --- a/crates/organize/src/import.rs +++ b/crates/organize/src/import.rs @@ -17,7 +17,7 @@ //! - layers (optional content groups) used by the pages are registered in the destination's //! `/OCProperties`, keeping their default on/off state. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use pdfcraft_cos::{Dict, Document, ObjRef, Object}; @@ -240,23 +240,31 @@ pub fn import_pages_with( at: usize, conflicts: FieldConflicts, ) -> Result { - let first_new = dst.object_numbers().last().map_or(1, |n| n + 1); + let before: HashSet = dst.object_numbers().into_iter().collect(); let imported = match import_pages_mapped(dst, src, src_pages, at, conflicts) { Ok(imported) => imported, Err(e) => { // A refusal while copying (a source too deep to follow) has only added objects: // take them back, so the destination is as it was. - for n in dst.object_numbers().into_iter().filter(|n| *n >= first_new) { - dst.free(ObjRef::new(n, dst.generation(n))); + for r in added_objects(dst, &before) { + dst.free(r); } return Err(e); } }; - let created: Vec = dst.object_numbers().into_iter().filter(|n| *n >= first_new).map(|n| ObjRef::new(n, dst.generation(n))).collect(); + let created = added_objects(dst, &before); crate::dedupe::dedupe_resources(dst, &created, true); Ok(ImportOutcome { pages: imported.pages, renamed_fields: imported.renamed_fields }) } +/// The objects in `dst` that are not among `before`, the numbers it held before an import: the +/// ones the import added. They are not always numbered above the others: when no number is left +/// above the highest (the document holds object 4294967295, or its `/Size` is exhausted), +/// [`Document::add`] fills free numbers below it. +fn added_objects(dst: &Document, before: &HashSet) -> Vec { + dst.object_numbers().into_iter().filter(|n| !before.contains(n)).map(|n| ObjRef::new(n, dst.generation(n))).collect() +} + /// Insert Pages from File: [`import_pages_with`], and the inserted file's bookmarks come along /// under one new bookmark titled `title` (the file's name) at its first inserted page, as Combine /// Files does for each file. That bookmark goes among the top-level bookmarks where the pages went: @@ -270,9 +278,9 @@ pub fn insert_file_pages( at: usize, conflicts: FieldConflicts, ) -> Result { - let first_new = dst.object_numbers().last().map_or(1, |n| n + 1); + let before: HashSet = dst.object_numbers().into_iter().collect(); let imported = import_pages_mapped(dst, src, src_pages, at, conflicts)?; - let created: Vec = dst.object_numbers().into_iter().filter(|n| *n >= first_new).map(|n| ObjRef::new(n, dst.generation(n))).collect(); + let created = added_objects(dst, &before); crate::dedupe::dedupe_resources(dst, &created, true); let outline = source_outline(src, imported.page_map); if let Some(first) = imported.pages.first() diff --git a/crates/organize/src/tests.rs b/crates/organize/src/tests.rs index 9bd3eb396..81123060a 100644 --- a/crates/organize/src/tests.rs +++ b/crates/organize/src/tests.rs @@ -2750,27 +2750,31 @@ fn combine_field_conflicts_previews_each_file_in_order() { assert_eq!(preview[2], [("Text1".to_string(), "Text1-2".to_string())], "the cascade continues past the earlier rename"); } +/// A one-page source whose page refers (`/Foo`) to object 6, followed by `tail` as objects 6, 7, … +fn chained(tail: &[String]) -> Document { + let mut b: Vec = vec![ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 400] >>".into(), + "<< /Type /Page /Parent 2 0 R /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Foo 6 0 R >>".into(), + body("C1"), + "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into(), + ]; + b.extend_from_slice(tail); + open(build(&b, "/Root 1 0 R")) +} + +/// [`chained`] with `n` objects, each referring to the next. +fn chain(n: usize) -> Document { + let mut tail: Vec = (0..n).map(|i| format!("<< /Next {} 0 R >>", 7 + i)).collect(); + tail.push("<< /End true >>".into()); + chained(&tail) +} + /// #1800: thousands of shallow objects, each referring to the next, used to recurse once per /// object and exhaust the stack. The copy is refused instead, and the destination is as it was; /// a chain within the budget and a short cycle copy as before. #[test] fn a_chain_of_objects_too_deep_to_copy_is_refused_and_leaves_the_destination_alone() { - fn chained(tail: &[String]) -> Document { - let mut b: Vec = vec![ - "<< /Type /Catalog /Pages 2 0 R >>".into(), - "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 400] >>".into(), - "<< /Type /Page /Parent 2 0 R /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Foo 6 0 R >>".into(), - body("C1"), - "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into(), - ]; - b.extend_from_slice(tail); - open(build(&b, "/Root 1 0 R")) - } - fn chain(n: usize) -> Document { - let mut tail: Vec = (0..n).map(|i| format!("<< /Next {} 0 R >>", 7 + i)).collect(); - tail.push("<< /End true >>".into()); - chained(&tail) - } // On a thread with less stack than any platform's main thread, so the budget is proven. let run = std::thread::Builder::new() .stack_size(512 << 10) @@ -2790,6 +2794,81 @@ fn a_chain_of_objects_too_deep_to_copy_is_refused_and_leaves_the_destination_alo assert!(run.is_ok(), "the copy must not overflow the stack"); } +/// The fixture after an update that adds object 20 and declares `/Size 4294967295`. No number is +/// left above that, so [`Document::add`] fills free numbers from 12 on, the gap below object 20 first. +fn fixture_with_an_exhausted_size() -> Document { + let mut bytes = fixture(); + let text = String::from_utf8_lossy(&bytes).into_owned(); + let prev: usize = text.rsplit("startxref").next().and_then(|t| t.split_whitespace().next()).and_then(|n| n.parse().ok()).expect("startxref"); + let added = bytes.len(); + bytes.extend_from_slice(b"20 0 obj\n<< /Unused true >>\nendobj\n"); + let xref = bytes.len(); + bytes.extend_from_slice( + format!( + "xref\n20 1\n{added:010} 00000 n \ntrailer\n<< /Size 4294967295 /Root 1 0 R /Info 11 0 R /Prev {prev} >>\nstartxref\n{xref}\n%%EOF\n" + ) + .as_bytes(), + ); + let doc = open(bytes); + assert!(doc.repair_log().iter().any(|l| l.contains("exhausted trailer")), "{:?}", doc.repair_log()); + doc +} + +/// The fixture holding object 4294967295, as a document does once an edit writes to an object of +/// that number (a damaged file's catalog, say). [`Document::add`] then fills free numbers below it. +fn fixture_holding_object_4294967295() -> Document { + let mut doc = open(fixture()); + doc.set(ObjRef::new(u32::MAX, 0), Object::String(PdfString::text("top"))); + doc +} + +/// Documents whose new objects are numbered in the gaps below their highest number. +fn documents_numbering_new_objects_below_their_highest() -> [(&'static str, Document); 2] { + [("an exhausted /Size", fixture_with_an_exhausted_size()), ("object 4294967295", fixture_holding_object_4294967295())] +} + +/// Duplicate, Replace and Insert Pages took the objects numbered above the destination's highest +/// number for the ones the import added. When new objects fill gaps below it, the copies escaped +/// resource sharing (each import stored the fonts again), and with object 4294967295 the number +/// after the highest overflowed: a panic in debug builds, and in release a wrap to 0 that counted +/// every object as added. The import now compares the object numbers before and after. +#[test] +fn an_import_shares_the_resources_it_added_wherever_they_are_numbered() { + let mut plain = open(fixture()); + duplicate_pages(&mut plain, &[0], None).unwrap(); + for (what, mut doc) in documents_numbering_new_objects_below_their_highest() { + let before = doc.object_numbers(); + duplicate_pages(&mut doc, &[0], None).unwrap(); + assert_eq!(labels(&doc), ["Page 1", "Page 1", "Page 2", "Page 3"], "{what}"); + assert_eq!(count_fonts(&doc), count_fonts(&plain), "{what}: the copy shares the font, as it does in the plain fixture"); + insert_file_pages(&mut doc, &open(fixture()), "Inserted", &[2], 0, FieldConflicts::Rename).unwrap(); + assert_eq!(labels(&doc), ["Page 3", "Page 1", "Page 1", "Page 2", "Page 3"], "{what}"); + assert_eq!(count_fonts(&doc), count_fonts(&plain), "{what}: the inserted page shares the font too"); + let after = doc.object_numbers(); + assert!(before.iter().all(|n| after.contains(n)), "{what}: every object the document had is still there"); + assert_eq!(labels(&full_roundtrip(&doc)), ["Page 3", "Page 1", "Page 1", "Page 2", "Page 3"], "{what}"); + } +} + +/// A refused import takes back what it added, which in these documents is numbered below their +/// highest object, and leaves the objects they had. +#[test] +fn a_refused_import_takes_back_what_it_added_wherever_it_is_numbered() { + let run = std::thread::Builder::new() + .stack_size(512 << 10) + .spawn(|| { + for (what, mut dst) in documents_numbering_new_objects_below_their_highest() { + let before = (labels(&dst), dst.object_numbers()); + let err = import_pages(&mut dst, &chain(20_000), &[0], 0).unwrap_err(); + assert!(matches!(&err, OrganizeError::Invalid(m) if m.contains("levels deep")), "{what}: {err:?}"); + assert_eq!((labels(&dst), dst.object_numbers()), before, "{what}: the destination is as it was"); + } + }) + .unwrap() + .join(); + assert!(run.is_ok(), "a refused import must leave the document as it was, without panicking"); +} + #[test] fn rotate_repeated_indices_accumulate_from_the_current_page_value() { for level in 0..3 {