From 008b035326488d20b9f45e98f6432cfdd14a7167 Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Mon, 9 Nov 2026 18:30:27 +0800 Subject: [PATCH] fix(optimize): validate named destination targets and ignore orphaned refs (#5362) --- crates/optimize/src/audit.rs | 2 +- crates/optimize/src/links.rs | 107 +++++++++++++++++------------- crates/optimize/tests/optimize.rs | 101 ++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 47 deletions(-) diff --git a/crates/optimize/src/audit.rs b/crates/optimize/src/audit.rs index afa95f555e..f5da17c45d 100644 --- a/crates/optimize/src/audit.rs +++ b/crates/optimize/src/audit.rs @@ -262,7 +262,7 @@ pub fn audit_space(doc: &Document, file_len: u64) -> Vec { 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") { diff --git a/crates/optimize/src/links.rs b/crates/optimize/src/links.rs index cbd64eb928..5b74ad7023 100644 --- a/crates/optimize/src/links.rs +++ b/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> { +/// Named destinations whose target page exists: the `/Names /Dests` tree and the old `/Dests` +/// dictionary. +fn named_dests(doc: &Document, pages: &HashSet, count: usize) -> HashSet> { 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> { } } -fn valid(o: &Object, pages: &HashSet, count: usize, names: &HashSet>) -> 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> { + 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, 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, count: usize, names: &HashSet>) -> 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 = 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> { 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 } diff --git a/crates/optimize/tests/optimize.rs b/crates/optimize/tests/optimize.rs index 2b8f513a24..b6fe624082 100644 --- a/crates/optimize/tests/optimize.rs +++ b/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);