From ecdd1e658cbb9ab5ceaa924a5598f091a6135bec Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 10:30:12 -0500 Subject: [PATCH] fix(audit): read content one operator at a time and charge hidden text and findings (#5875) The faux-redaction audit held every operator of a page's content and of each form XObject in a vector before walking it, so a stream of millions of short operators (compressed to a few kilobytes) was materialised whole within the page's decode budget. Each Do and gs also copied the whole XObject or ExtGState table to pick one entry, and each annotation listed in /Annots was deep-copied before its subtype was read. The walk now reads operators one at a time through pdfcraft_content::ops, borrows resource tables and annotation dictionaries through the resolved Arc, charges a hidden line's text bytes (padding included) against the comparison bound once per cover, and charges each finding's record against the output bound, so a shared annotation list on many pages or withheld text cannot grow the finding list for free. --- crates/audit/src/cover.rs | 37 +++++++++++----------- crates/audit/src/lib.rs | 57 ++++++++++++++++++++++------------ crates/audit/src/tests.rs | 65 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 117 insertions(+), 42 deletions(-) diff --git a/crates/audit/src/cover.rs b/crates/audit/src/cover.rs index 041ba9ddd8..bb5adf7ff8 100644 --- a/crates/audit/src/cover.rs +++ b/crates/audit/src/cover.rs @@ -15,7 +15,7 @@ //! the joined stream)`; rectangles inside a form XObject carry the `Do` operator's position. //! Positions compare lexicographically in paint order. -use pdfcraft_content::{Matrix, Op, Pieces, parse}; +use pdfcraft_content::{Matrix, Op, Pieces, ops}; use pdfcraft_cos::{DecodeBudget, Dict, Document, Object}; use crate::AuditError; @@ -123,12 +123,13 @@ impl<'a> Walker<'a> { Ok(()) } - /// Walk one operator slice with the given graphics state. `at_do` is the `Do` position to + /// Walk one stream's operators, read one at a time (a stream of millions of short operators + /// is never held whole), with the given graphics state. `at_do` is the `Do` position to /// stamp on rectangles found inside a form XObject, or `None` at page level (where each /// fill operator's own position is used). fn walk_ops( &mut self, - ops: &[Op], + ops: impl Iterator, gs: &mut Gs, stack: &mut Vec, resources: &Dict, @@ -136,8 +137,9 @@ impl<'a> Walker<'a> { at_do: Option<(usize, usize)>, ) -> Result<(), AuditError> { let mut path = Path::Empty; - for (i, op) in ops.iter().enumerate() { + for (i, op) in ops.enumerate() { self.bump()?; + let op = &op; // Paint position of this operator: its own `(piece, joined index)` at page level, // the enclosing `Do`'s position inside a form XObject. let pos = match at_do { @@ -257,8 +259,9 @@ impl<'a> Walker<'a> { /// changes inside never leak out; every rectangle found is stamped with the `Do` /// position `pos`. fn walk_xobject(&mut self, name: &[u8], gs: &Gs, resources: &Dict, depth: usize, pos: (usize, usize)) -> Result<(), AuditError> { - let xobjects = resources.get(b"XObject").map(|o| self.doc.resolve(o)).and_then(|o| o.as_dict().cloned()).unwrap_or_default(); - let Some(entry) = xobjects.get(name) else { return Ok(()) }; + // Borrowed, not copied: a `Do` must not cost the size of the whole XObject table. + let xobjects = resources.get(b"XObject").map(|o| self.doc.resolve(o)); + let Some(entry) = xobjects.as_deref().and_then(Object::as_dict).and_then(|d| d.get(name)) else { return Ok(()) }; let resolved = self.doc.resolve(entry); let Object::Stream(stream) = &*resolved else { return Ok(()) }; let dict = &stream.dict; @@ -274,18 +277,18 @@ impl<'a> Walker<'a> { (v.len() == 6 && v.iter().all(|x| x.is_finite())).then(|| Matrix([v[0], v[1], v[2], v[3], v[4], v[5]])) }) .unwrap_or(Matrix::IDENTITY); - let sub_resources = dict.get(b"Resources").map(|o| self.doc.resolve(o)).and_then(|o| o.as_dict().cloned()).unwrap_or_default(); + let (sub_resources, empty) = (dict.get(b"Resources").map(|o| self.doc.resolve(o)), Dict::new()); + let sub_resources = sub_resources.as_deref().and_then(Object::as_dict).unwrap_or(&empty); if !self.budget.enter_form() { return Err(AuditError::TooManyForms); } // A form stream that cannot be decoded fails the audit: it might hide a cover. let data = self.budget.decode_strict(stream)?; - let ops = parse(&data).ops; // The XObject matrix maps form space into the user space active at `Do`. let mut sub_gs = gs.clone(); sub_gs.ctm = matrix.then(&gs.ctm); let mut sub_stack = Vec::new(); - self.walk_ops(&ops, &mut sub_gs, &mut sub_stack, &sub_resources, depth + 1, Some(pos)) + self.walk_ops(ops(&data), &mut sub_gs, &mut sub_stack, sub_resources, depth + 1, Some(pos)) } } @@ -298,13 +301,10 @@ fn user_point(gs: &Gs, op: &Op) -> Option<[f64; 2]> { /// The non-stroking alpha (`/ca`) of a named ExtGState, defaulting to opaque. fn ext_alpha(doc: &Document, resources: &Dict, name: &[u8]) -> f64 { - let alpha = resources - .get(b"ExtGState") - .map(|o| doc.resolve(o)) - .and_then(|o| o.as_dict().cloned()) - .and_then(|d| d.get(name).map(|o| doc.resolve(o))) - .and_then(|o| o.as_dict().cloned()) - .and_then(|d| d.get(b"ca").map(|o| doc.resolve(o)).and_then(|o| o.as_f64())); + // Borrowed, not copied: a `gs` must not cost the size of the whole ExtGState table. + let states = resources.get(b"ExtGState").map(|o| doc.resolve(o)); + let state = states.as_deref().and_then(Object::as_dict).and_then(|d| d.get(name)).map(|o| doc.resolve(o)); + let alpha = state.as_deref().and_then(Object::as_dict).and_then(|d| d.get(b"ca").map(|o| doc.resolve(o)).and_then(|o| o.as_f64())); alpha.filter(|a| a.is_finite()).map(|a| a.clamp(0.0, 1.0)).unwrap_or(1.0) } @@ -330,10 +330,11 @@ pub fn filled_rects(doc: &Document, page: &Dict) -> Result, Audi } } let pieces = Pieces::join(&streams); - let parsed = pieces.parse(); let mut w = Walker { doc, pieces: &pieces, out: Vec::new(), ops_seen: 0, budget }; let mut gs = Gs::default(); let mut stack = Vec::new(); - w.walk_ops(&parsed.ops, &mut gs, &mut stack, &resources, 0, None)?; + // Read one operator at a time: the joined stream can hold millions of them within the + // decode budget, and the walk only ever needs the current one. + w.walk_ops(ops(pieces.data()), &mut gs, &mut stack, &resources, 0, None)?; Ok(w.out) } diff --git a/crates/audit/src/lib.rs b/crates/audit/src/lib.rs index f2bcf63358..97429d4c40 100644 --- a/crates/audit/src/lib.rs +++ b/crates/audit/src/lib.rs @@ -99,6 +99,9 @@ const COVER_FRACTION: f64 = 0.4; const WORD_FRACTION: f64 = 0.5; /// WCAG contrast ratio below which the text is unreadable against the cover. const COVER_CONTRAST: f64 = 3.0; +/// Bytes of a hidden line's text read (copied or trimmed) that cost one comparison: a byte scan +/// is far cheaper than a box comparison, but a long padded line under many covers adds up. +const TEXT_BYTES_PER_COMPARISON: usize = 16; /// Audit every page for faux redactions and unapplied redaction marks. Fails closed: any /// content that cannot be decoded, or that needs more work or report than the audit's bounds, is @@ -157,15 +160,24 @@ impl Budget { cover: &[f64; 4], hides: impl Fn(&TextLine) -> bool, ) -> Result, AuditError> { - self.comparisons = self.comparisons.saturating_add(self.page_cost); - if self.comparisons > self.limits.comparisons { - return Err(AuditError::TooManyComparisons); - } + self.spend(self.page_cost)?; if !copying { - return Ok(lines.iter().any(|line| hides(line) && !line.text.trim().is_empty()).then(String::new)); + let mut hidden = false; + for line in lines.iter().filter(|l| hides(l)) { + // Trimming walks the line's text (padding and all): it costs its bytes. + self.spend(line.text.len() / TEXT_BYTES_PER_COMPARISON)?; + if !line.text.trim().is_empty() { + hidden = true; + break; + } + } + return Ok(hidden.then(String::new)); } let mut covered = String::new(); for line in lines.iter().filter(|l| hides(l)) { + // Copying and trimming the line walks its text once per cover: a long padded + // line under many covers would otherwise be read far more than its runs suggest. + self.spend(line.text.len() / TEXT_BYTES_PER_COMPARISON)?; if !covered.is_empty() { covered.push(' '); } @@ -174,11 +186,21 @@ impl Budget { Ok((!covered.trim().is_empty()).then_some(covered)) } + /// `n` units of work done against the comparison bound. + fn spend(&mut self, n: usize) -> Result<(), AuditError> { + self.comparisons = self.comparisons.saturating_add(n); + if self.comparisons > self.limits.comparisons { + return Err(AuditError::TooManyComparisons); + } + Ok(()) + } + /// The text a finding reports for `covered`: none when copying is withheld. Charges what it - /// reports against the output bound. + /// reports, and the finding's own record, against the output bound: a finding with no text + /// (copying withheld, or a shared annotation list on many pages) still takes memory. fn report_text(&mut self, covered: String, copying: bool) -> Result { let reported = if copying { covered } else { String::new() }; - self.covered_text = self.covered_text.saturating_add(reported.len()); + self.covered_text = self.covered_text.saturating_add(reported.len()).saturating_add(std::mem::size_of::()); if self.covered_text > self.limits.covered_text { return Err(AuditError::TooMuchOutput); } @@ -321,17 +343,14 @@ enum AnnotationCover { fn annots(doc: &Document, page: &Dict) -> Result, AuditError> { let Some(a) = page.get(b"Annots") else { return Ok(Vec::new()) }; let resolved = doc.resolve(a); - let items: Vec = match &*resolved { - pdfcraft_cos::Object::Array(items) => items.clone(), - // A missing /Annots means no marks; a malformed one fails the audit. - _ => return Err(AuditError::BadAnnots), - }; + // A missing /Annots means no marks; a malformed one fails the audit. + let Some(items) = resolved.as_array() else { return Err(AuditError::BadAnnots) }; let mut out = Vec::new(); - for a in &items { - let dict = match &*doc.resolve(a) { - pdfcraft_cos::Object::Dict(d) => d.clone(), - _ => continue, - }; + for a in items { + // Borrowed from the resolved object: one large dictionary listed many times is not + // copied once per listing. + let resolved = doc.resolve(a); + let Some(dict) = resolved.as_dict() else { continue }; let v: Vec = match dict.get(b"Rect") { None => continue, Some(r) => doc.resolve(r).as_array().map(|arr| arr.iter().filter_map(|o| doc.resolve(o).as_f64()).collect()).unwrap_or_default(), @@ -340,7 +359,7 @@ fn annots(doc: &Document, page: &Dict) -> Result, AuditErro continue; } let rect = [v[0].min(v[2]), v[1].min(v[3]), v[0].max(v[2]), v[1].max(v[3])]; - match subtype(doc, &dict)?.as_deref() { + match subtype(doc, dict)?.as_deref() { Some(b"Redact") => out.push(AnnotationCover::Redact(rect)), Some(b"Square") => { let flags = dict.get(b"F").and_then(|f| doc.resolve(f).as_int()).unwrap_or(0); @@ -349,7 +368,7 @@ fn annots(doc: &Document, page: &Dict) -> Result, AuditErro } let alpha = dict.get(b"CA").map(|o| doc.resolve(o)).and_then(|o| o.as_f64()).unwrap_or(1.0); if alpha >= 1.0 - && let Some(rgb) = annotation_color(doc, &dict, b"IC") + && let Some(rgb) = annotation_color(doc, dict, b"IC") { out.push(AnnotationCover::Square { rect, rgb }); } diff --git a/crates/audit/src/tests.rs b/crates/audit/src/tests.rs index e5fb9ab683..074eb39f66 100644 --- a/crates/audit/src/tests.rs +++ b/crates/audit/src/tests.rs @@ -720,9 +720,11 @@ fn runs_without_words_count_against_the_comparison_bound() { let doc = open(one_page(&content, "")); let lines = pdfcraft_edit::glyph_text_lines(&doc, &pdfcraft_model::pages(&doc)[0]).expect("text"); assert_eq!(lines.iter().map(|l| (l.ink.len(), l.runs().count())).collect::>(), [(1, 51)]); - let limits = crate::Limits { comparisons: 50, covered_text: usize::MAX }; + // Plus the hidden line's text, read once to copy it. + let text = lines[0].text.len() / crate::TEXT_BYTES_PER_COMPARISON; + let limits = crate::Limits { comparisons: 50 + text, covered_text: usize::MAX }; assert!(matches!(crate::audit(&doc, limits), Err(crate::AuditError::TooManyComparisons))); - let limits = crate::Limits { comparisons: 51, covered_text: usize::MAX }; + let limits = crate::Limits { comparisons: 51 + text, covered_text: usize::MAX }; assert_eq!(crate::audit(&doc, limits).expect("within the budget").len(), 1); } @@ -746,14 +748,18 @@ fn copy_restricted_documents_withhold_covered_text() { let findings = audit_redactions(&user).expect("audit must run"); assert_eq!(findings.len(), 1, "the cover is still reported: {findings:?}"); assert_eq!(findings[0].covered_text, ""); - // The output bound counts the text reported, so withheld text costs nothing. - let nothing = crate::Limits { comparisons: usize::MAX, covered_text: 0 }; + // The output bound counts the text reported and the finding's record, so withheld text + // costs only the record. + let record = std::mem::size_of::(); + let nothing = crate::Limits { comparisons: usize::MAX, covered_text: record }; assert_eq!(crate::audit(&user, nothing).expect("withheld text is not output").len(), 1); + let none = crate::Limits { covered_text: record - 1, ..nothing }; + assert!(matches!(crate::audit(&user, none), Err(crate::AuditError::TooMuchOutput))); let owner = Document::open_with_password(bytes, Some("owner")).expect("open"); let findings = audit_redactions(&owner).expect("audit must run"); assert_eq!(findings.len(), 1, "{findings:?}"); assert_eq!(findings[0].covered_text, "SECRET DATA 123"); - let exact = crate::Limits { comparisons: usize::MAX, covered_text: "SECRET DATA 123".len() }; + let exact = crate::Limits { comparisons: usize::MAX, covered_text: "SECRET DATA 123".len() + record }; assert!(crate::audit(&owner, exact).is_ok()); let short = crate::Limits { covered_text: exact.covered_text - 1, ..exact }; assert!(matches!(crate::audit(&owner, short), Err(crate::AuditError::TooMuchOutput))); @@ -820,3 +826,52 @@ fn form_entries_past_the_page_allowance_fail_the_audit() { let page: Dict = [(b"Contents".to_vec(), Object::Ref(draws)), (b"Resources".to_vec(), Object::Dict(resources))].into_iter().collect(); assert!(matches!(crate::cover::filled_rects(&doc, &page), Err(crate::AuditError::TooManyForms))); } + +/// Copying a covered line's text walks all of it, padding included, so a long padded line under +/// many covers costs its bytes once per cover, not one run per cover (CodeRabbit). +#[test] +fn covered_text_bytes_count_against_the_comparison_bound() { + // One run: 20 KiB of spaces around a word, which trims to the word alone. + let mut content = b"BT /F1 24 Tf 100 700 Td (".to_vec(); + content.extend_from_slice(&[b' '; 10_000]); + content.extend_from_slice(b"SECRET"); + content.extend_from_slice(&[b' '; 10_000]); + content.extend_from_slice(b") Tj ET\n0 g\n"); + for _ in 0..10 { + content.extend_from_slice(b"-1 688 1000000 32 re f\n"); + } + let doc = open(one_page(&content, "")); + let lines = pdfcraft_edit::glyph_text_lines(&doc, &pdfcraft_model::pages(&doc)[0]).expect("text"); + assert_eq!(lines.len(), 1); + let bytes = lines[0].text.len(); + assert!(bytes > 20_000, "{bytes} bytes of text"); + // Each cover: one comparison for the line, plus its bytes when it is hidden. + let exact = crate::Limits { comparisons: 10 * (1 + bytes / crate::TEXT_BYTES_PER_COMPARISON), covered_text: usize::MAX }; + let findings = crate::audit(&doc, exact).expect("within the budget"); + assert_eq!(findings.len(), 10, "{findings:?}"); + assert!(findings.iter().all(|f| f.covered_text == "SECRET"), "{findings:?}"); + let short = crate::Limits { comparisons: exact.comparisons - 1, ..exact }; + assert!(matches!(crate::audit(&doc, short), Err(crate::AuditError::TooManyComparisons))); +} + +/// One annotation list shared by many pages makes a finding on each: every finding's record +/// counts against the output bound, text or no text (CodeRabbit). +#[test] +fn every_finding_record_counts_against_the_output_bound() { + let content = b"BT /F1 24 Tf 100 700 Td (SECRET) Tj ET\n"; + let mut b = Builder::new(); + b.add("<< /Type /Catalog /Pages 2 0 R >>"); + b.add("<< /Type /Pages /Kids [3 0 R 7 0 R 8 0 R] /Count 3 >>"); + b.add("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Annots 6 0 R >>"); + b.add_stream(content); + b.add(FONT); + b.add("[<< /Type /Annot /Subtype /Redact /Rect [95 685 200 725] >>]"); + b.add("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Annots 6 0 R >>"); + b.add("<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> >> /Annots 6 0 R >>"); + let doc = open(b.build()); + let one = "SECRET".len() + std::mem::size_of::(); + let exact = crate::Limits { comparisons: usize::MAX, covered_text: 3 * one }; + assert_eq!(crate::audit(&doc, exact).expect("within the budget").len(), 3); + let short = crate::Limits { covered_text: 3 * one - 1, ..exact }; + assert!(matches!(crate::audit(&doc, short), Err(crate::AuditError::TooMuchOutput))); +}