M8: one search budget per metadata scrub, bounded entity scan, fail closed for unread forms (#2448)

Review by Losminek on #2539:
- The work cap was checked per value, so very many small /Info or XMP
  values could add up without bound. One budget now covers the whole
  scrub; past it every further entry or packet counts as matching
  (fail-closed). Identical removed runs are searched once.
- unescape looked for ';' through the rest of the text before checking
  the 12-byte entity limit, so a packet of bare '&'s was quadratic. It
  now looks only at the next 13 characters (200,000 '&': 943 ms -> 5 ms).
- A form XObject removed whole past the depth limit, or when its stream
  doesn't decode, never had its text read; both paths now set
  removed_cut so the metadata check clears everything instead of
  searching incomplete text.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/redact/src/interp.rs

@@ -861,6 +861,8 @@ fn xobject(
                     return None;
                 }
                 scope.report.forms_removed += 1;
+                // Its text was never read, so the metadata check can't know what went with it.
+                scope.removed_cut = true;
                 out.replaced.push(name);
                 out.doomed.push(r);
                 return Some(None);
@@ -887,6 +889,7 @@ fn xobject(
                 }
                 Err(_) if scope.mode == Mode::Apply => {
                     scope.report.forms_removed += 1;
+                    scope.removed_cut = true;
                     out.replaced.push(name);
                     out.doomed.push(r);
                     return Some(None);

crates/redact/src/metadata.rs

@@ -26,8 +26,9 @@ const MAX_TERM_CHARS: usize = 4096;
 const MAX_TERMS: usize = 20_000;
 /// XMP packets larger than this, decoded, are removed without reading them.
 const MAX_XMP_BYTES: usize = 16 * 1024 * 1024;
-/// Text whose length times the number of terms exceeds this is treated as matching rather than
-/// searched (fail-closed), so a page of removed text against a large packet stays quick.
+/// The searching (text length times terms, or value length times removed text) one scrub may
+/// do in all; past it every entry and packet counts as matching (fail-closed), so a page of
+/// removed text against a large packet, or against very many small values, stays quick.
 const MAX_WORK: usize = 1 << 28;
 
 /// Lower-case `s` with every run of whitespace made one space, trimmed.
@@ -65,16 +66,41 @@ pub(crate) fn terms(removed: &[String]) -> Option<Vec<String>> {
     Some(out)
 }
 
-fn mentions(text: &str, terms: &[String]) -> bool {
+/// The searching one scrub may do, across every `/Info` entry and XMP value: past it, each
+/// further question is answered "yes" (fail-closed), so a packet of many small values can't add
+/// up to more than one large one.
+struct Work(std::cell::Cell<usize>);
+
+impl Work {
+    fn new(limit: usize) -> Self {
+        Self(std::cell::Cell::new(limit))
+    }
+
+    /// Take `cost` from what is left; `false` (and nothing left) when it doesn't fit.
+    fn spend(&self, cost: usize) -> bool {
+        match self.0.get().checked_sub(cost) {
+            Some(left) => {
+                self.0.set(left);
+                true
+            }
+            None => {
+                self.0.set(0);
+                false
+            }
+        }
+    }
+}
+
+fn mentions(text: &str, terms: &[String], work: &Work) -> bool {
     let text = normalize(text);
-    text.len().saturating_mul(terms.len()) > MAX_WORK || terms.iter().any(|t| text.contains(t.as_str()))
+    !work.spend(text.len().saturating_mul(terms.len())) || terms.iter().any(|t| text.contains(t.as_str()))
 }
 
 /// Is `value` itself part of one of the removed `runs` (normalized), as whole words? It needs
 /// three letters or digits, so an entry such as "by" doesn't go with every sentence that
 /// contains it, and word boundaries, so an XMP value "True" doesn't go with "untrue". Too much
 /// text to search counts as part (fail-closed).
-fn within(value: &str, runs: &[String]) -> bool {
+fn within(value: &str, runs: &[String], work: &Work) -> bool {
     let value = normalize(value);
     if !significant(&value, 3) {
         return false;
@@ -88,7 +114,7 @@ fn within(value: &str, runs: &[String]) -> bool {
             !(word(before) && value.starts_with(char::is_alphanumeric)) && !(word(after) && value.ends_with(char::is_alphanumeric))
         })
     };
-    value.len().saturating_mul(total) > MAX_WORK || runs.iter().any(whole)
+    !work.spend(value.len().saturating_mul(total)) || runs.iter().any(whole)
 }
 
 /// Decode the five predefined XML entities and character references; anything else stays.
@@ -98,7 +124,8 @@ fn unescape(s: &str) -> String {
     while let Some(at) = rest.find('&') {
         out.push_str(rest.get(..at).unwrap_or(""));
         let tail = rest.get(at..).unwrap_or("");
-        let end = tail.find(';').filter(|&e| e <= 12);
+        // An entity ends within 12 bytes; looking further made a text of bare `&`s quadratic.
+        let end = tail.char_indices().take(13).find(|&(_, c)| c == ';').map(|(e, _)| e);
         let entity = end.and_then(|e| tail.get(1..e));
         let decoded = entity.and_then(|e| match e {
             "amp" => Some('&'),
@@ -243,13 +270,24 @@ fn xmp_values(doc: &Document, o: &Object) -> Option<Vec<String>> {
 /// removed text was cut short; then, as when the terms themselves exceed their limits, every
 /// `/Info` string and every XMP stream goes. Returns how many entries and streams went.
 pub(crate) fn scrub(doc: &mut Document, removed: &[String], complete: bool) -> Result<usize, pdfcraft_cos::CosError> {
+    scrub_within(doc, removed, complete, MAX_WORK)
+}
+
+/// [`scrub`] with the searching it may do given, so tests can run out of it.
+pub(crate) fn scrub_within(doc: &mut Document, removed: &[String], complete: bool, limit: usize) -> Result<usize, pdfcraft_cos::CosError> {
     // `None` matches everything (fail-closed).
     let search = match terms(removed).filter(|_| complete) {
         Some(t) if t.is_empty() => return Ok(0),
-        Some(t) => Some((t, removed.iter().map(|r| normalize(r)).collect::<Vec<String>>())),
+        Some(t) => {
+            // The same run removed many times is searched for once.
+            let mut seen = HashSet::new();
+            let runs: Vec<String> = removed.iter().map(|r| normalize(r)).filter(|r| seen.insert(r.clone())).collect();
+            Some((t, runs))
+        }
         None => None,
     };
-    let hit = |text: &str| search.as_ref().is_none_or(|(terms, runs)| mentions(text, terms) || within(text, runs));
+    let work = Work::new(limit);
+    let hit = |text: &str| search.as_ref().is_none_or(|(terms, runs)| mentions(text, terms, &work) || within(text, runs, &work));
     let mut n = 0;
 
     // The information dictionary, direct in the trailer or an object of its own.
@@ -278,8 +316,8 @@ pub(crate) fn scrub(doc: &mut Document, removed: &[String], complete: bool) -> R
         let doomed = match xmp_values(doc, &xmp) {
             None => true,
             Some(values) => {
-                let words = |(terms, _): &(Vec<String>, Vec<String>)| mentions(&values.join(" "), terms);
-                let part = |(_, runs): &(Vec<String>, Vec<String>)| values.iter().any(|v| within(v, runs));
+                let words = |(terms, _): &(Vec<String>, Vec<String>)| mentions(&values.join(" "), terms, &work);
+                let part = |(_, runs): &(Vec<String>, Vec<String>)| values.iter().any(|v| within(v, runs, &work));
                 search.as_ref().is_none_or(|s| words(s) || part(s))
             }
         };

crates/redact/src/tests.rs

@@ -257,6 +257,63 @@ fn metadata_is_cleared_when_the_removed_text_was_cut_short() {
     assert!(!has_xmp(&doc));
 }
 
+/// #2539 review: the searching a scrub may do is one budget for all of it, not a check per
+/// value: very many small `/Info` values each fit, but together they don't, and past the budget
+/// the rest counts as matching (fail-closed). With room to search, nothing that doesn't mention
+/// the removed text goes.
+#[test]
+fn metadata_search_has_one_budget_for_every_value() {
+    let entries: String = (0..50).map(|i| format!("/K{i} (an unrelated value number {i:02} in the list) ")).collect();
+    let info = format!("<< {entries}/Producer (Test) >>");
+    let fresh = || with_metadata(b"BT /F1 10 Tf 10 100 Td (Bluefin) Tj ET", &info, "");
+    let mut doc = fresh();
+    assert_eq!(metadata::scrub_within(&mut doc, &["Bluefin".into()], true, 1 << 28).unwrap(), 0, "room to search: nothing goes");
+    let mut doc = fresh();
+    // Each value costs about 40; fifty of them don't fit in 1000.
+    let n = metadata::scrub_within(&mut doc, &["Bluefin".into()], true, 1000).unwrap();
+    assert!(n > 20, "past the budget the rest goes ({n} went)");
+}
+
+/// #2539 review: an entity is looked for only in the 12 bytes after its `&`, so a packet of bare
+/// `&` characters is read in linear time, and decoding is unchanged.
+#[test]
+fn xml_entities_are_found_near_their_ampersand_only() {
+    assert_eq!(metadata::xml_values("<a>R&#233;sum&eacute; &amp; &#x41;&#65; &#1234567890123;</a>"), ["Résum&eacute; & AA &#1234567890123;"]);
+    let bare = "&".repeat(200_000);
+    let started = std::time::Instant::now();
+    let values = metadata::xml_values(&format!("<a>{bare}</a>"));
+    assert_eq!(values.concat().len(), bare.len());
+    eprintln!("200000 bare ampersands: {:?}", started.elapsed());
+}
+
+/// #2539 review: Apply removes a form XObject whole when it is nested past the depth limit (or
+/// its stream can't be decoded), without reading its text, so what it held is unknown: the
+/// metadata check fails closed.
+#[test]
+fn a_form_removed_unread_clears_the_metadata() {
+    // Forms 5..=20 each draw the next; the last one shows text. The page draws form 5.
+    let mut objs: Vec<Vec<u8>> = vec![
+        b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(),
+        b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 300 300] /Contents 4 0 R /Resources << /XObject << /Fm 5 0 R >> >> >>".to_vec(),
+        stream("", b"q /Fm Do Q"),
+    ];
+    for n in 5..=20 {
+        objs.push(stream(
+            &format!("/Type /XObject /Subtype /Form /BBox [0 0 300 300] /Resources << /XObject << /Fm {} 0 R >> >>", n + 1),
+            b"q /Fm Do Q",
+        ));
+    }
+    objs.push(stream("/Type /XObject /Subtype /Form /BBox [0 0 300 300]", b"0 g 10 10 50 50 re f"));
+    objs.push(b"<< /Title (Annual figures) /Producer (Test) >>".to_vec());
+    let info = objs.len();
+    let mut doc = Document::open(Arc::new(Pdf::new().objs(objs).trailer_entry(&format!("/Info {info} 0 R")).build())).unwrap();
+    mark(&mut doc, 0, &[[0.0, 0.0, 300.0, 300.0]], "");
+    let r = apply(&mut doc, None).unwrap();
+    assert!(r.forms_removed >= 1, "{r:?}");
+    assert!(!info_keys(&doc).contains(&"Title".to_string()), "the deep form's text was never read: {:?}", info_keys(&doc));
+}
+
 /// A value shorter than the redacted sentence goes from XMP too, in a custom schema's element or
 /// attribute, as from `/Info` (the regression #2598 suggested keeping): the packets holding it are
 /// removed, one that doesn't stays.