From d3a01307ea4a694307247dd6f364f93d11711da9 Mon Sep 17 00:00:00 2001 From: Taha-Mahmoodi <85902429+Taha-Mahmoodi@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:35:14 +0430 Subject: [PATCH] PDF import: a string entered from many dictionaries in skipped text is read once Review of #3924: in a flat area of ` <<(` (CMYK or RGBA 32, 60, 60, 40) in an uncompressed image, each `<<` after white space was read as a dictionary, and each read its string, which never ends, to 64 KiB past it: about 16 K bytes a pixel, so a few hundred bytes of it spent the budget and the file was refused. Whether a byte in a literal string is escaped doesn't depend on where reading began (the run of `\` before it lies after any `(` before it), so the string from any `(` ends after the first `)` that takes the file-wide count of `(` less `)` below the count at that `(`. Those ends are found for the whole file in three linear passes the first time a string read in skipped text runs past 64 bytes: forward for the final count, back for the `(` whose string never ends, forward pairing the rest. Ends further than 64 bytes are kept (at most one for every 16 bytes of the file; a file built to nest past that is refused); nearer ones are read again. So entering a string costs at most 64 bytes, charged as one. A comment's end of line is kept for every comment read, not only the last. --- crates/pdf/src/streams.rs | 189 +++++++++++++++++++++++++++++++++----- docs/gaps.md | 2 +- 2 files changed, 166 insertions(+), 25 deletions(-) diff --git a/crates/pdf/src/streams.rs b/crates/pdf/src/streams.rs index c19b99a74..7da9ac94c 100644 --- a/crates/pdf/src/streams.rs +++ b/crates/pdf/src/streams.rs @@ -4,6 +4,7 @@ //! that large whatever the stream holds (#1701). A file with such a stream is refused instead. use std::borrow::Cow; +use std::collections::BTreeMap; use std::io::Read; use craft_pdf::syntax::content::TypedIter; @@ -279,7 +280,14 @@ fn unsafe_data(stream: &Stream<'_>, inside: bool) -> bool { /// of its own, within a budget for the whole file: a file that spends it is refused. Linear in /// the file (#3586), a run of `<` included (#3924 review). pub(crate) fn unsafe_file_streams(pdf: &[u8]) -> bool { - let mut scan = FileScan { pdf, budget: pdf.len().saturating_mul(4).saturating_add(1 << 20), endstream: None, line: None, read_from: Vec::new() }; + let mut scan = FileScan { + pdf, + budget: pdf.len().saturating_mul(4).saturating_add(1 << 20), + endstream: None, + lines: BTreeMap::new(), + strings: None, + read_from: Vec::new(), + }; scan.read().is_err() } @@ -324,6 +332,85 @@ fn string_end(pdf: &[u8], at: usize, limit: usize) -> Result { Err(i.min(limit)) } +/// How far past its `(` a string read in skipped text may end to be read again each time it is +/// entered (bytes); one ending further is looked up in [`StringEnds`]. +const NEAR_STRING: usize = 64; + +/// Is the byte at `at` escaped, a `\` before it that isn't escaped itself? Reading a string from +/// any `(` before it sees the same, as the run of `\` before it lies after that `(`. +fn escaped(pdf: &[u8], at: usize) -> bool { + pdf.get(..at).unwrap_or_default().iter().rev().take_while(|b| **b == b'\\').count() % 2 == 1 +} + +/// Where the literal strings read from the file's `(` end, for those ending more than +/// [`NEAR_STRING`] bytes on (#3924 review). As whether a byte is escaped doesn't depend on where +/// reading started ([`escaped`]), the string from any `(` ends after the first `)` that takes the +/// count of unescaped `(` less unescaped `)` from the file's start below the count with that `(` +/// (counted even when escaped). So all are found in three passes over the file: forward for the +/// count at its end, back for the `(` whose string never ends (the count never falls below theirs +/// again), and forward pairing the others with a stack. +struct StringEnds { + /// Each `(` whose string ends more than [`NEAR_STRING`] bytes on, and where, in file order. + far: Vec<(usize, usize)>, +} + +impl StringEnds { + /// The ends in `pdf`; `None` when more than one in 16 bytes of it are kept or pending (only a + /// file built for it nests strings so deep). + fn of(pdf: &[u8]) -> Option { + let most = pdf.len() / 16 + 1024; + // What the byte at `i` adds to the count: an escaped `(` adds nothing but is one too. + let step = |i: usize| match pdf.get(i) { + Some(b'(') => Some(i64::from(!escaped(pdf, i))), + Some(b')') => (!escaped(pdf, i)).then_some(-1), + _ => None, + }; + let total: i64 = (0..pdf.len()).filter_map(step).sum(); + // The `(` whose string never ends: none after it takes the count below its own. + let mut endless = vec![0u64; pdf.len() / 64 + 1]; + let (mut count, mut lowest) = (total, i64::MAX); + for i in (0..pdf.len()).rev() { + let Some(d) = step(i) else { continue }; + if pdf.get(i) == Some(&b'(') + && count <= lowest + && let Some(word) = endless.get_mut(i / 64) + { + *word |= 1 << (i % 64); + } + lowest = lowest.min(count); + count -= d; + } + let (mut far, mut open, mut count) = (vec![], Vec::<(usize, i64)>::new(), 0); + for i in 0..pdf.len() { + let Some(d) = step(i) else { continue }; + count += d; + if pdf.get(i) == Some(&b'(') { + if endless.get(i / 64).is_some_and(|word| word >> (i % 64) & 1 == 0) { + open.push((i, count)); + } + } else { + while let Some(&(at, _)) = open.last().filter(|(_, level)| *level > count) { + open.pop(); + if i + 1 - at > NEAR_STRING { + far.push((at, i + 1)); + } + } + } + if far.len() + open.len() > most { + return None; + } + } + far.sort_unstable(); + Some(StringEnds { far }) + } + + /// Where the string from the `(` at `at` ends, if it ends more than [`NEAR_STRING`] bytes on; + /// `None` for one that doesn't end. + fn far(&self, at: usize) -> Option { + self.far.binary_search_by_key(&at, |(open, _)| *open).ok().and_then(|n| self.far.get(n)).map(|(_, end)| *end) + } +} + /// Past the hex string opening at `at` (a `<`): hex digits and white space, then `>`. `Err` with /// how far it got for one the reader fails on. fn hex_end(pdf: &[u8], at: usize, limit: usize) -> Result { @@ -356,8 +443,11 @@ struct FileScan<'a> { budget: usize, /// Where an `endstream` was last looked for, and the first one found there (`None`: none). endstream: Option<(usize, Option)>, - /// Where a comment's end of line was last looked for, and where it is. - line: Option<(usize, usize)>, + /// The comments read in skipped text: each one's end of line, and where reading it began. + lines: BTreeMap, + /// The ends of strings read in skipped text far from their `(` ([`StringEnds`]), found once + /// the first such string is read. + strings: Option, /// The `<<` a dictionary in skipped text was read from, a bit a byte of the file (empty /// until one is). read_from: Vec, @@ -446,7 +536,9 @@ impl FileScan<'_> { let mut opens = std::collections::VecDeque::new(); let (mut depth, mut limit, mut i, mut work) = (0usize, at.saturating_add(MAX_SKIPPED_DICT), at, 0usize); while let Some(&b) = pdf.get(i).filter(|_| i < limit) { - let next = match pdf.get(i..i + 2) { + // Where to read on (`Err`: the dictionary ends there, read or unreadable), and the + // bytes read to get there. + let (next, read) = match pdf.get(i..i + 2) { Some(b"<<") => { self.mark(i); limit = limit.max(i.saturating_add(MAX_SKIPPED_DICT)); @@ -455,30 +547,29 @@ impl FileScan<'_> { } opens.push_back(i); depth += 1; - Ok(i + 2) + (Ok(i + 2), 2) } Some(b">>") => { depth = depth.saturating_sub(1); if let Some(open) = opens.pop_back() { self.closed(open, i + 2)?; } - if depth == 0 { Err(i + 2) } else { Ok(i + 2) } + (if depth == 0 { Err(i + 2) } else { Ok(i + 2) }, 2) } _ => match b { - b'(' => string_end(pdf, i, limit), - b'<' => hex_end(pdf, i, limit), - b'%' => { - i = self.line_end(i)?; - continue; + // Strings and comments are charged as they are first read. + b'(' => (self.string_end(i)?.ok_or(pdf.len()), 0), + b'%' => (Ok(self.line_end(i)?), 0), + b'<' => { + let end = hex_end(pdf, i, limit); + let (Ok(n) | Err(n)) = end; + (end, n.saturating_sub(i)) } - _ => Ok(i + 1), + _ => (Ok(i + 1), 1), }, }; - let (Ok(n) | Err(n)) = next; - work += n.saturating_sub(i); - if next.is_err() { - break; - } + work += read; + let Ok(n) = next else { break }; i = n; } self.charge(work) @@ -501,21 +592,44 @@ impl FileScan<'_> { Ok(()) } - /// The end of the comment at `at` ([`comment_end`]), the last one found kept: comments in - /// skipped text are read again from many places. + /// The end of the comment at `at` ([`comment_end`]). Comments in skipped text are read from + /// many places, so each one's end is kept: a byte is read as comment once. fn line_end(&mut self, at: usize) -> Result { - if let Some((from, end)) = self.line + // The first comment kept that ends at or after `at`. + let kept = self.lines.range(at..).next().map(|(end, from)| (*end, *from)); + if let Some((end, from)) = kept && from <= at - && at <= end { return Ok(end); } - let end = comment_end(self.pdf, at); - self.charge(end - at)?; - self.line = Some((at, end)); + // Read up to the next end of line, or to where that comment was read from. + let upto = kept.map_or(self.pdf.len(), |(_, from)| from); + let rest = self.pdf.get(at..upto).unwrap_or_default(); + let end = match rest.iter().position(|b| matches!(b, b'\n' | b'\r')) { + Some(n) => at + n, + None => kept.map_or(self.pdf.len(), |(end, _)| end), + }; + self.charge(rest.len().min(end - at))?; + self.lines.insert(end, at); Ok(end) } + /// Past the literal string at `at` (a `(`), as the reader reads it; `None` for one that + /// doesn't end. A string ending within [`NEAR_STRING`] bytes of its `(` is read again each + /// time a reading enters it; one ending further is looked up in [`StringEnds`], found for the + /// whole file in three passes (linear, so not charged) the first time one is needed. So each + /// time a string is entered costs at most [`NEAR_STRING`] bytes, charged as one. + fn string_end(&mut self, at: usize) -> Result, Unsafe> { + self.charge(1)?; + if let Ok(end) = string_end(self.pdf, at, at.saturating_add(NEAR_STRING)) { + return Ok(Some(end)); + } + if self.strings.is_none() { + self.strings = Some(StringEnds::of(self.pdf).ok_or(Unsafe)?); + } + Ok(self.strings.as_ref().and_then(|s| s.far(at))) + } + fn mark(&mut self, at: usize) { if self.read_from.is_empty() { self.read_from = vec![0; self.pdf.len() / 64 + 1]; @@ -770,6 +884,13 @@ mod tests { // reader starting at the run's first `<<` reads it inside the others). image(&[&run[..], b"\n", bad].concat()), image(&[&run[..], b" /Type /XRef /DecodeParms 9 0 R >> stream\nx\nendstream"].concat()), + // Strings in it that end far from their `(` (looked up, not read again): one, one + // holding others, one from an escaped `(`, and the same after a flat area of + // ` <<(` whose strings never end. + image(&[&b"<< /Type /XRef /Pad ("[..], &[b'x'; 200], b") /DecodeParms 9 0 R >> stream\nx\nendstream"].concat()), + image(&[&b"<< /Type /XRef /Pad (a (b) "[..], &[b'x'; 200], b" (c (d)) \\) ) /DecodeParms 9 0 R >> stream\nx\nendstream"].concat()), + image(&[&b"<< /Type /XRef /Pad \\("[..], &[b'x'; 200], b") /DecodeParms 9 0 R >> stream\nx\nendstream"].concat()), + image(&[&b" <<(".repeat(1 << 14)[..], b"\n", bad].concat()), ]; let unread: Vec<_> = cases.iter().filter(|bad| !unsafe_file_streams(bad)).map(|bad| String::from_utf8_lossy(&bad[..bad.len().min(80)])).collect(); @@ -795,6 +916,26 @@ mod tests { } } + #[test] + fn a_flat_area_that_opens_a_string_after_each_dictionary_opens() { + // #3924 review: in a flat area of ` <<(` (CMYK or RGBA 32, 60, 60, 40) each `<<` was read + // as a dictionary, and each read its never-ending string to 64 KiB past it: the budget + // was spent and the file refused. A string ending far from its `(` is now read once. + let noise: Vec = (0u32..1 << 18).map(|i| (i.wrapping_mul(2_654_435_761) >> 24) as u8).collect(); + for (what, data) in [ + (" <<(", b" <<(".repeat(1 << 18)), + ("\\0<<(", b"\0<<(".repeat(1 << 18)), + ("( <<", b"( <<".repeat(1 << 18)), + (" <<( then noise", [&b" <<(".repeat(1 << 16)[..], &noise].concat()), + ] { + let pdf = [format!("1 0 obj << /Length {} >> stream\n", data.len()).as_bytes(), &data[..], b"\nendstream endobj\n"].concat(); + let t = std::time::Instant::now(); + assert!(!unsafe_file_streams(&pdf), "{what}"); + let took = t.elapsed(); + assert!(took < std::time::Duration::from_secs(5), "{what} took {took:?}"); + } + } + #[test] fn strings_and_comments_dense_with_dictionaries_are_scanned_in_bounded_time() { // Each `<<` in a string or comment the reader could start at is read as a dictionary of diff --git a/docs/gaps.md b/docs/gaps.md index e2e64bad0..eec7dc4f6 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -613,7 +613,7 @@ trap, and the defaults (0.25 pt, 100%, 40%) are VectorCraft's own, the pages giv |---|---|---| | 2026-10-24 | minor | G14 shrinks: pen pressure and the pen's eraser end on macOS, read from AppKit's tablet events (#852; not yet tried on a Mac); estimate 8–16 h → 4–10 h. | | 2026-10-24 | minor | G11: Shape Builder pieces: the planar map contracts the stubs the sweep left between two cuts of a line's passes a rounding error apart (bounded by each cluster's extent), so dense hatches at exact binary steps no longer add a speck per crossing; estimate unchanged, #937 open. | -| 2026-10-24 | minor | G10: the PDF pre-open stream scan reads the file as the reader does (literal and hex strings, comments, stream data skipped), so a `stream` or `>>` in a string or comment, or a comment before `stream`, no longer hides a cross-reference or object stream from it (#3919 review); a `<<` in skipped text that the reader could start a dictionary at (after white space, `>>` or `obj`) is read as one too, each about once, so a run of `<` in an uncompressed image (grey 60) no longer spends the budget and refuses the file (#3924 review). No entry closed, estimates unchanged. | +| 2026-10-24 | minor | G10: the PDF pre-open stream scan reads the file as the reader does (literal and hex strings, comments, stream data skipped), so a `stream` or `>>` in a string or comment, or a comment before `stream`, no longer hides a cross-reference or object stream from it (#3919 review); a `<<` in skipped text that the reader could start a dictionary at (after white space, `>>` or `obj`) is read as one too, each about once, and a string or comment it enters is read once however many such readings enter it, so a run of `<` (grey 60) or a flat area of ` <<(` (CMYK 32, 60, 60, 40) in an uncompressed image no longer spends the budget and refuses the file (#3924 review). No entry closed, estimates unchanged. | | 2026-10-24 | minor | G4: images in OpenType SVG glyph documents draw, export and outline with the glyph; colour fonts are done but for CPAL palette variables; estimate unchanged. | | 2026-10-24 | minor | G4: bitmap glyphs (emoji) go with type that effects reshape, into Expand Appearance, envelope Expand and EMF/WMF export, as images that hold their own pixels until a document takes them in; estimate unchanged. | | 2026-10-24 | minor | G10: Fix #3586: the PDF pre-open scan reads a file dense with `stream` keywords in linear time; each keyword walked back up to 64 KiB through every dictionary before it, so 1 MiB of `<> >>stream` took minutes. No entry closed, estimates unchanged. |