fix(organize): imports find the objects they added when new numbers fill gaps

import_pages_with and insert_file_pages took the objects numbered above the
destination's highest number for the ones the import added. When no number
is left above the highest (an exhausted /Size, or object 4294967295),
Document::add fills free numbers below it: the copies skipped resource
sharing, and a refused import left its objects in the destination. With
object 4294967295 the `n + 1` overflowed, a panic in debug builds and in
release a wrap to 0 that counted every object as added.

Record the object numbers before the import and take the ones present
afterwards but not before.

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<ImportOutcome, OrganizeError> {
-    let first_new = dst.object_numbers().last().map_or(1, |n| n + 1);
+    let before: HashSet<u32> = 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<ObjRef> = 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<u32>) -> Vec<ObjRef> {
+    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<ImportOutcome, OrganizeError> {
-    let first_new = dst.object_numbers().last().map_or(1, |n| n + 1);
+    let before: HashSet<u32> = dst.object_numbers().into_iter().collect();
     let imported = import_pages_mapped(dst, src, src_pages, at, conflicts)?;
-    let created: Vec<ObjRef> = 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()

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<String> = 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<String> = (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<String> = 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<String> = (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 {