From 93436d41b050f7a199efdea1f5d2b84f0348a7de Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 15:43:04 +0000 Subject: [PATCH 01/18] fix(xfdf): escape paired line-ending names in exported XFDF The head and tail attributes were written from the annotation's /LE names without XML escaping, so a name holding a quote or angle bracket could add attributes or elements, including field values, to the exported file. --- crates/xfdf/src/lib.rs | 2 +- crates/xfdf/src/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/xfdf/src/lib.rs b/crates/xfdf/src/lib.rs index 219b0de81..bc3ea01f4 100644 --- a/crates/xfdf/src/lib.rs +++ b/crates/xfdf/src/lib.rs @@ -354,7 +354,7 @@ fn xfdf_annots(doc: &Document, out: &mut String) { && le.len() == 2 { let name = |o: &Object| o.as_name().map(|x| String::from_utf8_lossy(x).into_owned()).unwrap_or_else(|| "None".into()); - let _ = write!(attrs, " head=\"{}\" tail=\"{}\"", name(&le[0]), name(&le[1])); + let _ = write!(attrs, " head=\"{}\" tail=\"{}\"", esc(&name(&le[0])), esc(&name(&le[1]))); } // A callout: its leader path, text-box inset, intent and (single-name) line ending. let cl = nums_of(doc, d.get(b"CL")); diff --git a/crates/xfdf/src/tests.rs b/crates/xfdf/src/tests.rs index c98fab06a..a93f70c70 100644 --- a/crates/xfdf/src/tests.rs +++ b/crates/xfdf/src/tests.rs @@ -1758,3 +1758,26 @@ fn an_imported_redaction_rect_with_a_non_number_is_not_used() { let mark = imported_redact_mark(fdf); assert!(mark.get(b"QuadPoints").is_none(), "quads were drawn from a malformed /Rect: {mark:?}"); } + +/// Line-ending names are attribute values: a name holding a quote or angle bracket cannot add +/// attributes or elements to the exported XFDF. +#[test] +fn xfdf_line_ending_names_are_escaped() { + let mut doc = blank(); + let page = pdfcraft_model::pages(&doc)[0].obj; + let hostile = b"a\" tail=\"x\">pwned Date: Thu, 22 Oct 2026 15:43:57 +0000 Subject: [PATCH 02/18] fix(xfdf): bound what an FDF import copies out of repeated references The FDF field walker and the comment copier followed every reference again each time it appeared, so a field tree or comment whose kids or properties repeat one reference level after level expanded a small file into millions of rows or values before the depth limit stopped it. Both now skip a reference back to an object they are inside of, and share one budget per import of values and text; past it the import is refused. --- crates/xfdf/src/lib.rs | 117 +++++++++++++++++++++++++++++++++------ crates/xfdf/src/tests.rs | 61 ++++++++++++++++++++ 2 files changed, 161 insertions(+), 17 deletions(-) diff --git a/crates/xfdf/src/lib.rs b/crates/xfdf/src/lib.rs index bc3ea01f4..99470b58a 100644 --- a/crates/xfdf/src/lib.rs +++ b/crates/xfdf/src/lib.rs @@ -1198,18 +1198,66 @@ fn find_by_name(doc: &Document, nm: &str) -> Option { None } -/// Copy an object out of another document, resolving references (FDF objects are small). -fn deep(src: &Document, o: &Object, depth: usize) -> Object { +/// What one FDF import may copy out of the file. A field tree or comment that repeats a reference +/// level after level would otherwise expand a small file into millions of values. +const MAX_FDF_IMPORT_VALUES: usize = 1_000_000; + +struct ImportBudget { + values: usize, + bytes: usize, +} + +impl ImportBudget { + fn new() -> Self { + Self { values: MAX_FDF_IMPORT_VALUES, bytes: MAX_FDF_BYTES } + } + + fn spend(&mut self, values: usize, bytes: usize) -> Result<(), DataError> { + match (self.values.checked_sub(values), self.bytes.checked_sub(bytes)) { + (Some(v), Some(b)) => { + (self.values, self.bytes) = (v, b); + Ok(()) + } + _ => Err(DataError::Malformed(format!( + "the file expands to more than {MAX_FDF_IMPORT_VALUES} values or {} MiB of text", + MAX_FDF_BYTES >> 20 + ))), + } + } +} + +/// Copy an object out of another document, resolving references. A reference back to an object +/// being copied becomes null. +fn deep(src: &Document, o: &Object, depth: usize, path: &mut Vec, budget: &mut ImportBudget) -> Result { + budget.spend(1, 0)?; if depth > 16 { - return Object::Null; + return Ok(Object::Null); } - match o { - Object::Ref(r) => deep(src, &src.get(*r), depth + 1), - Object::Array(a) => Object::Array(a.iter().map(|x| deep(src, x, depth + 1)).collect()), - Object::Dict(d) => Object::Dict(d.iter().map(|(k, v)| (k.clone(), deep(src, v, depth + 1))).collect()), + Ok(match o { + Object::Ref(r) => { + if path.contains(r) { + return Ok(Object::Null); + } + path.push(*r); + let copy = deep(src, &src.get(*r), depth + 1, path, budget); + path.pop(); + copy? + } + Object::Array(a) => Object::Array(a.iter().map(|x| deep(src, x, depth + 1, path, budget)).collect::>()?), + Object::Dict(d) => { + Object::Dict(d.iter().map(|(k, v)| Ok((k.clone(), deep(src, v, depth + 1, path, budget)?))).collect::>()?) + } Object::Stream(_) => Object::Null, + Object::String(s) => { + budget.spend(0, s.bytes.len())?; + o.clone() + } + Object::Name(n) => { + budget.spend(0, n.len())?; + o.clone() + } other => other.clone(), - } + }) } fn walk_xfdf(node: roxmltree::Node, prefix: &str, out: &mut Vec<(String, Vec)>, depth: usize) -> Result<(), DataError> { @@ -1233,27 +1281,59 @@ fn walk_xfdf(node: roxmltree::Node, prefix: &str, out: &mut Vec<(String, Vec)>, depth: usize) { +/// Read an FDF field tree. A field listed again beneath itself is skipped. +fn walk_fdf( + fdf: &Document, + list: &[Object], + prefix: &str, + out: &mut Vec<(String, Vec)>, + depth: usize, + path: &mut Vec, + budget: &mut ImportBudget, +) -> Result<(), DataError> { if depth > 32 { - return; + return Ok(()); } for f in list { - let Some(d) = fdf.resolve(f).as_dict().cloned() else { continue }; + budget.spend(1, 0)?; + let held; + let field = match f { + Object::Ref(r) if path.contains(r) => continue, + Object::Ref(r) => { + held = fdf.get(*r); + &*held + } + direct => direct, + }; + let Some(d) = field.as_dict() else { continue }; let Some(t) = d.get(b"T").and_then(|t| fdf.resolve(t).as_string().map(|s| s.to_text())) else { continue }; let full = if prefix.is_empty() { t } else { format!("{prefix}.{t}") }; + budget.spend(0, full.len())?; if let Some(v) = d.get(b"V").map(|v| fdf.resolve(v)) { let vals: Vec = match &*v { Object::String(s) => vec![s.to_text()], Object::Name(n) => vec![pdfcraft_forms::name_text(n)], - Object::Array(a) => a.iter().filter_map(|x| fdf.resolve(x).as_string().map(|s| s.to_text())).collect(), + Object::Array(a) => { + budget.spend(a.len(), 0)?; + a.iter().filter_map(|x| fdf.resolve(x).as_string().map(|s| s.to_text())).collect() + } _ => Vec::new(), }; + budget.spend(vals.len(), vals.iter().map(String::len).fold(0, usize::saturating_add))?; out.push((full.clone(), vals)); } - if let Some(kids) = d.get(b"Kids").map(|k| fdf.resolve(k)).and_then(|k| k.as_array().cloned()) { - walk_fdf(fdf, &kids, &full, out, depth + 1); + if let Some(kids) = d.get(b"Kids").map(|k| fdf.resolve(k)) + && let Some(kids) = kids.as_array() + { + let entered = f.as_ref().map(|r| path.push(r)).is_some(); + let walked = walk_fdf(fdf, kids, &full, out, depth + 1, path, budget); + if entered { + path.pop(); + } + walked?; } } + Ok(()) } fn fdf_body(fdf: &Document) -> Result { @@ -1272,8 +1352,9 @@ pub fn data_values(bytes: &[u8]) -> Result)>, DataError let fdf = Document::open(std::sync::Arc::new(bytes.to_vec())).map_err(|e| DataError::Malformed(e.to_string()))?; let body = fdf_body(&fdf)?; let mut values = Vec::new(); + let mut budget = ImportBudget::new(); if let Some(list) = body.get(b"Fields").map(|f| fdf.resolve(f)).and_then(|f| f.as_array().cloned()) { - walk_fdf(&fdf, &list, "", &mut values, 0); + walk_fdf(&fdf, &list, "", &mut values, 0, &mut Vec::new(), &mut budget)?; } return Ok(values); } @@ -1346,10 +1427,11 @@ fn import_fdf(doc: &mut Document, bytes: &[u8]) -> Result { let fdf = Document::open(std::sync::Arc::new(bytes.to_vec())).map_err(|e| DataError::Malformed(e.to_string()))?; let body = fdf_body(&fdf)?; let mut report = Report::default(); + let mut budget = ImportBudget::new(); // Fields (with /Kids for hierarchical names). let mut values = Vec::new(); if let Some(list) = body.get(b"Fields").map(|f| fdf.resolve(f)).and_then(|f| f.as_array().cloned()) { - walk_fdf(&fdf, &list, "", &mut values, 0); + walk_fdf(&fdf, &list, "", &mut values, 0, &mut Vec::new(), &mut budget)?; } // Comments: parents first, then replies (whose /IRT needs the parent's new reference). let pages: Vec = pdfcraft_model::pages(doc).iter().map(|p| p.obj).collect(); @@ -1360,7 +1442,8 @@ fn import_fdf(doc: &mut Document, bytes: &[u8]) -> Result { // The parent is another comment: link it below rather than copying it in. let irt = src.get(b"IRT").cloned(); src.remove(b"IRT"); - let Object::Dict(mut d) = deep(&fdf, &Object::Dict(src), 0) else { continue }; + let mut path: Vec = a.as_ref().into_iter().collect(); + let Object::Dict(mut d) = deep(&fdf, &Object::Dict(src), 0, &mut path, &mut budget)? else { continue }; let Some(page) = d.get(b"Page").and_then(Object::as_int).and_then(|p| usize::try_from(p).ok()).and_then(|p| pages.get(p).copied()) else { continue; }; diff --git a/crates/xfdf/src/tests.rs b/crates/xfdf/src/tests.rs index a93f70c70..cf9095bf3 100644 --- a/crates/xfdf/src/tests.rs +++ b/crates/xfdf/src/tests.rs @@ -1759,6 +1759,67 @@ fn an_imported_redaction_rect_with_a_non_number_is_not_used() { assert!(mark.get(b"QuadPoints").is_none(), "quads were drawn from a malformed /Rect: {mark:?}"); } +/// An FDF body with `body` as its `/FDF` dictionary contents and `objects` (numbered from 2). +fn fdf_with(body: &str, objects: &[String]) -> Vec { + let mut out = format!("%FDF-1.2\n1 0 obj\n<< /FDF << {body} >> >>\nendobj\n"); + for (i, o) in objects.iter().enumerate() { + out.push_str(&format!("{} 0 obj\n{o}\nendobj\n", i + 2)); + } + out.push_str("trailer\n<< /Root 1 0 R >>\n%%EOF\n"); + out.into_bytes() +} + +/// A field that lists itself as a kid is read once. +#[test] +fn an_fdf_field_listing_itself_is_read_once() { + let cyclic = fdf_with("/Fields [2 0 R]", &["<< /T (a) /V (x) /Kids [2 0 R] >>".into()]); + assert_eq!(data_values(&cyclic).unwrap(), vec![("a".to_string(), vec!["x".to_string()])]); +} + +/// A field tree whose `/Kids` repeat one subtree level after level is refused rather than +/// expanded into millions of rows. +#[test] +fn fdf_field_trees_that_repeat_references_are_bounded() { + let levels = 19; + let mut objects: Vec = (0..levels).map(|i| format!("<< /T (f{i}) /V (x) /Kids [{n} 0 R {n} 0 R] >>", n = i + 3)).collect(); + objects.push("<< /T (leaf) /V (x) >>".into()); + let shared = fdf_with("/Fields [2 0 R]", &objects); + let read = data_values(&shared); + assert!(read.is_err(), "{} rows", read.map_or(0, |v| v.len())); + assert!(matches!(import(&mut blank(), &shared), Err(DataError::Malformed(_)))); +} + +fn fdf_note(data: &str) -> String { + format!("<< /Subtype /Text /Rect [0 0 9 9] /NM (n) /Page 0 /PCData {data} >>") +} + +/// A comment property that refers back to the comment is copied once, not nested level after +/// level. +#[test] +fn an_fdf_comment_referring_to_itself_is_copied_once() { + let cyclic = fdf_with("/Annots [2 0 R]", &[fdf_note("2 0 R")]); + let mut dst = blank(); + assert_eq!(import(&mut dst, &cyclic).unwrap().comments, 1); + let placed = find_by_name(&dst, "n").unwrap(); + let data = dst.get(placed).as_dict().unwrap().get(b"PCData").cloned(); + assert!(!matches!(data, Some(Object::Dict(_))), "{data:?}"); +} + +/// A comment whose properties repeat one reference level after level is refused rather than +/// copied into millions of values. +#[test] +fn fdf_comment_graphs_that_repeat_references_are_bounded() { + let note = fdf_note; + let width = 6; + let mut objects = vec![note("3 0 R")]; + for i in 0..8 { + objects.push(format!("[{}]", vec![format!("{} 0 R", i + 4); width].join(" "))); + } + objects.push("(leaf)".into()); + let shared = fdf_with("/Annots [2 0 R]", &objects); + assert!(matches!(import(&mut blank(), &shared), Err(DataError::Malformed(_)))); +} + /// Line-ending names are attribute values: a name holding a quote or angle bracket cannot add /// attributes or elements to the exported XFDF. #[test] From ae791d667bfaf2785979eaac34b45502dc66c320 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 15:47:11 +0000 Subject: [PATCH 03/18] fix(sign): a time-stamp response must carry the requested nonce and policy parse_response compared only the digest algorithm and imprint with the query, and the token parser dropped the TSTInfo nonce. A genuine token for the same imprint from another request or under another policy was accepted. The token keeps its nonce, and a requested nonce or policy must match. --- crates/sign/src/timestamp.rs | 13 ++++++++++++- crates/sign/tests/it/pdf.rs | 22 ++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/crates/sign/src/timestamp.rs b/crates/sign/src/timestamp.rs index 58eda4f04..0eba6bb72 100644 --- a/crates/sign/src/timestamp.rs +++ b/crates/sign/src/timestamp.rs @@ -62,6 +62,8 @@ pub struct TimestampToken { pub policy: String, pub serial: Vec, pub gen_time: Time, + /// The TSTInfo nonce, when it has one that fits a `u64` (the size requests use). + pub nonce: Option, } impl TimestampToken { @@ -140,6 +142,9 @@ pub fn parse_token(raw: &[u8]) -> Result { .to_vec(); let serial = fields.get(3).ok_or_else(|| bad("TSTInfo has no serial number"))?.uint_bytes().to_vec(); let gen_time = fields.get(4).ok_or_else(|| bad("TSTInfo has no generation time"))?.time()?; + // The nonce is the only INTEGER after genTime (RFC 3161 §2.4.2): accuracy is a SEQUENCE and + // ordering a BOOLEAN. + let nonce = fields.iter().skip(5).find(|f| f.tag == tag::INTEGER).and_then(|f| f.u64().ok()); // The signed messageDigest uses the SignerInfo's digest algorithm, which need not be the // imprint's (RFC 5652 §5.4). let content_digest = token.signer.digest.digest(&[info]); @@ -151,7 +156,7 @@ pub fn parse_token(raw: &[u8]) -> Result { if !token.verify_signature(cert, &content_digest)? { return Err(bad("TSA signature is invalid")); } - Ok(TimestampToken { raw: raw.to_vec(), digest, imprint, policy, serial, gen_time }) + Ok(TimestampToken { raw: raw.to_vec(), digest, imprint, policy, serial, gen_time, nonce }) } /// Parse and validate a `TimeStampResp`, including the CMS signature and message imprint. @@ -170,6 +175,12 @@ pub fn parse_response(bytes: &[u8], query: &TimestampQuery) -> Result, policy: Option<&str>| { + let mut q = TimestampQuery::new(alg, alg.digest(&[b"document"])).unwrap(); + (q.nonce, q.policy) = (nonce, policy.map(str::to_string)); + q + }; + let answer = |q: &TimestampQuery| respond(&tsa.key, &tsa.certificate, &tsa.chain, alg, q, "1.2.3.4", time, 9).unwrap(); + let with_nonce = answer(&query(Some(42), None)); + assert!(parse_response(&with_nonce, &query(Some(42), Some("1.2.3.4"))).is_ok()); + assert!(parse_response(&with_nonce, &query(None, None)).is_ok()); + assert!(parse_response(&with_nonce, &query(Some(43), None)).is_err(), "another request's nonce"); + assert!(parse_response(&with_nonce, &query(Some(42), Some("1.2.3.5"))).is_err(), "another policy"); + let without_nonce = answer(&query(None, None)); + assert!(parse_response(&without_nonce, &query(Some(42), None)).is_err(), "no nonce in the token"); +} + #[cfg(windows)] #[test] fn windows_store_enumeration_and_missing_identity() { From 0a30b4e08107ad1b5809afc83c1f9790a6ef26f8 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:01:27 +0000 Subject: [PATCH 04/18] fix(preflight): read content colour one operator at a time content_colour parsed each content stream into a full list of operators before looking at any of them, so a small compressed stream of millions of tiny operators became gigabytes of parsed operators in memory. The content parser's loop is now an iterator (pdfcraft_content::ops), which parse collects; preflight reads it one operator at a time and stops once gray, RGB and CMYK are all found. --- crates/content/src/lib.rs | 197 +++++++++++++++++++++------------- crates/preflight/src/lib.rs | 16 ++- crates/preflight/src/tests.rs | 13 +++ 3 files changed, 151 insertions(+), 75 deletions(-) diff --git a/crates/content/src/lib.rs b/crates/content/src/lib.rs index eb126ffa2..3432f7ff3 100644 --- a/crates/content/src/lib.rs +++ b/crates/content/src/lib.rs @@ -79,90 +79,143 @@ pub fn parse(data: &[u8]) -> Parsed { parse_pieces(data, &[]) } +/// The operators of a (decoded) content stream, read one at a time, as [`parse`] reads them. A +/// caller that stops early or keeps only a summary never holds every operator of the stream. +pub fn ops(data: &[u8]) -> Ops<'_> { + Ops::new(data, &[]) +} + /// Parse `data`, recording which of the piece boundaries `ends` (as in [`Pieces`]) a comment /// crosses. fn parse_pieces(data: &[u8], ends: &[usize]) -> Parsed { - let mut out = Parsed { comment_edges: vec![(false, false); ends.len()], ..Parsed::default() }; - let mut boundary = 0; - let mut lx = Lexer::new(data, 0); - let mut operands: Vec = Vec::new(); - let mut start: Option = None; - loop { - let trivia = lx.pos; - lx.skip_ws(); - // Only the whitespace the lexer skips here holds top-level comments: a `%` inside a - // string, a name, an array or dictionary operand or inline image data is not one. - if boundary < ends.len() { - comment_edges(data, trivia..lx.pos, ends, &mut boundary, &mut out.comment_edges); + let mut reader = Ops::new(data, ends); + let ops = reader.by_ref().collect(); + Parsed { ops, skipped: reader.skipped, comment_edges: reader.comment_edges } +} + +/// The iterator [`ops`] returns. +pub struct Ops<'a> { + data: &'a [u8], + lx: Lexer<'a>, + operands: Vec, + start: Option, + skipped: usize, + ends: &'a [usize], + boundary: usize, + comment_edges: Vec<(bool, bool)>, +} + +impl<'a> Ops<'a> { + fn new(data: &'a [u8], ends: &'a [usize]) -> Self { + Ops { + data, + lx: Lexer::new(data, 0), + operands: Vec::new(), + start: None, + skipped: 0, + ends, + boundary: 0, + comment_edges: vec![(false, false); ends.len()], } - let Some(&b) = data.get(lx.pos) else { break }; - let at = lx.pos; - let operand_start = matches!(b, b'/' | b'(' | b'<' | b'[' | b'+' | b'-' | b'.') || b.is_ascii_digit(); - if operand_start { - match lx.object() { - Ok(o) => { + } + + /// Bytes read so far that could not be read as operands or operators. + pub fn skipped(&self) -> usize { + self.skipped + } +} + +impl Iterator for Ops<'_> { + type Item = Op; + + fn next(&mut self) -> Option { + let Ops { data, lx, operands, start, skipped, ends, boundary, comment_edges: edges } = self; + let (data, ends): (&[u8], &[usize]) = (data, ends); + loop { + let trivia = lx.pos; + lx.skip_ws(); + // Only the whitespace the lexer skips here holds top-level comments: a `%` inside a + // string, a name, an array or dictionary operand or inline image data is not one. + if *boundary < ends.len() { + comment_edges(data, trivia..lx.pos, ends, boundary, edges); + } + let Some(&b) = data.get(lx.pos) else { + // Operands left without an operator. + *skipped += operands.len(); + operands.clear(); + return None; + }; + let at = lx.pos; + let operand_start = matches!(b, b'/' | b'(' | b'<' | b'[' | b'+' | b'-' | b'.') || b.is_ascii_digit(); + if operand_start { + match lx.object() { + Ok(o) => { + start.get_or_insert(at); + operands.push(o); + } + Err(_) if b == b'(' => { + // An unterminated string runs to the end of the stream. + *skipped += data.len() - at; + lx.pos = data.len(); + } + Err(_) => { + // Skip the offending byte and carry on. + lx.pos = at + 1; + *skipped += 1; + } + } + continue; + } + if is_delim(b) { + // A stray `)`, `>`, `]`, `{`, `}`. + lx.pos += 1; + *skipped += 1; + continue; + } + let kw = lx.token(); + if kw.is_empty() { + lx.pos = at + 1; + *skipped += 1; + continue; + } + match kw { + b"true" => { start.get_or_insert(at); - operands.push(o); + operands.push(Object::Bool(true)); } - Err(_) if b == b'(' => { - // An unterminated string runs to the end of the stream. - out.skipped += data.len() - at; - lx.pos = data.len(); + b"false" => { + start.get_or_insert(at); + operands.push(Object::Bool(false)); } - Err(_) => { - // Skip the offending byte and carry on. - lx.pos = at + 1; - out.skipped += 1; + b"null" => { + start.get_or_insert(at); + operands.push(Object::Null); } - } - continue; - } - if is_delim(b) { - // A stray `)`, `>`, `]`, `{`, `}`. - lx.pos += 1; - out.skipped += 1; - continue; - } - let kw = lx.token(); - if kw.is_empty() { - lx.pos = at + 1; - out.skipped += 1; - continue; - } - match kw { - b"true" => { - start.get_or_insert(at); - operands.push(Object::Bool(true)); - } - b"false" => { - start.get_or_insert(at); - operands.push(Object::Bool(false)); - } - b"null" => { - start.get_or_insert(at); - operands.push(Object::Null); - } - b"BI" => match inline_image(data, lx.pos) { - Ok((dict, img, end)) => { - lx.pos = end; - out.ops.push(Op { op: b"BI".to_vec(), operands: Vec::new(), inline: Some((dict, img)), span: start.take().unwrap_or(at)..end }); - operands.clear(); + b"BI" => match inline_image(data, lx.pos) { + Ok((dict, img, end)) => { + lx.pos = end; + operands.clear(); + return Some(Op { + op: b"BI".to_vec(), + operands: Vec::new(), + inline: Some((dict, img)), + span: start.take().unwrap_or(at)..end, + }); + } + Err(end) => { + *skipped += end.saturating_sub(at); + lx.pos = end; + *start = None; + operands.clear(); + } + }, + _ => { + let s = start.take().unwrap_or(at); + return Some(Op { op: kw.to_vec(), operands: std::mem::take(operands), inline: None, span: s..lx.pos }); } - Err(end) => { - out.skipped += end.saturating_sub(at); - lx.pos = end; - start = None; - operands.clear(); - } - }, - _ => { - let s = start.take().unwrap_or(at); - out.ops.push(Op { op: kw.to_vec(), operands: std::mem::take(&mut operands), inline: None, span: s..lx.pos }); } } } - out.skipped += operands.len(); - out } /// Record the piece boundaries crossed by the comments in `trivia` (skipped whitespace and diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs index 2cda29fea..4e5f7eb2a 100644 --- a/crates/preflight/src/lib.rs +++ b/crates/preflight/src/lib.rs @@ -339,8 +339,17 @@ fn is_content(d: &Dict) -> bool { /// The device colour a content stream sets. It is read operator by operator, so the bytes of a /// string, a comment or an inline image are never taken for an operator, and an operator right /// before a delimiter (`1 k/GS0 gs`) is still found. -fn content_colour(data: &[u8], used: &mut DeviceColour) { - for op in pdfcraft_content::parse(data).ops { +/// +/// Operators are read one at a time and dropped, so a stream of millions of tiny operators never +/// becomes millions of parsed ones in memory, and reading stops once all three spaces are set. +/// Returns how many operators it read. +fn content_colour(data: &[u8], used: &mut DeviceColour) -> usize { + let mut read = 0; + for op in pdfcraft_content::ops(data) { + if used.gray && used.rgb && used.cmyk { + break; + } + read += 1; match op.op.as_slice() { b"g" | b"G" => used.gray = true, b"rg" | b"RG" => used.rgb = true, @@ -366,6 +375,7 @@ fn content_colour(data: &[u8], used: &mut DeviceColour) { _ => {} } } + read } /// Whether any content or object uses a device colour space. @@ -400,7 +410,7 @@ fn uses_device_colour(doc: &Document, objs: &[(ObjRef, std::sync::Arc)]) && (referenced.contains(r) || is_content(&s.dict)) && let Ok(data) = s.decoded() { - content_colour(&data, &mut used); + let _ = content_colour(&data, &mut used); } } used diff --git a/crates/preflight/src/tests.rs b/crates/preflight/src/tests.rs index 80b1c63c4..290c7695a 100644 --- a/crates/preflight/src/tests.rs +++ b/crates/preflight/src/tests.rs @@ -417,3 +417,16 @@ fn oversized_xmp_is_refused_while_decoding() { assert_eq!(stream.decoded().unwrap().len(), xmp::MAX_XMP_BYTES + 1, "fixture must actually decompress"); assert!(metadata(&doc).is_none(), "compressed metadata must be limited during decoding"); } + +/// Content is read one operator at a time and no further than it needs: once gray, RGB and CMYK +/// are all set, the rest of the stream is not read. +#[test] +fn content_colour_reads_operators_one_at_a_time_and_stops_when_all_are_found() { + let mut data = b"0 g 0 0 0 rg 0 0 0 0 k\n".to_vec(); + data.extend(b"n\n".repeat(100_000)); + let mut used = DeviceColour::default(); + let read = content_colour(&data, &mut used); + assert!(used.gray && used.rgb && used.cmyk); + assert_eq!(read, 3); +} + From adab150a4558722503506617de545563308c2e2e Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:01:51 +0000 Subject: [PATCH 05/18] fix(preflight): find annotation pages from one pass over the pages verify found each annotation's page by copying and searching every page's /Annots array again for each annotation, so a page with many annotations cost quadratic time. The annotation-to-page map is built once (the first page that lists an annotation keeps it, as before). --- crates/preflight/src/lib.rs | 31 ++++++++++++++++++++----------- crates/preflight/src/tests.rs | 18 ++++++++++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs index 4e5f7eb2a..731234aa6 100644 --- a/crates/preflight/src/lib.rs +++ b/crates/preflight/src/lib.rs @@ -416,6 +416,12 @@ fn uses_device_colour(doc: &Document, objs: &[(ObjRef, std::sync::Arc)]) used } +#[cfg(test)] +thread_local! { + /// Page `/Annots` entries read while matching annotations to their pages. + static ANNOT_ENTRIES_READ: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// Check `doc` against `level`. pub fn verify(doc: &Document, level: Level) -> Vec { let mut out = Vec::new(); @@ -486,17 +492,20 @@ pub fn verify(doc: &Document, level: Level) -> Vec { } // Pages: annotations and page actions. let pages = pdfcraft_model::pages(doc); - let page_of = |r: ObjRef| { - pages.iter().position(|p| { - p.dict - .get(b"Annots") - .map(|a| doc.resolve(a)) - .and_then(|a| a.as_array().cloned()) - .unwrap_or_default() - .iter() - .any(|x| x.as_ref() == Some(r)) - }) - }; + // Each annotation's page: the first page that lists it. + let mut annot_pages: std::collections::HashMap = std::collections::HashMap::new(); + for (i, p) in pages.iter().enumerate() { + if let Some(a) = p.dict.get(b"Annots").map(|a| doc.resolve(a)) + && let Some(a) = a.as_array() + { + #[cfg(test)] + ANNOT_ENTRIES_READ.with(|n| n.set(n.get() + a.len())); + for r in a.iter().filter_map(Object::as_ref) { + annot_pages.entry(r).or_insert(i); + } + } + } + let page_of = |r: ObjRef| annot_pages.get(&r).copied(); for (i, p) in pages.iter().enumerate() { if p.dict.get(b"AA").is_some() { issue("6.5.2", "Page {} has additional actions", Some((i + 1).to_string()), Some(i), true); diff --git a/crates/preflight/src/tests.rs b/crates/preflight/src/tests.rs index 290c7695a..a44d47de1 100644 --- a/crates/preflight/src/tests.rs +++ b/crates/preflight/src/tests.rs @@ -430,3 +430,21 @@ fn content_colour_reads_operators_one_at_a_time_and_stops_when_all_are_found() { assert_eq!(read, 3); } +/// Each annotation's page is found from one pass over the pages' `/Annots`, not by searching +/// every page's list again for each annotation. +#[test] +fn annotation_pages_are_found_in_one_pass() { + let n = 2_000; + let annots: Vec = (0..n).map(|i| format!("{} 0 R", i + 4)).collect(); + let page = format!("<< /Type /Page /Parent 2 0 R /Annots [{}] >>", annots.join(" ")); + let mut objs = + vec!["<< /Type /Catalog /Pages 2 0 R >>".to_string(), "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>".into(), page]; + objs.extend((0..n).map(|_| "<< /Type /Annot /Subtype /Link /Rect [0 0 0 0] /P 3 0 R >>".to_string())); + let refs: Vec<&str> = objs.iter().map(String::as_str).collect(); + let doc = build(&refs, ""); + ANNOT_ENTRIES_READ.with(|c| c.set(0)); + let issues = verify(&doc, Level::A2b); + assert!(issues.iter().any(|i| i.clause == "6.3.2" && i.page == Some(0)), "{issues:#?}"); + let read = ANNOT_ENTRIES_READ.with(std::cell::Cell::get); + assert!(read <= 2 * n, "{read} entries read for {n} annotations"); +} From 90982ba9a86233db45e674baea493fc26196151f Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:03:50 +0000 Subject: [PATCH 06/18] fix(preflight): check the actions an allowed action chains to Conversion and verification looked only at an action's own /S. An allowed action (a GoTo, say) whose /Next held a JavaScript or other forbidden action written directly inside it was kept, and verification never looked at actions written directly in a catalog /OpenAction or an annotation /A. Both now follow /Next through the actions written inside an action (an indirect /Next is an object of its own, checked as one), within a bound, and conversion removes the whole chain. --- crates/preflight/src/lib.rs | 63 +++++++++++++++++++++++++++-------- crates/preflight/src/tests.rs | 26 +++++++++++++++ 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs index 731234aa6..905ba8cbf 100644 --- a/crates/preflight/src/lib.rs +++ b/crates/preflight/src/lib.rs @@ -515,12 +515,14 @@ pub fn verify(doc: &Document, level: Level) -> Vec { let Some(d) = dict_of(o) else { continue }; let ty = d.name(b"Type"); let sub = d.name(b"Subtype"); - // Actions anywhere (structure elements also have /S: their role). - if let Some(s) = d.name(b"S") - && FORBIDDEN_ACTIONS.contains(&s) - && is_action(d) - { - issue("6.5.1", "A {} action isn't allowed", Some(String::from_utf8_lossy(s).into_owned()), None, true); + // Actions anywhere (structure elements also have /S: their role), with the actions they + // chain to and those written directly in an /A or /OpenAction. + let own = (d.get(b"S").is_some() && is_action(d)).then_some(d); + let direct = [b"A".as_slice(), b"OpenAction"].map(|k| d.get(k).and_then(|a| a.as_dict())); + for a in [own].into_iter().chain(direct).flatten() { + if let Some(s) = chain_forbidden(a) { + issue("6.5.1", "A {} action isn't allowed", Some(s), None, true); + } } // Annotations. if ty == Some(b"Annot") || (sub.is_some() && d.get(b"Rect").is_some() && d.get(b"P").is_some()) { @@ -652,10 +654,39 @@ fn is_action(d: &Dict) -> bool { } } -/// Whether `o` is (or refers to) a forbidden action, or to nothing. +/// How many actions one action and the `/Next` actions written inside it may hold before the +/// whole chain counts as forbidden, since it can't be checked. +const MAX_ACTION_CHAIN: usize = 1024; + +/// The first forbidden action type in the action `a` or in the actions written directly in its +/// `/Next` (ISO 32000-2 12.6.2: a dictionary or an array of them). An action `/Next` refers to +/// is an object of its own, checked and removed as one. +fn chain_forbidden(a: &Dict) -> Option { + let mut stack = vec![a]; + let mut read = 0; + while let Some(d) = stack.pop() { + read += 1; + if read > MAX_ACTION_CHAIN { + return Some("too deeply chained".into()); + } + if let Some(s) = d.name(b"S") + && FORBIDDEN_ACTIONS.contains(&s) + { + return Some(String::from_utf8_lossy(s).into_owned()); + } + match d.get(b"Next") { + Some(Object::Dict(next)) => stack.push(next), + Some(Object::Array(list)) => stack.extend(list.iter().filter_map(Object::as_dict)), + _ => {} + } + } + None +} + +/// Whether `o` is (or refers to) a forbidden action or one chaining to it, or to nothing. fn forbidden(doc: &Document, o: &Object) -> bool { match &*doc.resolve(o) { - Object::Dict(a) => a.name(b"S").is_some_and(|s| FORBIDDEN_ACTIONS.contains(&s)), + Object::Dict(a) => chain_forbidden(a).is_some(), Object::Null => true, _ => false, } @@ -801,14 +832,20 @@ pub fn convert(doc: &mut Document, level: Level) -> Result>"); + let open_action = format!("<< /Type /Catalog /Pages 2 0 R /OpenAction << /S /GoTo /D [3 0 R /Fit] /Next {js} >> >>"); + let goto = format!("<< /Type /Action /S /GoTo /D [3 0 R /Fit] /Next [{js}] >>"); + let cases = [ + build(&[&open_action, pages, page, &link("<< /S /GoTo /D [3 0 R /Fit] >>")], ""), + build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("5 0 R"), &goto], ""), + build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link(&format!("<< /S /GoTo /D [3 0 R /Fit] /Next {js} >>"))], ""), + ]; + for (i, mut doc) in cases.into_iter().enumerate() { + let found = messages(&verify(&doc, Level::A2b)).contains(&"A JavaScript action isn't allowed"); + assert!(found, "case {i}: {:#?}", verify(&doc, Level::A2b)); + let r = convert(&mut doc, Level::A2b).unwrap(); + assert!(!r.remaining.iter().any(|x| x.clause == "6.5.1"), "case {i}: {:#?}", r.remaining); + let plain = pdfcraft_cos::SaveOptions { object_streams: false, ..Default::default() }; + let bytes = pdfcraft_cos::write_full(&doc, &plain).unwrap(); + assert!(!bytes.windows(11).any(|w| w == b"/JavaScript"), "case {i}: {}", String::from_utf8_lossy(&bytes)); + } +} + /// Each annotation's page is found from one pass over the pages' `/Annots`, not by searching /// every page's list again for each annotation. #[test] From 467c8556ec655be08e5b68b1f35ffbbc974415f7 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:08:25 +0000 Subject: [PATCH 07/18] fix(engine): encode each image once for Office export Office export encoded and kept a separate copy of an image for every place a page drew it, so a short content stream repeating Do on one large image multiplied it into gigabytes before the writers' limits applied. Each image object is now encoded once and its bytes shared (pdfcraft_export::Image holds an Arc<[u8]>), distinct pictures share a 512 MiB budget whose overflow lists the page as truncated, and RTF, which leaves pictures out, prepares none. --- crates/engine/src/compare.rs | 50 +++++++++++++++---- crates/engine/src/tests.rs | 2 +- crates/engine/tests/it/main.rs | 1 + .../engine/tests/it/office_export_images.rs | 43 ++++++++++++++++ crates/export/src/lib.rs | 9 ++-- crates/export/src/pptx.rs | 10 +++- 6 files changed, 97 insertions(+), 18 deletions(-) create mode 100644 crates/engine/tests/it/office_export_images.rs diff --git a/crates/engine/src/compare.rs b/crates/engine/src/compare.rs index c23db91f1..095e666c1 100644 --- a/crates/engine/src/compare.rs +++ b/crates/engine/src/compare.rs @@ -1,8 +1,11 @@ //! Compare files: the text differences between two open documents, a report, and the //! differences marked as comments in the newer one. +use std::collections::HashMap; use std::sync::Arc; +use pdfcraft_cos::ObjRef; + pub use pdfcraft_compare::{Change, Comparison, Kind, Side}; // How far Office export reads into form XObjects; `OfficeExport::truncated_pages` lists the pages // that draw more. @@ -10,6 +13,10 @@ pub use pdfcraft_edit::{MAX_FORM_DEPTH, MAX_FORM_VISITS}; use crate::{DocId, Edit, EditError, Markup, NewAnnotation, NoteIcon, Session, Shape}; +/// Most bytes of encoded pictures one Office export prepares, counting each image object once +/// (the Word and PowerPoint writers keep at most 512 MiB of them as well). +pub const MAX_EXPORT_IMAGE_BYTES: usize = 512 << 20; + /// Acrobat's compare colours: replaced blue, inserted green, deleted red. pub fn colour(kind: Kind) -> crate::Rgb { match kind { @@ -163,23 +170,32 @@ pub struct OfficeExport { /// The exported file. pub bytes: Vec, /// Pages (0-based) that draw forms nested deeper than [`MAX_FORM_DEPTH`] or more of them - /// than [`MAX_FORM_VISITS`]: export stops reading there to stay bounded on hostile files, so - /// the text and images of those forms are missing from `bytes`. + /// than [`MAX_FORM_VISITS`], or pictures past [`MAX_EXPORT_IMAGE_BYTES`]: export stops + /// reading there to stay bounded on hostile files, so the text and images of those forms, or + /// those pictures, are missing from `bytes`. pub truncated_pages: Vec, } +/// An image object prepared for Office export: its file extension and encoded bytes, or why it is +/// left out (`Err(true)`: past [`MAX_EXPORT_IMAGE_BYTES`]; `Err(false)`: it can't be encoded). +type PreparedImage = Result<(&'static str, Arc<[u8]>), bool>; + impl crate::Document { /// The pages as paragraphs and images (for Word, HTML and RTF export), including what /// form XObjects draw. pub fn export_pages(&self) -> Vec { - self.export_pages_checked().0 + self.export_pages_checked(true).0 } - /// [`Self::export_pages`], and the pages whose forms were left partly unread - /// ([`OfficeExport::truncated_pages`]). - fn export_pages_checked(&self) -> (Vec, Vec) { + /// [`Self::export_pages`], and the pages whose forms were left partly unread or whose + /// pictures went past [`MAX_EXPORT_IMAGE_BYTES`] ([`OfficeExport::truncated_pages`]). Without + /// `images` no picture is prepared. + fn export_pages_checked(&self, images: bool) -> (Vec, Vec) { let Some(cos) = self.editor.as_ref().map(|e| &e.cos) else { return (Vec::new(), Vec::new()) }; let mut truncated = Vec::new(); + // Each image object is encoded once and shared by every place that draws it. + let mut encoded: HashMap = HashMap::new(); + let mut image_bytes = MAX_EXPORT_IMAGE_BYTES; let pages = self .info .pages @@ -217,13 +233,21 @@ impl crate::Document { } }) .collect(); - let images = pdfcraft_edit::reading_images(cos, i) - .unwrap_or_default() + let placed = if images { pdfcraft_edit::reading_images(cos, i).unwrap_or_default() } else { Vec::new() }; + let mut over_budget = false; + let images = placed .iter() .filter_map(|im| { - let (ext, bytes) = pdfcraft_create::image_file(cos, im.object?).ok()?; + let object = im.object?; + let prepared = encoded.entry(object).or_insert_with(|| { + let (ext, bytes) = pdfcraft_create::image_file(cos, object).map_err(|_| false)?; + image_bytes = image_bytes.checked_sub(bytes.len()).ok_or(true)?; + Ok((if ext == "jpg" { "jpg" } else { "png" }, Arc::from(bytes))) + }); + over_budget |= matches!(prepared, Err(true)); + let (ext, bytes) = prepared.clone().ok()?; Some(pdfcraft_export::Image { - ext: if ext == "jpg" { "jpg" } else { "png" }, + ext, bytes, rect: visible_rect(im.rect), size: [im.matrix[0].hypot(im.matrix[1]) * unit, im.matrix[2].hypot(im.matrix[3]) * unit], @@ -232,6 +256,9 @@ impl crate::Document { }) .collect(); // The page colour (Edit a PDF ▸ Background, a producer's page fill) for formats that keep one. + if over_budget { + truncated.push(i); + } let background = pdfcraft_edit::page_background(cos, i).ok().flatten(); pdfcraft_export::Page { width: info.width as f64, height: info.height as f64, blocks, images, background } }) @@ -247,7 +274,8 @@ impl crate::Document { /// [`Self::export_office`], and the pages whose text it had to leave out /// ([`OfficeExport::truncated_pages`]). pub fn export_office_checked(&self, format: OfficeFormat) -> OfficeExport { - let (pages, truncated_pages) = self.export_pages_checked(); + // RTF leaves pictures out. + let (pages, truncated_pages) = self.export_pages_checked(format != OfficeFormat::Rtf); let title = self.info.title.clone().unwrap_or_else(|| self.name.trim_end_matches(".pdf").to_string()); let bytes = match format { OfficeFormat::Docx => pdfcraft_export::docx(&pages, &title), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3d347e649..47ad482d4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6645,7 +6645,7 @@ fn exporting_a_jpeg_logo_to_word_keeps_the_image() { assert!(jpeg.starts_with(&[0xFF, 0xD8])); let docx = d.export_office(compare::OfficeFormat::Docx); assert!(docx.windows(21).any(|w| w == b"word/media/image1.jpg"), "overlap {overlaps}"); - assert!(docx.windows(jpeg.len()).any(|w| w == jpeg.as_slice()), "the JPEG is stored unchanged (overlap {overlaps})"); + assert!(docx.windows(jpeg.len()).any(|w| w == &jpeg[..]), "the JPEG is stored unchanged (overlap {overlaps})"); } } diff --git a/crates/engine/tests/it/main.rs b/crates/engine/tests/it/main.rs index 15e1a8548..8d9026600 100644 --- a/crates/engine/tests/it/main.rs +++ b/crates/engine/tests/it/main.rs @@ -8,5 +8,6 @@ mod document_fonts; mod form_rotation; mod marked_content_isolation; mod ocr_edit; +mod office_export_images; mod private_image_resize; mod private_text_glyphs; diff --git a/crates/engine/tests/it/office_export_images.rs b/crates/engine/tests/it/office_export_images.rs new file mode 100644 index 000000000..4f2ff618f --- /dev/null +++ b/crates/engine/tests/it/office_export_images.rs @@ -0,0 +1,43 @@ +//! Office export of pages that draw one image many times. + +use std::sync::Arc; + +use pdfcraft_engine::Session; + +fn pdf(objects: &[String]) -> Vec { + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objects.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objects.len() + 1).as_bytes()); + for o in offsets { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objects.len() + 1).as_bytes()); + out +} + +/// One image drawn many times is prepared once: every placement shares its encoded bytes. +#[test] +fn an_image_drawn_many_times_is_encoded_once() { + let placements = 500; + let content = "q 10 0 0 10 0 0 cm /Im Do Q\n".repeat(placements); + let bytes = pdf(&[ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".into(), + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 200 200] /Contents 4 0 R /Resources << /XObject << /Im 5 0 R >> >> >>".into(), + format!("<< /Length {} >>\nstream\n{content}endstream", content.len()), + "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /Length 4 >>\nstream\n\u{0}\u{40}\u{80}\u{ff}\nendstream".into(), + ]); + let mut s = Session::new(); + let id = s.open("many.pdf", None, Arc::new(bytes), None).unwrap(); + let pages = s.get(id).unwrap().export_pages(); + let images = &pages[0].images; + assert_eq!(images.len(), placements); + let first = images[0].bytes.as_ptr(); + assert!(!images[0].bytes.is_empty()); + assert!(images.iter().all(|im| im.bytes.as_ptr() == first), "each placement holds its own copy of the image"); +} diff --git a/crates/export/src/lib.rs b/crates/export/src/lib.rs index f3030ad58..b4ea9e965 100644 --- a/crates/export/src/lib.rs +++ b/crates/export/src/lib.rs @@ -35,7 +35,8 @@ pub struct Block { pub struct Image { /// "png" or "jpg". pub ext: &'static str, - pub bytes: Vec, + /// Shared: a picture drawn in many places is held once. + pub bytes: std::sync::Arc<[u8]>, pub rect: [f64; 4], /// Width and height before rotation, in points. pub size: [f64; 2], @@ -932,7 +933,7 @@ mod tests { ], images: vec![Image { ext: "png", - bytes: b"\x89PNG\r\n\x1a\n\0\0\0\rIHDR\0\0\0\x02\0\0\0\x03".to_vec(), + bytes: b"\x89PNG\r\n\x1a\n\0\0\0\rIHDR\0\0\0\x02\0\0\0\x03".as_slice().into(), rect: [72.0, 450.0, 272.0, 600.0], size: [200.0, 150.0], rotation: 0.0, @@ -1374,7 +1375,7 @@ mod tests { /// file in the package, with a content type; JPEG pictures too, inline or placed. #[test] fn word_pictures_resolve_to_media_in_the_package() { - let jpeg = Image { ext: "jpg", bytes: vec![0xFF, 0xD8, 0xFF, 0xD9], ..page().images[0].clone() }; + let jpeg = Image { ext: "jpg", bytes: [0xFF, 0xD8, 0xFF, 0xD9].as_slice().into(), ..page().images[0].clone() }; for placed in [false, true] { let mut p = page(); p.images = vec![jpeg.clone()]; @@ -1401,7 +1402,7 @@ mod tests { fn word_export_bounds_the_number_of_pictures_and_skips_empty_ones() { let mut p = page(); let one = p.images[0].clone(); - p.images = vec![Image { bytes: Vec::new(), ..one.clone() }]; + p.images = vec![Image { bytes: [].as_slice().into(), ..one.clone() }]; p.images.extend(std::iter::repeat_n(one, MAX_MEDIA + 10)); let zip = docx(&[p], "T"); let rels = part(&zip, "word/_rels/document.xml.rels"); diff --git a/crates/export/src/pptx.rs b/crates/export/src/pptx.rs index 1a7250d9e..886a4bdbd 100644 --- a/crates/export/src/pptx.rs +++ b/crates/export/src/pptx.rs @@ -391,7 +391,7 @@ mod tests { ], images: vec![Image { ext: "png", - bytes: b"\x89PNG\r\n\x1a\n\0\0\0\rIHDR\0\0\0\x02\0\0\0\x03".to_vec(), + bytes: b"\x89PNG\r\n\x1a\n\0\0\0\rIHDR\0\0\0\x02\0\0\0\x03".as_slice().into(), rect: [72.0, 150.0, 300.0, 300.0], size: [228.0, 150.0], rotation: 0.0, @@ -619,7 +619,13 @@ mod tests { blocks.push(huge("inf", f64::INFINITY, 200.0 - k as f64)); } let images = (0..40) - .map(|k| Image { ext: "png", bytes: vec![0; 4], rect: [0.0, -1e300, 1e-300, 1e300 - k as f64], size: [1.0, 1.0], rotation: 0.0 }) + .map(|k| Image { + ext: "png", + bytes: [0; 4].as_slice().into(), + rect: [0.0, -1e300, 1e-300, 1e300 - k as f64], + size: [1.0, 1.0], + rotation: 0.0, + }) .collect(); let p = Page { width: f64::MAX, height: f64::NAN, blocks, images, background: None }; let bytes = pptx(&[p], "Hostile"); From f2f86455ddb0aa538bcb9c159b87effe0342aa08 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:09:45 +0000 Subject: [PATCH 08/18] fix(engine): read long underscore runs in one pass Word assembly checked whether the whole word so far was underscores for every glyph it added, so a long run of underscores on one line took quadratic time. The underscore state of the current word is kept as it grows. --- crates/engine/src/js.rs | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/js.rs b/crates/engine/src/js.rs index a2c444bd6..c80537fba 100644 --- a/crates/engine/src/js.rs +++ b/crates/engine/src/js.rs @@ -289,6 +289,12 @@ pub(crate) fn page_words(text: &pdfcraft_render::PageText, info: &pdfcraft_rende .collect() } +#[cfg(test)] +thread_local! { + /// Characters read to tell underscore runs from other words. + static UNDERSCORE_CHARS_READ: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// A page's words in reading order, their boxes mapped from view space by `map` (and ordered /// again, so that [x0, y0, x1, y1] holds whatever way `map` turns them). pub(crate) fn words_with(text: &pdfcraft_render::PageText, map: impl Fn(f32, f32) -> [f32; 2]) -> Vec<(String, [f32; 4])> { @@ -296,6 +302,8 @@ pub(crate) fn words_with(text: &pdfcraft_render::PageText, map: impl Fn(f32, f32 let mut last_line = u32::MAX; // A space glyph ends the word before it. let mut gap = false; + // Whether the last word is an underscore run (a form's blank), which other glyphs don't join. + let mut last_under = false; for (i, g) in text.glyphs.iter().enumerate() { let line = text.line_of.get(i).copied().unwrap_or(0); if g.text.trim().is_empty() { @@ -303,10 +311,9 @@ pub(crate) fn words_with(text: &pdfcraft_render::PageText, map: impl Fn(f32, f32 continue; } let under = g.text.chars().all(|c| c == '_'); - let breaks = match words.last() { - None => true, - Some((w, _)) => gap || line != last_line || text.space_before.get(i).copied().unwrap_or(false) || w.chars().all(|c| c == '_') != under, - }; + #[cfg(test)] + UNDERSCORE_CHARS_READ.with(|n| n.set(n.get() + g.text.len())); + let breaks = words.is_empty() || gap || line != last_line || text.space_before.get(i).copied().unwrap_or(false) || last_under != under; gap = false; last_line = line; match words.last_mut() { @@ -317,7 +324,10 @@ pub(crate) fn words_with(text: &pdfcraft_render::PageText, map: impl Fn(f32, f32 r[2] = r[2].max(g.rect[2]); r[3] = r[3].max(g.rect[3]); } - _ => words.push((g.text.clone(), g.rect)), + _ => { + words.push((g.text.clone(), g.rect)); + last_under = under; + } } } words @@ -409,6 +419,27 @@ impl Session { mod review_tests { use super::*; + /// A long run of underscores (a form's blank) is read in one pass: each glyph is compared with + /// the word so far without reading the word again. + #[test] + fn a_long_underscore_run_is_read_in_one_pass() { + let n = 10_000; + let glyph = |i: usize, t: &str| pdfcraft_render::TextGlyph { + text: t.into(), + rect: [i as f32, 0.0, i as f32 + 1.0, 10.0], + quad: [[0.0; 2]; 4], + direction: [1.0, 0.0], + }; + let mut glyphs: Vec<_> = (0..n).map(|i| glyph(i, "_")).collect(); + glyphs.push(glyph(n, "x")); + let text = pdfcraft_render::PageText { line_of: vec![0; glyphs.len()], space_before: vec![false; glyphs.len()], glyphs }; + UNDERSCORE_CHARS_READ.with(|c| c.set(0)); + let words = words_with(&text, |x, y| [x, y]); + assert_eq!(words.iter().map(|(w, _)| w.len()).collect::>(), [n, 1]); + let read = UNDERSCORE_CHARS_READ.with(std::cell::Cell::get); + assert!(read <= 4 * n, "{read} characters read for {n} glyphs"); + } + #[test] fn duplicate_field_names_diff_against_first_original() { let first = pdfcraft_js::FieldState::new("duplicate", pdfcraft_js::FieldType::Text, vec!["first".into()]); From 2d5c5054e22b77e1bce18bd43cf8398dfc93f2f9 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:11:06 +0000 Subject: [PATCH 09/18] fix(forms): merge a calculation's read names with a set calculation_order merged the names a Calculate script reads into the field's other reads by searching the list so far for each name, so a script naming many fields took quadratic time before the link limits applied. The merge looks names up in a set. --- crates/forms/src/calc_order.rs | 11 ++++++++++- crates/forms/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/forms/src/calc_order.rs b/crates/forms/src/calc_order.rs index e75a05d47..428c1724d 100644 --- a/crates/forms/src/calc_order.rs +++ b/crates/forms/src/calc_order.rs @@ -122,6 +122,12 @@ pub(crate) fn run_order(doc: &Document, fields: &[Field]) -> (Vec<(usize, bool)> (run, extra) } +#[cfg(test)] +thread_local! { + /// Read names compared while merging a field's reads. + pub(crate) static NAMES_COMPARED: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// The form's calculation order (`fields` as [`fields`] reads them), with each calculated field's /// reads and issues, and Fix order's order. pub fn calculation_order(doc: &Document, fields: &[Field]) -> CalculationOrder { @@ -132,8 +138,11 @@ pub fn calculation_order(doc: &Document, fields: &[Field]) -> CalculationOrder { .map(|(f, _)| { let mut names = f.actions.calculate.reads(); if let Some(js) = &f.actions.scripts.calculate { + let mut known: HashSet = names.iter().cloned().collect(); for name in crate::af::calculation_reads(js) { - if !names.contains(&name) { + #[cfg(test)] + NAMES_COMPARED.with(|n| n.set(n.get() + 1)); + if known.insert(name.clone()) { names.push(name); } } diff --git a/crates/forms/src/tests.rs b/crates/forms/src/tests.rs index b2e4994ec..2b2042865 100644 --- a/crates/forms/src/tests.rs +++ b/crates/forms/src/tests.rs @@ -2981,3 +2981,20 @@ fn page_walk_skips_repeated_and_cyclic_kids_without_queueing_them() { assert_eq!(walk.next(), None); assert_eq!(page_refs(&doc), [page]); } + +/// A Calculate script naming thousands of fields is merged with the field's other reads in one +/// pass, not by searching the names so far for each one. +#[test] +fn a_script_reading_many_fields_merges_its_reads_in_one_pass() { + use crate::calc_order::NAMES_COMPARED; + let mut doc = one_page(); + add_field(&mut doc, 0, [50.0, 700.0, 250.0, 720.0], &NewField::Text { multiline: false }, Some("total")).unwrap(); + let n = 2_000; + let script: String = (0..n).map(|i| format!("getField(\"f{i}\").value + ")).collect::() + "0"; + set_field_script(&mut doc, "total", "calculate", Some(&format!("event.value = {script};"))).unwrap(); + NAMES_COMPARED.with(|c| c.set(0)); + let order = calculation_order(&doc, &fields(&doc)); + assert_eq!(order.fields[0].reads.len(), n); + let compared = NAMES_COMPARED.with(std::cell::Cell::get); + assert!(compared <= 4 * n, "{compared} names compared for {n} reads"); +} From b6cb15177484df065f7aa87231cf2e4f63785127 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:12:06 +0000 Subject: [PATCH 10/18] fix(measure): read a stream's XObject resources once when snapping Every Do operator resolved the stream's /XObject resources again, and resolving a direct dictionary copies it, so a large direct XObject dictionary and many short Do operators multiplied the copying. The dictionary is read on the first Do and kept for the rest of the stream. --- crates/measure/src/snap.rs | 14 +++++++++++++- crates/measure/src/tests.rs | 11 +++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/measure/src/snap.rs b/crates/measure/src/snap.rs index 3cf6bbd17..37ee2975f 100644 --- a/crates/measure/src/snap.rs +++ b/crates/measure/src/snap.rs @@ -250,6 +250,8 @@ impl Walker<'_> { let mut current = None; let mut closures = Vec::new(); let mut path = Geometry::default(); + // Read on the first `Do` and kept: resolving a direct dictionary copies it. + let mut xobjects = None; let point = |m: Matrix, x: f64, y: f64| { let (x, y) = m.apply(x, y); [x, y] @@ -384,7 +386,11 @@ impl Walker<'_> { } "Do" => { if let Some(name) = op.name(0) { - let xobjects = resources.get(b"XObject").map(|o| self.doc.resolve(o)); + let xobjects = xobjects.get_or_insert_with(|| { + #[cfg(test)] + XOBJECT_DICT_READS.with(|n| n.set(n.get() + 1)); + resources.get(b"XObject").map(|o| self.doc.resolve(o)) + }); if let Some(o) = xobjects.as_ref().and_then(|o| o.as_dict()).and_then(|d| d.get(name)) { let r = o.as_ref(); if r.is_some_and(|r| self.seen.contains(&r)) { @@ -420,6 +426,12 @@ impl Walker<'_> { Ok(()) } } +#[cfg(test)] +thread_local! { + /// Times a content stream's `/XObject` resources were read. + pub(crate) static XOBJECT_DICT_READS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + pub fn geometry(doc: &Document, page_index: usize) -> Result { let p = crate::page(doc, page_index)?; let resources = p.dict.get(b"Resources").map(|o| doc.resolve(o)); diff --git a/crates/measure/src/tests.rs b/crates/measure/src/tests.rs index 303f9b438..06bc1e42d 100644 --- a/crates/measure/src/tests.rs +++ b/crates/measure/src/tests.rs @@ -403,3 +403,14 @@ fn a_scale_ratio_converts_its_page_side_to_points() { assert!(Scale::ratio([0.0, 0.0], [1.0, 0.0], page, PageUnit::Mm, real, "mm", 2).is_err(), "{page} {real}"); } } +/// A content stream reads its `/XObject` resources once, not again (as a copy of a direct +/// dictionary) for every `Do`. +#[test] +fn xobject_resources_are_read_once_per_stream() { + let d = fixture(&"/Missing Do\n".repeat(10_000), "", &[]); + snap::XOBJECT_DICT_READS.with(|c| c.set(0)); + let paths = snap::geometry(&d, 0).unwrap(); + assert!(paths.segments.is_empty()); + let reads = snap::XOBJECT_DICT_READS.with(std::cell::Cell::get); + assert!(reads <= 1, "{reads} reads of the page's XObject resources"); +} From 240d051b4e9973c5d980560657f6377c63f0856b Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:12:43 +0000 Subject: [PATCH 11/18] fix(measure): read viewports within bounds when finding a scale scale_at converted every number of a viewport's /BBox before checking it had four, and read every viewport the page listed, so many viewports sharing one long box array took time proportional to their product. A box of any other length is skipped unread, and at most 1024 viewports are read, the most set_scale gives a page. --- crates/measure/src/lib.rs | 20 +++++++++++++++++--- crates/measure/src/tests.rs | 13 +++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/crates/measure/src/lib.rs b/crates/measure/src/lib.rs index 72b6c4b07..dda45a75a 100644 --- a/crates/measure/src/lib.rs +++ b/crates/measure/src/lib.rs @@ -497,6 +497,15 @@ pub fn list(doc: &Document) -> Listing { } out } +/// Most viewports [`set_scale`] gives a page, and most [`scale_at`] reads. +const MAX_VIEWPORTS: usize = 1024; + +#[cfg(test)] +thread_local! { + /// Numbers read from viewport `/BBox` arrays. + pub(crate) static VIEWPORT_NUMBERS_READ: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + /// Last containing viewport wins, using the first point of the measurement (ISO 32000-2 §12.9). /// Outside every viewport the page measures true size in inches. pub fn scale_at(doc: &Document, page_index: usize, at: Point) -> Result { @@ -510,12 +519,17 @@ pub fn scale_at_or(doc: &Document, page_index: usize, at: Point, per_point: f64, if let Some(o) = p.dict.get(b"VP") { let o = doc.resolve(o); let arr = o.as_array().ok_or_else(|| invalid("invalid page viewports"))?; - for viewport in arr.iter().rev() { + // The last viewports win; a page holds at most MAX_VIEWPORTS of its own. + for viewport in arr.iter().rev().take(MAX_VIEWPORTS) { let o = doc.resolve(viewport); let Some(d) = o.as_dict() else { continue }; if let Some(bbox) = d.get(b"BBox") { let o = doc.resolve(bbox); - let b: Option> = o.as_array().and_then(|a| a.iter().map(|v| doc.resolve(v).as_f64()).collect()); + let b: Option> = o.as_array().filter(|a| a.len() == 4).and_then(|a| { + #[cfg(test)] + VIEWPORT_NUMBERS_READ.with(|n| n.set(n.get() + a.len())); + a.iter().map(|v| doc.resolve(v).as_f64()).collect() + }); if let Some(b) = b.filter(|b| b.len() == 4 && b.iter().all(|v| v.is_finite())) && at[0] >= b[0].min(b[2]) && at[0] <= b[0].max(b[2]) @@ -541,7 +555,7 @@ pub fn set_scale(doc: &mut Document, page_index: usize, bbox: [f64; 4], name: &s Some(o) => doc.resolve(o).as_array().cloned().ok_or_else(|| invalid("invalid page viewports"))?, None => Vec::new(), }; - if list.len() >= 1024 { + if list.len() >= MAX_VIEWPORTS { return Err(invalid("page has too many measurement viewports")); } let mut d = Dict::new(); diff --git a/crates/measure/src/tests.rs b/crates/measure/src/tests.rs index 06bc1e42d..259caeb5f 100644 --- a/crates/measure/src/tests.rs +++ b/crates/measure/src/tests.rs @@ -414,3 +414,16 @@ fn xobject_resources_are_read_once_per_stream() { let reads = snap::XOBJECT_DICT_READS.with(std::cell::Cell::get); assert!(reads <= 1, "{reads} reads of the page's XObject resources"); } +/// Finding the scale under a point reads only well-formed viewport boxes, and only as many +/// viewports as a page can be given, however many a file lists or however long their boxes. +#[test] +fn scale_lookup_reads_viewports_within_bounds() { + let vp = vec!["5 0 R"; 5_000].join(" "); + let long_box = vec!["0"; 2_000].join(" "); + let d = fixture("", &format!("/VP [{vp}]"), &["<< /Type /Viewport /BBox 6 0 R >>", &format!("[{long_box}]")]); + VIEWPORT_NUMBERS_READ.with(|c| c.set(0)); + let scale = scale_at(&d, 0, [10.0, 10.0]).unwrap(); + assert_eq!(scale.unit, "in"); + let read = VIEWPORT_NUMBERS_READ.with(std::cell::Cell::get); + assert!(read <= 4 * 1024, "{read} viewport numbers read"); +} From 05d09de1e337c3dc40bac6286721fe85239e6c8b Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:13:45 +0000 Subject: [PATCH 12/18] fix(annot): read a shared page tree Kids array once The page walk pushed a node's /Kids array for every new node, so many page tree nodes sharing one indirect /Kids array re-read it for each node: quadratic work while the seen check skipped every entry. A shared array object is read under the first node that lists it, and the walk stops after four million /Kids entries. --- crates/annot/src/lib.rs | 21 ++++++++++++++++----- crates/annot/src/tests.rs | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index 6edfbf9cb..61dd5ff1c 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -665,17 +665,20 @@ pub fn page_refs(doc: &Document) -> Result, AnnotError> { /// Depth-first walk over the leaf pages under `top`. It keeps one cursor per open `/Kids` array, /// never one entry per child, so a child listed many times or shared between nodes is skipped by /// the seen check without being queued first (`pdfcraft_model::pages` walks the same way). -/// Cycles end at the seen check and at most a million nodes are visited. +/// Cycles end at the seen check, a `/Kids` array object shared by several nodes is read once, +/// and at most a million nodes and four million `/Kids` entries are read. struct PageWalk<'a> { doc: &'a Document, seen: std::collections::HashSet, pending: Vec<(std::sync::Arc, usize)>, + /// `/Kids` entries read so far. + examined: usize, } impl<'a> PageWalk<'a> { fn new(doc: &'a Document, top: ObjRef) -> Self { let top = std::sync::Arc::new(Object::Array(vec![Object::Ref(top)])); - Self { doc, seen: Default::default(), pending: vec![(top, 0)] } + Self { doc, seen: Default::default(), pending: vec![(top, 0)], examined: 0 } } } @@ -690,8 +693,9 @@ impl Iterator for PageWalk<'_> { }; let node = kid.as_ref(); *next += 1; + self.examined += 1; let Some(node) = node else { continue }; - if self.seen.len() >= 1_000_000 { + if self.seen.len() >= 1_000_000 || self.examined > 4_000_000 { self.pending.clear(); return None; } @@ -700,8 +704,15 @@ impl Iterator for PageWalk<'_> { } let obj = self.doc.get(node); let Some(d) = obj.as_dict() else { continue }; - match d.get(b"Kids").map(|k| self.doc.resolve(k)) { - Some(kids) if d.name(b"Type") != Some(b"Page") => self.pending.push((kids, 0)), + match d.get(b"Kids") { + // A `/Kids` array object shared by several nodes is read under the first: under + // the others it would only list nodes already seen. + Some(Object::Ref(array)) if d.name(b"Type") != Some(b"Page") => { + if self.seen.insert(*array) { + self.pending.push((self.doc.get(*array), 0)); + } + } + Some(kids) if d.name(b"Type") != Some(b"Page") => self.pending.push((self.doc.resolve(kids), 0)), _ => return Some(node), } } diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index a65996681..44328bbd0 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1504,3 +1504,25 @@ fn typed_signature_uses_nonzero_winding_fill() { assert!(ap.ends_with("h\nf\n"), "expected non-zero winding fill (`f`), got: {ap}"); assert!(!ap.contains("f*"), "must not use even-odd fill (`f*`): {ap}"); } + +/// Many page tree nodes sharing one `/Kids` array read it once, not once per node. +#[test] +fn page_walk_reads_a_shared_kids_array_once() { + let mut doc = fixture(); + let root = ObjRef { num: 2, generation: 0 }; + let (first, second) = (ObjRef { num: 3, generation: 0 }, ObjRef { num: 4, generation: 0 }); + let shared = doc.add(Object::Array([Object::Ref(first), Object::Ref(second)].into_iter().cycle().take(2_000).collect())); + let nodes: Vec = (0..1_000) + .map(|_| { + let mut d = Dict::new(); + d.set(b"Type".to_vec(), Object::name("Pages")); + d.set(b"Kids".to_vec(), Object::Ref(shared)); + Object::Ref(doc.add(d)) + }) + .collect(); + doc.update_dict(root, |d| d.set(b"Kids".to_vec(), Object::Array(nodes))).unwrap(); + let mut walk = PageWalk::new(&doc, root); + let pages: Vec = walk.by_ref().collect(); + assert_eq!(pages, [first, second]); + assert!(walk.examined <= 10_000, "{} kids entries read", walk.examined); +} From c2e96de0b240f6d41ef340cc50c29d9e74198232 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 16:14:34 +0000 Subject: [PATCH 13/18] fix(annot): extend each page's annotation list once when adding many links add_many added links one at a time, and each addition walked the page tree and copied and rewrote the page's whole /Annots array, so a batch of links cost quadratic time. The pages are found once and each page's new links are appended in one update. --- crates/annot/src/lib.rs | 11 ++++++++++- crates/annot/src/links.rs | 37 ++++++++++++++++++++++++++++++------- crates/annot/src/tests.rs | 15 +++++++++++++++ 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index 61dd5ff1c..d1d74ce41 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -728,7 +728,16 @@ fn page_ref(doc: &Document, page: usize) -> Result { fn annots(doc: &Document, page: ObjRef) -> Vec { let obj = doc.get(page); let Some(a) = obj.as_dict().and_then(|d| d.get(b"Annots")) else { return Vec::new() }; - doc.resolve(a).as_array().cloned().unwrap_or_default() + let list = doc.resolve(a).as_array().cloned().unwrap_or_default(); + #[cfg(test)] + ANNOTS_ENTRIES_COPIED.with(|n| n.set(n.get() + list.len())); + list +} + +#[cfg(test)] +thread_local! { + /// Entries copied out of pages' `/Annots` arrays. + static ANNOTS_ENTRIES_COPIED: std::cell::Cell = const { std::cell::Cell::new(0) }; } /// Replace a page's `/Annots`, writing into the shared array object when it is indirect. diff --git a/crates/annot/src/links.rs b/crates/annot/src/links.rs index 4d3eef632..f11eecbd8 100644 --- a/crates/annot/src/links.rs +++ b/crates/annot/src/links.rs @@ -232,6 +232,16 @@ fn checked_rect(rect: [f64; 4]) -> Result<[f64; 4], AnnotError> { /// Add a link over `rect` (user space). Returns its index in the page's `/Annots`. pub fn add(doc: &mut Document, page: usize, rect: [f64; 4], action: &LinkAction, style: &LinkStyle) -> Result { let p = page_ref(doc, page)?; + let obj = new_link(doc, p, rect, action, style)?; + let mut list = annots(doc, p); + list.push(Object::Ref(obj)); + let index = list.len() - 1; + set_annots(doc, p, list)?; + Ok(index) +} + +/// A link annotation object for page `p`, not yet on the page's `/Annots`. +fn new_link(doc: &mut Document, p: ObjRef, rect: [f64; 4], action: &LinkAction, style: &LinkStyle) -> Result { let r = checked_rect(rect)?; if matches!(action, LinkAction::Other(_)) { return Err(AnnotError::Invalid("choose a page or a web address".into())); @@ -244,12 +254,7 @@ pub fn add(doc: &mut Document, page: usize, rect: [f64; 4], action: &LinkAction, d.set(b"F".to_vec(), Object::Int(4)); write_style(&mut d, style); write_action(doc, &mut d, action)?; - let obj = doc.add(Object::Dict(d)); - let mut list = annots(doc, p); - list.push(Object::Ref(obj)); - let index = list.len() - 1; - set_annots(doc, p, list)?; - Ok(index) + Ok(doc.add(Object::Dict(d))) } fn link_at(doc: &Document, page: usize, index: usize) -> Result<(ObjRef, ObjRef), AnnotError> { @@ -414,12 +419,30 @@ pub fn find_urls(chars: &[char]) -> Vec<(std::ops::Range, String)> { /// Create links for found URLs: `(page, quads, uri)` with quads in user space (one link per /// line rectangle). Returns how many links were added. pub fn add_many(doc: &mut Document, links: &[(usize, Vec<[f64; 4]>, String)], style: &LinkStyle) -> Result { + let pages = page_refs(doc)?; + // Each page's `/Annots` is extended once, with its new links in the order given. + let mut added: Vec<(ObjRef, Vec)> = Vec::new(); + let mut slot: std::collections::HashMap = std::collections::HashMap::new(); let mut n = 0; for (page, rects, uri) in links { + let p = pages.get(*page).copied().ok_or(AnnotError::NoSuchPage(*page))?; + let action = LinkAction::Uri(uri.clone()); for r in rects { - add(doc, *page, *r, &LinkAction::Uri(uri.clone()), style)?; + let obj = new_link(doc, p, *r, &action, style)?; + let k = *slot.entry(p).or_insert_with(|| { + added.push((p, Vec::new())); + added.len() - 1 + }); + if let Some((_, list)) = added.get_mut(k) { + list.push(Object::Ref(obj)); + } n += 1; } } + for (p, new) in added { + let mut list = annots(doc, p); + list.extend(new); + set_annots(doc, p, list)?; + } Ok(n) } diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 44328bbd0..cfd80093e 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1526,3 +1526,18 @@ fn page_walk_reads_a_shared_kids_array_once() { assert_eq!(pages, [first, second]); assert!(walk.examined <= 10_000, "{} kids entries read", walk.examined); } + +/// Adding many links to a page extends its `/Annots` once, not once per link. +#[test] +fn many_links_extend_a_page_annotation_list_once() { + let mut doc = fixture(); + let before = annots(&doc, page_refs(&doc).unwrap()[0]).len(); + let n = 2_000; + let rects: Vec<[f64; 4]> = (0..n).map(|i| [10.0, i as f64 * 0.1, 50.0, i as f64 * 0.1 + 5.0]).collect(); + ANNOTS_ENTRIES_COPIED.with(|c| c.set(0)); + let added = links::add_many(&mut doc, &[(0, rects, "https://example.com".into())], &links::LinkStyle::default()).unwrap(); + let copied = ANNOTS_ENTRIES_COPIED.with(std::cell::Cell::get); + assert_eq!(added, n); + assert_eq!(annots(&doc, page_refs(&doc).unwrap()[0]).len(), before + n); + assert!(copied <= 4 * n, "{copied} annotation entries copied for {n} links"); +} From 8adf8e2d3b4a86aaa688f3347e07560f85d11530 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 17:58:47 +0000 Subject: [PATCH 14/18] fix(xfdf): read an indirect /F when deciding a comment is locked against import --- crates/xfdf/src/lib.rs | 7 ++++--- crates/xfdf/src/tests.rs | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/crates/xfdf/src/lib.rs b/crates/xfdf/src/lib.rs index 99470b58a..5c0be5232 100644 --- a/crates/xfdf/src/lib.rs +++ b/crates/xfdf/src/lib.rs @@ -1037,8 +1037,9 @@ const LOCKED: i64 = 128; /// An existing annotation an import must never replace, move or unlock: one the user locked, a /// redaction mark (replacing it would drop the region the user marked to remove), or a form /// widget or field (replacing it would set the field's value and policy around the form setter). -fn import_protected(x: &Dict) -> bool { - x.int(b"F").unwrap_or(0) & LOCKED != 0 || matches!(x.name(b"Subtype"), Some(b"Redact" | b"Widget")) || x.contains(b"FT") +fn import_protected(doc: &Document, x: &Dict) -> bool { + let flags = x.get(b"F").and_then(|f| doc.resolve(f).as_int()).unwrap_or(0); + flags & LOCKED != 0 || matches!(x.name(b"Subtype"), Some(b"Redact" | b"Widget")) || x.contains(b"FT") } fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result { @@ -1089,7 +1090,7 @@ fn place(doc: &mut Document, page_ref: ObjRef, mut d: Dict) -> Result"#; + assert_eq!(import(&mut doc, xfdf).unwrap().comments, 1); + let annots = doc.get(page).as_dict().and_then(|p| p.get(b"Annots").and_then(Object::as_array).cloned()).unwrap(); + assert!(annots.contains(&Object::Ref(note)), "the locked comment was replaced"); + assert_eq!(doc.get(note), before, "the locked comment was changed"); +} From 6394779b81f7ff7c65df139aff12a1a617c41c95 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 20:51:55 +0000 Subject: [PATCH 15/18] fix(annot): check a whole link batch before creating any link object --- crates/annot/src/links.rs | 10 ++++++++++ crates/annot/src/tests.rs | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/crates/annot/src/links.rs b/crates/annot/src/links.rs index f11eecbd8..95d408058 100644 --- a/crates/annot/src/links.rs +++ b/crates/annot/src/links.rs @@ -420,6 +420,16 @@ pub fn find_urls(chars: &[char]) -> Vec<(std::ops::Range, String)> { /// line rectangle). Returns how many links were added. pub fn add_many(doc: &mut Document, links: &[(usize, Vec<[f64; 4]>, String)], style: &LinkStyle) -> Result { let pages = page_refs(doc)?; + // Check the whole batch first, so a bad entry fails it before any link object is created. + for (page, rects, uri) in links { + pages.get(*page).ok_or(AnnotError::NoSuchPage(*page))?; + if uri.trim().is_empty() { + return Err(AnnotError::Invalid("type the web address first".into())); + } + for r in rects { + checked_rect(*r)?; + } + } // Each page's `/Annots` is extended once, with its new links in the order given. let mut added: Vec<(ObjRef, Vec)> = Vec::new(); let mut slot: std::collections::HashMap = std::collections::HashMap::new(); diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index cfd80093e..017e06671 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -1541,3 +1541,21 @@ fn many_links_extend_a_page_annotation_list_once() { assert_eq!(annots(&doc, page_refs(&doc).unwrap()[0]).len(), before + n); assert!(copied <= 4 * n, "{copied} annotation entries copied for {n} links"); } + +/// A batch with one bad entry adds nothing: no link objects are left behind unattached. +#[test] +fn a_link_batch_with_a_bad_entry_creates_no_objects() { + let mut doc = fixture(); + let page = page_refs(&doc).unwrap()[0]; + let before = (annots(&doc, page).len(), doc.scan_objects().count()); + let good = [10.0, 10.0, 50.0, 20.0]; + let style = links::LinkStyle::default(); + for bad in [ + vec![(0, vec![good], "https://example.com".to_string()), (0, vec![good, [f64::NAN, 0.0, 10.0, 10.0]], "https://example.com".into())], + vec![(0, vec![good], "https://example.com".to_string()), (0, vec![good], " ".into())], + vec![(0, vec![good], "https://example.com".to_string()), (99, vec![good], "https://example.com".into())], + ] { + assert!(links::add_many(&mut doc, &bad, &style).is_err()); + assert_eq!((annots(&doc, page).len(), doc.scan_objects().count()), before); + } +} From aa1c13f71ce8043c76899c108960eaa3be0dec8b Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 20:56:02 +0000 Subject: [PATCH 16/18] fix(engine): list a page truncated for both forms and pictures once --- crates/engine/src/compare.rs | 18 +++++++-------- crates/engine/src/tests.rs | 44 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/compare.rs b/crates/engine/src/compare.rs index 095e666c1..1326ccb5e 100644 --- a/crates/engine/src/compare.rs +++ b/crates/engine/src/compare.rs @@ -184,18 +184,18 @@ impl crate::Document { /// The pages as paragraphs and images (for Word, HTML and RTF export), including what /// form XObjects draw. pub fn export_pages(&self) -> Vec { - self.export_pages_checked(true).0 + self.export_pages_checked(true, MAX_EXPORT_IMAGE_BYTES).0 } /// [`Self::export_pages`], and the pages whose forms were left partly unread or whose - /// pictures went past [`MAX_EXPORT_IMAGE_BYTES`] ([`OfficeExport::truncated_pages`]). Without + /// pictures went past `image_budget` bytes ([`OfficeExport::truncated_pages`]). Without /// `images` no picture is prepared. - fn export_pages_checked(&self, images: bool) -> (Vec, Vec) { + pub(crate) fn export_pages_checked(&self, images: bool, image_budget: usize) -> (Vec, Vec) { let Some(cos) = self.editor.as_ref().map(|e| &e.cos) else { return (Vec::new(), Vec::new()) }; let mut truncated = Vec::new(); // Each image object is encoded once and shared by every place that draws it. let mut encoded: HashMap = HashMap::new(); - let mut image_bytes = MAX_EXPORT_IMAGE_BYTES; + let mut image_bytes = image_budget; let pages = self .info .pages @@ -213,9 +213,7 @@ impl crate::Document { let display_width = f64::from(if info.rotation % 180 == 0 { info.width } else { info.height }); let unit = if crop_width > 0.0 && crop_width.is_finite() && display_width.is_finite() { display_width / crop_width } else { 1.0 }; let reading = pdfcraft_edit::reading_blocks_checked(cos, i).unwrap_or_default(); - if reading.truncated { - truncated.push(i); - } + let forms_unread = reading.truncated; let blocks = reading .blocks .into_iter() @@ -255,10 +253,10 @@ impl crate::Document { }) }) .collect(); - // The page colour (Edit a PDF ▸ Background, a producer's page fill) for formats that keep one. - if over_budget { + if forms_unread || over_budget { truncated.push(i); } + // The page colour (Edit a PDF ▸ Background, a producer's page fill) for formats that keep one. let background = pdfcraft_edit::page_background(cos, i).ok().flatten(); pdfcraft_export::Page { width: info.width as f64, height: info.height as f64, blocks, images, background } }) @@ -275,7 +273,7 @@ impl crate::Document { /// ([`OfficeExport::truncated_pages`]). pub fn export_office_checked(&self, format: OfficeFormat) -> OfficeExport { // RTF leaves pictures out. - let (pages, truncated_pages) = self.export_pages_checked(format != OfficeFormat::Rtf); + let (pages, truncated_pages) = self.export_pages_checked(format != OfficeFormat::Rtf, MAX_EXPORT_IMAGE_BYTES); let title = self.info.title.clone().unwrap_or_else(|| self.name.trim_end_matches(".pdf").to_string()); let bytes = match format { OfficeFormat::Docx => pdfcraft_export::docx(&pages, &title), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 412bd15de..3df5279ab 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -7283,3 +7283,47 @@ fn exporting_office_keeps_dotted_leader_entries_as_paragraphs() { let positions: Vec<_> = entries.iter().map(|entry| html.find(&format!("

{entry}

")).expect("one paragraph per entry")).collect(); assert!(positions.windows(2).all(|pair| pair[0] < pair[1]), "reading order: {html}"); } + +/// A page whose forms go past the reading limit and whose picture goes past the image budget is +/// listed once among the truncated pages. +#[test] +fn a_page_truncated_for_forms_and_pictures_is_listed_once() { + let depth = compare::MAX_FORM_DEPTH + 1; + let mut objs: Vec = vec![ + "<< /Type /Catalog /Pages 2 0 R >>".into(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>".into(), + String::new(), + "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /Length 4 >>\nstream\n\0\x40\u{80}\u{ff}\nendstream".into(), + "<< /Length 34 >>\nstream\nq 10 0 0 10 0 0 cm /Im Do Q /D1 Do\nendstream".into(), + ]; + // Forms D1..D{depth}, objects 6 onwards, each drawing the next. + let xobjects: String = (1..=depth).map(|i| format!("/D{i} {} 0 R ", 5 + i)).collect(); + let resources = format!("<< /XObject << /Im 4 0 R {xobjects}>> >>"); + for i in 1..=depth { + let body = if i == depth { "q Q".to_string() } else { format!("/D{} Do", i + 1) }; + objs.push(format!( + "<< /Type /XObject /Subtype /Form /BBox [0 0 200 200] /Resources {resources} /Length {} >>\nstream\n{body}\nendstream", + body.len() + )); + } + objs[2] = format!("<< /Type /Page /Parent 2 0 R /Contents 5 0 R /Resources {resources} >>"); + let mut bytes = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(bytes.len()); + bytes.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = bytes.len(); + bytes.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + bytes.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + bytes.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + let mut s = Session::new(); + let id = s.open("both.pdf", None, Arc::new(bytes), None).unwrap(); + let doc = s.get(id).unwrap(); + assert_eq!(doc.export_pages_checked(false, 0).1, [0], "the forms alone truncate the page"); + let (pages, truncated) = doc.export_pages_checked(true, 0); + assert!(pages[0].images.is_empty(), "the picture is past the budget"); + assert_eq!(truncated, [0]); +} From 62b54fce098b7ab5786350948339ef5741c55057 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 20:56:33 +0000 Subject: [PATCH 17/18] fix(preflight): check the actions in an indirect /Next array --- crates/preflight/src/lib.rs | 43 +++++++++++++++++++++++++++-------- crates/preflight/src/tests.rs | 2 ++ 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/crates/preflight/src/lib.rs b/crates/preflight/src/lib.rs index 905ba8cbf..afd6a3bab 100644 --- a/crates/preflight/src/lib.rs +++ b/crates/preflight/src/lib.rs @@ -520,7 +520,7 @@ pub fn verify(doc: &Document, level: Level) -> Vec { let own = (d.get(b"S").is_some() && is_action(d)).then_some(d); let direct = [b"A".as_slice(), b"OpenAction"].map(|k| d.get(k).and_then(|a| a.as_dict())); for a in [own].into_iter().chain(direct).flatten() { - if let Some(s) = chain_forbidden(a) { + if let Some(s) = chain_forbidden(doc, a) { issue("6.5.1", "A {} action isn't allowed", Some(s), None, true); } } @@ -659,14 +659,31 @@ fn is_action(d: &Dict) -> bool { const MAX_ACTION_CHAIN: usize = 1024; /// The first forbidden action type in the action `a` or in the actions written directly in its -/// `/Next` (ISO 32000-2 12.6.2: a dictionary or an array of them). An action `/Next` refers to -/// is an object of its own, checked and removed as one. -fn chain_forbidden(a: &Dict) -> Option { - let mut stack = vec![a]; +/// `/Next` (ISO 32000-2 12.6.2: a dictionary or an array of them, which may be an object of its +/// own). An action `/Next` refers to is an object of its own, checked and removed as one. +fn chain_forbidden(doc: &Document, a: &Dict) -> Option { let mut read = 0; + let mut seen = std::collections::HashSet::new(); + // Actions written in an indirect `/Next` array, copied out of it. + let mut held: Vec = Vec::new(); + if let Some(s) = walk_chain(doc, a, &mut read, &mut seen, &mut held) { + return Some(s); + } + while let Some(d) = held.pop() { + if let Some(s) = walk_chain(doc, &d, &mut read, &mut seen, &mut held) { + return Some(s); + } + } + None +} + +/// [`chain_forbidden`] for the actions reachable from `a` through direct `/Next` entries. The +/// actions of each indirect `/Next` array (each read once) go to `held`. +fn walk_chain(doc: &Document, a: &Dict, read: &mut usize, seen: &mut std::collections::HashSet, held: &mut Vec) -> Option { + let mut stack = vec![a]; while let Some(d) = stack.pop() { - read += 1; - if read > MAX_ACTION_CHAIN { + *read += 1; + if *read > MAX_ACTION_CHAIN { return Some("too deeply chained".into()); } if let Some(s) = d.name(b"S") @@ -677,6 +694,12 @@ fn chain_forbidden(a: &Dict) -> Option { match d.get(b"Next") { Some(Object::Dict(next)) => stack.push(next), Some(Object::Array(list)) => stack.extend(list.iter().filter_map(Object::as_dict)), + Some(Object::Ref(r)) if seen.insert(*r) => { + if let Object::Array(list) = &*doc.get(*r) { + // More than the chain limit can't be read anyway. + held.extend(list.iter().filter_map(Object::as_dict).take(MAX_ACTION_CHAIN).cloned()); + } + } _ => {} } } @@ -686,7 +709,7 @@ fn chain_forbidden(a: &Dict) -> Option { /// Whether `o` is (or refers to) a forbidden action or one chaining to it, or to nothing. fn forbidden(doc: &Document, o: &Object) -> bool { match &*doc.resolve(o) { - Object::Dict(a) => chain_forbidden(a).is_some(), + Object::Dict(a) => chain_forbidden(doc, a).is_some(), Object::Null => true, _ => false, } @@ -836,11 +859,11 @@ pub fn convert(doc: &mut Document, level: Level) -> Result>")], ""), build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("5 0 R"), &goto], ""), build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link(&format!("<< /S /GoTo /D [3 0 R /Fit] /Next {js} >>"))], ""), + // `/Next` refers to an array that holds the action directly. + build(&["<< /Type /Catalog /Pages 2 0 R >>", pages, page, &link("<< /S /GoTo /D [3 0 R /Fit] /Next 5 0 R >>"), &format!("[{js}]")], ""), ]; for (i, mut doc) in cases.into_iter().enumerate() { let found = messages(&verify(&doc, Level::A2b)).contains(&"A JavaScript action isn't allowed"); From 35b5abd5b3c73d47c2fac71277d659d185001a2a Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 21:42:02 +0000 Subject: [PATCH 18/18] test(engine): show the truncated-page picture exports within a sufficient budget The fixture's image stream held six bytes under /Length 4, and the test only checked that the picture was missing at a zero budget, which a picture that could not be prepared also passes. The stream is four bytes, and the picture is first shown to export at a 1 MiB budget. --- crates/engine/src/tests.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3df5279ab..afe6fd06c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -7293,7 +7293,7 @@ fn a_page_truncated_for_forms_and_pictures_is_listed_once() { "<< /Type /Catalog /Pages 2 0 R >>".into(), "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 200] >>".into(), String::new(), - "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /Length 4 >>\nstream\n\0\x40\u{80}\u{ff}\nendstream".into(), + "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /Length 4 >>\nstream\n\0\x20\x40\x7f\nendstream".into(), "<< /Length 34 >>\nstream\nq 10 0 0 10 0 0 cm /Im Do Q /D1 Do\nendstream".into(), ]; // Forms D1..D{depth}, objects 6 onwards, each drawing the next. @@ -7323,6 +7323,8 @@ fn a_page_truncated_for_forms_and_pictures_is_listed_once() { let id = s.open("both.pdf", None, Arc::new(bytes), None).unwrap(); let doc = s.get(id).unwrap(); assert_eq!(doc.export_pages_checked(false, 0).1, [0], "the forms alone truncate the page"); + let (exported, _) = doc.export_pages_checked(true, 1 << 20); + assert_eq!(exported[0].images.len(), 1, "the picture exports within a sufficient budget"); let (pages, truncated) = doc.export_pages_checked(true, 0); assert!(pages[0].images.is_empty(), "the picture is past the budget"); assert_eq!(truncated, [0]);