fix(optimize): validate named destination targets and ignore orphaned refs (#5362)

crates/optimize/src/audit.rs

@@ -262,7 +262,7 @@ pub fn audit_space(doc: &Document, file_len: u64) -> Vec<SpaceUse> {
         a.claim(o, SpaceCategory::Bookmarks);
     }
     let names = a.resolve_dict(cat.get(b"Names"));
-    if let Some(d) = names.get(b"Dests").or_else(|| cat.get(b"Dests")) {
+    for d in [names.get(b"Dests"), cat.get(b"Dests")].into_iter().flatten() {
         a.claim(d, SpaceCategory::NamedDestinations);
     }
     if let Some(e) = names.get(b"EmbeddedFiles") {

crates/optimize/src/links.rs

@@ -5,16 +5,19 @@ use std::collections::{HashSet, VecDeque};
 
 use pdfcraft_cos::{Dict, Document, ObjRef, Object, PdfString};
 
-/// Named destinations: the `/Names /Dests` tree (entries) and the old `/Dests` dictionary.
-fn named_dests(doc: &Document) -> HashSet<Vec<u8>> {
+/// Named destinations whose target page exists: the `/Names /Dests` tree and the old `/Dests`
+/// dictionary.
+fn named_dests(doc: &Document, pages: &HashSet<ObjRef>, count: usize) -> HashSet<Vec<u8>> {
     let mut out = HashSet::new();
     let Some(cat) = doc.root().and_then(|r| doc.get(r).as_dict().cloned()) else { return out };
     if let Some(d) = cat.get(b"Dests").map(|d| doc.resolve(d)).and_then(|d| d.as_dict().cloned()) {
-        out.extend(d.iter().map(|(k, _)| k.clone()));
+        out.extend(d.iter().filter(|(_, v)| valid_explicit(doc, v, pages, count)).map(|(k, _)| k.clone()));
     }
     if let Some(tree) = names_tree(doc, &cat) {
-        for (k, _) in tree_entries(doc, &tree) {
-            out.insert(k);
+        for (k, v) in tree_entries(doc, &tree) {
+            if valid_explicit(doc, &v, pages, count) {
+                out.insert(k);
+            }
         }
     }
     out
@@ -74,22 +77,33 @@ fn dest_name(o: &Object) -> Option<Vec<u8>> {
     }
 }
 
-fn valid(o: &Object, pages: &HashSet<ObjRef>, count: usize, names: &HashSet<Vec<u8>>) -> bool {
-    match o {
-        Object::Array(a) => match a.first() {
-            Some(Object::Ref(r)) => pages.contains(r),
-            Some(Object::Int(i)) => *i >= 0 && (*i as usize) < count,
-            _ => false,
-        },
-        other => dest_name(other).is_some_and(|n| names.contains(&n)),
+/// The `[page /Fit …]` array of an explicit destination (an array or a `<< /D […] >>` dictionary).
+fn explicit_array(doc: &Document, o: &Object) -> Option<Vec<Object>> {
+    match doc.resolve(o).as_ref() {
+        Object::Array(a) => Some(a.clone()),
+        Object::Dict(d) => d.get(b"D").and_then(|x| doc.resolve(x).as_array().cloned()),
+        _ => None,
     }
 }
 
+/// Whether an explicit destination targets one of the document's pages.
+fn valid_explicit(doc: &Document, o: &Object, pages: &HashSet<ObjRef>, count: usize) -> bool {
+    explicit_array(doc, o).is_some_and(|a| match a.first() {
+        Some(Object::Ref(r)) => pages.contains(r),
+        Some(Object::Int(i)) => usize::try_from(*i).is_ok_and(|i| i < count),
+        _ => false,
+    })
+}
+
+fn valid(doc: &Document, o: &Object, pages: &HashSet<ObjRef>, count: usize, names: &HashSet<Vec<u8>>) -> bool {
+    valid_explicit(doc, o, pages, count) || dest_name(o).is_some_and(|n| names.contains(&n))
+}
+
 /// Remove links whose destination is missing, and clear the destination of such bookmarks
 /// (they stay, so their children do too). Returns (links, bookmarks).
 pub(crate) fn remove_invalid(doc: &mut Document, pages: &[ObjRef]) -> Result<(usize, usize), pdfcraft_cos::CosError> {
     let set: HashSet<ObjRef> = pages.iter().copied().collect();
-    let names = named_dests(doc);
+    let names = named_dests(doc, &set, pages.len());
     let mut links = 0;
     for p in pages {
         let Some(pd) = doc.get(*p).as_dict().cloned() else { continue };
@@ -101,7 +115,7 @@ pub(crate) fn remove_invalid(doc: &mut Document, pages: &[ObjRef]) -> Result<(us
                 if d.name(b"Subtype") != Some(b"Link") {
                     return true;
                 }
-                destination(doc, &d).is_none_or(|dest| valid(&dest, &set, pages.len(), &names))
+                destination(doc, &d).is_none_or(|dest| valid(doc, &dest, &set, pages.len(), &names))
             })
             .cloned()
             .collect();
@@ -130,7 +144,7 @@ pub(crate) fn remove_invalid(doc: &mut Document, pages: &[ObjRef]) -> Result<(us
             stack.push(f);
         }
         if let Some(dest) = destination(doc, &d)
-            && !valid(&dest, &set, pages.len(), &names)
+            && !valid(doc, &dest, &set, pages.len(), &names)
         {
             doc.update_dict(r, |d| {
                 d.remove(b"Dest");
@@ -142,39 +156,40 @@ pub(crate) fn remove_invalid(doc: &mut Document, pages: &[ObjRef]) -> Result<(us
     Ok((links, bookmarks))
 }
 
-/// Every named destination some object refers to (links, bookmarks, actions, open action).
+/// Every named destination some reachable object refers to (links, bookmarks, actions, open action).
 fn referenced_names(doc: &Document) -> HashSet<Vec<u8>> {
     let mut out = HashSet::new();
-    let mut visit = |o: &Object| {
-        let mut stack = vec![o.clone()];
-        while let Some(o) = stack.pop() {
-            match o {
-                Object::Dict(d) => {
-                    // Resolve first: /Dest and a GoTo's /D may be an indirect reference to the name
-                    // string, and an unresolved ref has no name, so the destination it uses would
-                    // look unreferenced and be removed. A GoToR or GoToE /D names a destination in
-                    // another document, so it does not count as a use of this one's.
-                    if let Some(n) = d.get(b"Dest").and_then(|x| dest_name(doc.resolve(x).as_ref())) {
-                        out.insert(n);
-                    }
-                    if d.name(b"S") == Some(b"GoTo")
-                        && let Some(n) = d.get(b"D").and_then(|x| dest_name(doc.resolve(x).as_ref()))
-                    {
-                        out.insert(n);
-                    }
-                    stack.extend(d.iter().map(|(_, v)| v.clone()));
-                }
-                Object::Array(a) => stack.extend(a),
-                Object::Stream(s) => stack.push(Object::Dict(s.dict.clone())),
-                _ => {}
-            }
-        }
-    };
-    for num in doc.object_numbers() {
-        visit(&doc.get(ObjRef::new(num, doc.generation(num))));
-    }
+    let mut seen = HashSet::new();
+    let mut stack = vec![Object::Dict(doc.trailer().clone())];
     if let Some(r) = doc.root() {
-        visit(&doc.get(r));
+        stack.push(Object::Ref(r));
+    }
+    while let Some(o) = stack.pop() {
+        match o {
+            Object::Ref(r) => {
+                if seen.insert(r) {
+                    stack.push(doc.get(r).as_ref().clone());
+                }
+            }
+            Object::Dict(d) => {
+                // Resolve first: /Dest and a GoTo's /D may be an indirect reference to the name
+                // string, and an unresolved ref has no name, so the destination it uses would
+                // look unreferenced and be removed. A GoToR or GoToE /D names a destination in
+                // another document, so it does not count as a use of this one's.
+                if let Some(n) = d.get(b"Dest").and_then(|x| dest_name(doc.resolve(x).as_ref())) {
+                    out.insert(n);
+                }
+                if d.name(b"S") == Some(b"GoTo")
+                    && let Some(n) = d.get(b"D").and_then(|x| dest_name(doc.resolve(x).as_ref()))
+                {
+                    out.insert(n);
+                }
+                stack.extend(d.iter().map(|(_, v)| v.clone()));
+            }
+            Object::Array(a) => stack.extend(a),
+            Object::Stream(s) => stack.push(Object::Dict(s.dict)),
+            _ => {}
+        }
     }
     out
 }

crates/optimize/tests/optimize.rs

@@ -4242,6 +4242,107 @@ fn mixed_text_and_small_halftone_regions_keep_their_tones_with_wide_white_margin
     }
 }
 
+#[test]
+fn dictionary_destinations_broken_named_destinations_and_both_dest_tables() {
+    let mut doc = Document::new_empty();
+    let p = page(&mut doc, &[], "BT ET");
+    let s = |t: &str| Object::String(pdfcraft_cos::PdfString::literal(t.as_bytes().to_vec()));
+    let dict_dest = |target: ObjRef| {
+        let mut d = Dict::new();
+        d.set(b"D".to_vec(), Object::Array(vec![Object::Ref(target), Object::name("Fit")]));
+        Object::Dict(d)
+    };
+    let link = |doc: &mut Document, dest: Object| {
+        let mut d = Dict::new();
+        d.set(b"Type".to_vec(), Object::name("Annot"));
+        d.set(b"Subtype".to_vec(), Object::name("Link"));
+        d.set(b"Rect".to_vec(), Object::Array(vec![0.into(), 0.into(), 10.into(), 10.into()]));
+        d.set(b"Dest".to_vec(), dest);
+        Object::Ref(doc.add(Object::Dict(d)))
+    };
+    let missing = ObjRef::new(999, 0);
+    // Links: (1) explicit dictionary destination to `p` (valid), (2) explicit dictionary
+    // destination to `missing` (invalid), (3) named destination "dict_ok" whose value is
+    // `<< /D [p /Fit] >>` (valid), (4) named destination "broken_tree" whose target page is
+    // `missing` (invalid).
+    let annots = vec![
+        link(&mut doc, dict_dest(p)),
+        link(&mut doc, dict_dest(missing)),
+        link(&mut doc, Object::Array(vec![Object::Int(i64::from(u32::MAX) + 1), Object::name("Fit")])),
+        link(&mut doc, s("dict_ok")),
+        link(&mut doc, s("broken_tree")),
+    ];
+    doc.update_dict(p, |d| d.set(b"Annots".to_vec(), Object::Array(annots))).unwrap();
+
+    // Bookmark with an indirect GoTo action pointing to "broken_old" in catalog /Dests (whose
+    // page is `missing`), and a second bookmark pointing to "old_ok" in catalog /Dests.
+    let outlines = doc.add(Object::Null);
+    let mut goto_broken = Dict::new();
+    goto_broken.set(b"S".to_vec(), Object::name("GoTo"));
+    goto_broken.set(b"D".to_vec(), Object::name("broken_old"));
+    let goto_broken = doc.add(Object::Dict(goto_broken));
+    let mut bm_broken = Dict::new();
+    bm_broken.set(b"Title".to_vec(), s("Broken"));
+    bm_broken.set(b"Parent".to_vec(), Object::Ref(outlines));
+    bm_broken.set(b"A".to_vec(), Object::Ref(goto_broken));
+    let bm_broken = doc.add(Object::Dict(bm_broken));
+    let mut bm_ok = Dict::new();
+    bm_ok.set(b"Title".to_vec(), s("Ok"));
+    bm_ok.set(b"Parent".to_vec(), Object::Ref(outlines));
+    bm_ok.set(b"Dest".to_vec(), Object::name("old_ok"));
+    let bm_ok = doc.add(Object::Dict(bm_ok));
+    doc.update_dict(bm_broken, |d| d.set(b"Next".to_vec(), Object::Ref(bm_ok))).unwrap();
+    let mut o = Dict::new();
+    o.set(b"First".to_vec(), Object::Ref(bm_broken));
+    o.set(b"Last".to_vec(), Object::Ref(bm_ok));
+    doc.set(outlines, Object::Dict(o));
+
+    // /Names /Dests has "broken_tree" (pointing to `missing`) and "dict_ok" (`<< /D [p /Fit] >>`).
+    let broken_dest = doc.add(dict_dest(missing));
+    let ok_dest = doc.add(dict_dest(p));
+    let mut tree = Dict::new();
+    tree.set(b"Names".to_vec(), Object::Array(vec![s("broken_tree"), Object::Ref(broken_dest), s("dict_ok"), Object::Ref(ok_dest)]));
+    let tree = doc.add(Object::Dict(tree));
+    let mut names = Dict::new();
+    names.set(b"Dests".to_vec(), Object::Ref(tree));
+
+    // Catalog /Dests has "broken_old" (pointing to `missing`) and "old_ok" (`<< /D [p /Fit] >>`).
+    let mut old_dests = Dict::new();
+    old_dests.set(b"broken_old".to_vec(), Object::Array(vec![Object::Ref(missing), Object::name("Fit")]));
+    old_dests.set(b"old_ok".to_vec(), dict_dest(p));
+    let old_dests = doc.add(Object::Dict(old_dests));
+
+    let root = doc.root().unwrap();
+    doc.update_dict(root, |c| {
+        c.set(b"Names".to_vec(), Object::Dict(names));
+        c.set(b"Dests".to_vec(), Object::Ref(old_dests));
+        c.set(b"Outlines".to_vec(), Object::Ref(outlines));
+    })
+    .unwrap();
+
+    // Space audit claims both /Names /Dests and catalog /Dests under NamedDestinations.
+    let only_tree = {
+        let mut d = doc.clone();
+        d.update_dict(root, |c| {
+            c.remove(b"Dests");
+        })
+        .unwrap();
+        audit_space(&d, 0).into_iter().find(|u| u.category == SpaceCategory::NamedDestinations).unwrap().bytes
+    };
+    let both_tables = audit_space(&doc, 0).into_iter().find(|u| u.category == SpaceCategory::NamedDestinations).unwrap().bytes;
+    assert!(both_tables > only_tree, "catalog /Dests is counted alongside /Names /Dests: {both_tables} vs {only_tree}");
+
+    let report = optimize(&mut doc, &Settings::default()).unwrap();
+    assert_eq!((report.invalid_links, report.invalid_bookmarks, report.unreferenced_dests), (3, 1, 2), "{report:?}");
+    let left_links = doc.get(p).as_dict().unwrap().get(b"Annots").unwrap().as_array().unwrap().len();
+    assert_eq!(left_links, 2, "the two valid dictionary/named links remain");
+    assert!(doc.get(bm_broken).as_dict().unwrap().get(b"A").is_none(), "broken bookmark action is cleared");
+    assert!(doc.get(bm_ok).as_dict().unwrap().get(b"Dest").is_some(), "valid bookmark destination stays");
+    assert_eq!(named_dest_keys(&doc), vec![b"dict_ok".to_vec()]);
+    let left_old = doc.get(old_dests).as_dict().cloned().unwrap();
+    assert!(left_old.contains(b"old_ok") && !left_old.contains(b"broken_old"), "{left_old:?}");
+}
+
 /// A one-bit stencil of `w` × `h`, solid but for a hole in its middle.
 fn stencil(doc: &mut Document, w: u32, h: u32) -> ObjRef {
     let row = (w as usize).div_ceil(8);