From 4f85efb2b97779d4509bcb572851efde52f1f987 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Thu, 22 Oct 2026 21:49:46 -0400 Subject: [PATCH] security: bound print dialog name resolution, PPD labels, lpstat merge and inherited geometry - craft-ui shown_names resolves label collisions with hash-map counts and a worklist, so a crafted cascade is linear instead of cubic (CodeRabbit CWE-407, ui/src/printing.rs:32) - parse_ppd caps group, option and choice labels at 256 characters before each option copies its group (CodeRabbit CWE-400, print/src/lib.rs:232) - printers_parsed tracks seen destinations in a HashSet (CodeRabbit CWE-407, lib.rs:417) - sheets::Reader::rect checks an inline array's length before reading it and borrows direct objects instead of deep-cloning the inherited MediaBox/CropBox/Rotate for every page (CodeRabbit CWE-400, print/src/sheets.rs:774) --- crates/print/src/lib.rs | 16 +++++-- crates/print/src/sheets.rs | 26 ++++++++---- crates/print/src/tests.rs | 23 +++++++++++ crates/print/src/tests/pdf_bounds.rs | 29 +++++++++++++ crates/ui/src/printing.rs | 40 ++++++++++++------ crates/ui/src/printing/tests.rs | 62 ++++++++++++++++++++++++++++ 6 files changed, 173 insertions(+), 23 deletions(-) diff --git a/crates/print/src/lib.rs b/crates/print/src/lib.rs index 25e03ad5..3c26974a 100644 --- a/crates/print/src/lib.rs +++ b/crates/print/src/lib.rs @@ -195,6 +195,13 @@ pub const HANDLED_OPTIONS: &[&str] = &["PageSize", "PageRegion", "Duplex", "Coll /// Caps on what a PPD may make the dialog hold (PPDs are text files from the printer vendor). const MAX_OPTIONS: usize = 400; const MAX_CHOICES: usize = 400; +/// Longest group, option or choice label kept, in characters; every option copies its group's. +const MAX_PPD_LABEL: usize = 256; + +/// `label`, cut to [`MAX_PPD_LABEL`] characters. +fn ppd_label(label: &str) -> String { + label.chars().take(MAX_PPD_LABEL).collect() +} /// A PPD keyword: printable ASCII without spaces, `/`, `:` or `=`. fn is_ppd_keyword(s: &str) -> bool { @@ -220,7 +227,7 @@ pub fn parse_ppd(ppd: &str) -> Vec { let line = line.trim_end_matches('\r'); if let Some(rest) = line.strip_prefix("*OpenGroup:") { let (k, l) = name_label(rest); - (group_key, group) = (k.to_string(), l.to_string()); + (group_key, group) = (k.to_string(), ppd_label(l)); } else if line.starts_with("*CloseGroup") { (group_key, group) = (String::new(), String::new()); } else if let Some(rest) = line.strip_prefix("*OpenUI") { @@ -228,7 +235,7 @@ pub fn parse_ppd(ppd: &str) -> Vec { let (key, label) = name_label(head.trim().trim_start_matches('*')); open = is_ppd_keyword(key).then(|| PrinterOption { key: key.to_string(), - label: label.to_string(), + label: ppd_label(label), group: group.clone(), choices: Vec::new(), default: String::new(), @@ -251,7 +258,7 @@ pub fn parse_ppd(ppd: &str) -> Vec { let head = rest.split_once(':').map_or(rest, |(h, _)| h); let (choice, label) = name_label(head); if is_ppd_keyword(choice) && o.choices.len() < MAX_CHOICES && o.choices.iter().all(|(c, _)| c != choice) { - o.choices.push((choice.to_string(), label.to_string())); + o.choices.push((choice.to_string(), ppd_label(label))); } } } @@ -413,8 +420,9 @@ pub fn printers_parsed(queues: &str, available: Option<&str>) -> Vec { let mut printers = parse_lpstat(queues); let default = default_destination(queues); if let Some(names) = available { + let mut seen: std::collections::HashSet = printers.iter().map(|p| p.name.clone()).collect(); for name in parse_lpstat_e(names) { - if !printers.iter().any(|p| p.name == name) { + if seen.insert(name.clone()) { printers.push(Printer { default: default == Some(name.as_str()), name, description: String::new(), two_sided: None }); } } diff --git a/crates/print/src/sheets.rs b/crates/print/src/sheets.rs index d3c989cd..bbb4c7c7 100644 --- a/crates/print/src/sheets.rs +++ b/crates/print/src/sheets.rs @@ -11,6 +11,7 @@ //! Every length, count and depth is bounded, and anything unreadable is an [`Error`], never a //! panic: the caller's PDF is input like any other. +use std::borrow::Cow; use std::collections::{HashMap, HashSet}; use std::sync::Arc; @@ -781,16 +782,27 @@ impl Reader<'_> { None } - fn rect(&mut self, obj: Option<&Obj>) -> Option<[f64; 4]> { - let obj = self.resolve(obj?)?; - let items = obj.array()?; - let mut v = [0.0; 4]; - for (slot, item) in v.iter_mut().zip(items) { - *slot = self.resolve(item)?.num()?; + /// Like [`resolve`](Self::resolve), but a direct object is borrowed, not copied. Inherited + /// page attributes are read once per page, so copying a large inline value would repeat that + /// work for every page outside the budget. + fn resolve_borrowed<'o>(&mut self, obj: &'o Obj) -> Option> { + match obj { + Obj::Ref(..) => self.resolve(obj).map(Cow::Owned), + direct => Some(Cow::Borrowed(direct)), } + } + + fn rect(&mut self, obj: Option<&Obj>) -> Option<[f64; 4]> { + let obj = self.resolve_borrowed(obj?)?; + let items = obj.array()?; + // The shape is checked before reading any element. if items.len() != 4 { return None; } + let mut v = [0.0; 4]; + for (slot, item) in v.iter_mut().zip(items) { + *slot = self.resolve_borrowed(item)?.num()?; + } let [x0, y0, x1, y1] = v; Some([x0.min(x1), y0.min(y1), x0.max(x1), y0.max(y1)]) } @@ -938,7 +950,7 @@ pub(crate) fn read(data: &[u8]) -> Result, Error> { .map(|c| [c[0].max(media[0]), c[1].max(media[1]), c[2].min(media[2]), c[3].min(media[3])]) .filter(|c| c[2] > c[0] && c[3] > c[1]) .unwrap_or(media); - let rotate = here.rotate.as_ref().and_then(|r| reader.resolve(r)).and_then(|r| r.int()).unwrap_or(0).rem_euclid(360); + let rotate = here.rotate.as_ref().and_then(|r| reader.resolve_borrowed(r).and_then(|r| r.int())).unwrap_or(0).rem_euclid(360); // Not a multiple of 90 is invalid; such a page is shown unrotated. let rotate = if rotate % 90 == 0 { u32::try_from(rotate).unwrap_or(0) } else { 0 }; // Windows renders viewer visibility, not annotation print visibility. Only a plain diff --git a/crates/print/src/tests.rs b/crates/print/src/tests.rs index c3675b8b..db03fb8a 100644 --- a/crates/print/src/tests.rs +++ b/crates/print/src/tests.rs @@ -256,6 +256,29 @@ fn printer_options_come_from_the_ppd() { assert!(spool::parse_ppd("*OpenUI *Broken\n*Broken A/B\n").is_empty(), "unclosed or malformed blocks are skipped"); } +#[test] +fn ppd_labels_are_capped_before_every_option_copies_its_group() { + let long = "G".repeat(100_000); + let mut ppd = format!("*OpenGroup: Big/{long}\n"); + for i in 0..300 { + ppd += &format!("*OpenUI *Opt{i}/{long}: PickOne\n*Opt{i} A/{long}: \"\"\n*CloseUI: *Opt{i}\n"); + } + let options = spool::parse_ppd(&ppd); + assert_eq!(options.len(), 300); + assert!(options.iter().all(|o| o.group.len() <= 256 && o.label.len() <= 256 && o.choices[0].1.len() <= 256)); + assert_eq!(options[0].group, "G".repeat(256)); +} + +#[test] +fn many_driverless_destinations_merge_in_linear_time() { + let names: String = (0..200_000).map(|i| format!("dest_{i}\n")).collect(); + let started = std::time::Instant::now(); + let printers = spool::printers_parsed("printer Office is idle.\n", Some(&format!("Office\n{names}dest_7\n"))); + assert!(started.elapsed() < std::time::Duration::from_secs(10), "{:?}", started.elapsed()); + assert_eq!(printers.len(), 200_001, "each destination once, queues first"); + assert_eq!(printers[0].name, "Office"); +} + #[test] fn printer_option_defaults_and_job_arguments() { let current = diff --git a/crates/print/src/tests/pdf_bounds.rs b/crates/print/src/tests/pdf_bounds.rs index a1729273..76b1409a 100644 --- a/crates/print/src/tests/pdf_bounds.rs +++ b/crates/print/src/tests/pdf_bounds.rs @@ -254,3 +254,32 @@ fn an_inflate_limit_stops_later_stream_and_object_work() { assert_eq!(reader.budget.nodes, 0); assert_eq!(reader.budget.tokens, 0); } + +#[test] +fn an_oversized_inherited_media_box_is_not_copied_for_every_page() { + // One inline MediaBox far longer than four numbers, inherited by many small pages. + let pages = 4_000; + let media = vec!["0"; 25_000].join(" "); + let kids: Vec = (0..pages).map(|i| format!("{} 0 R", i + 3)).collect(); + let mut objects = vec![ + "<< /Type /Catalog /Pages 2 0 R >>".to_string(), + format!("<< /Type /Pages /Kids [{}] /Count {pages} /MediaBox [{media}] >>", kids.join(" ")), + ]; + objects.extend((0..pages).map(|_| "<< /Type /Page /Parent 2 0 R >>".to_string())); + let mut pdf = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, object) in objects.iter().enumerate() { + offsets.push(pdf.len()); + pdf.extend_from_slice(format!("{} 0 obj\n{object}\nendobj\n", i + 1).as_bytes()); + } + let xref = pdf.len(); + pdf.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f\r\n", objects.len() + 1).as_bytes()); + for offset in offsets { + pdf.extend_from_slice(format!("{offset:010} 00000 n\r\n").as_bytes()); + } + pdf.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objects.len() + 1).as_bytes()); + let started = std::time::Instant::now(); + let sheets = read(&pdf).unwrap(); + assert!(started.elapsed() < std::time::Duration::from_secs(10), "{:?}", started.elapsed()); + assert_eq!(sheets.sheets.len(), pages); +} diff --git a/crates/ui/src/printing.rs b/crates/ui/src/printing.rs index 30d7ad4b..bdcf5527 100644 --- a/crates/ui/src/printing.rs +++ b/crates/ui/src/printing.rs @@ -1,6 +1,7 @@ //! Printer selection and asynchronous job feedback, independent of any spooler or document. //! Hosts provide workers; applications own PDF generation, commands and translated wording. +use std::collections::HashMap; use std::sync::mpsc::{Receiver, TryRecvError}; #[derive(Clone, Debug, Default, PartialEq, Eq)] @@ -19,26 +20,41 @@ pub struct Printer { /// printers show their names, which the spooler keeps distinct. pub fn shown_names(printers: &[Printer]) -> Vec { let base: Vec<&str> = printers.iter().map(|p| if p.label.trim().is_empty() { p.name.as_str() } else { p.label.as_str() }).collect(); + let mut base_count: HashMap<&str, usize> = HashMap::with_capacity(base.len()); + for text in &base { + *base_count.entry(text).or_default() += 1; + } let mut shown: Vec = printers .iter() .zip(&base) .map(|(printer, text)| { - let shared = base.iter().filter(|other| *other == text).count() > 1; + let shared = base_count.get(text).is_some_and(|n| *n > 1); if shared && *text != printer.name { format!("{text} ({})", printer.name) } else { (*text).to_owned() } }) .collect(); - // Each pass turns at least one shared text into its name, or stops: at most one per printer. - for _ in 0..printers.len() { - let shared: Vec = shown.iter().map(|text| shown.iter().filter(|other| *other == text).count() > 1).collect(); - let mut changed = false; - for ((text, printer), shared) in shown.iter_mut().zip(printers).zip(shared) { - if shared && *text != printer.name { - text.clone_from(&printer.name); - changed = true; - } + // Which printers show each text. A shared text turns every printer showing it into its name; + // a printer that shows its name never changes again, so each moves at most once and the + // whole cascade stays linear in the number of printers. + let mut by_text: HashMap> = HashMap::with_capacity(shown.len()); + for (i, text) in shown.iter().enumerate() { + by_text.entry(text.clone()).or_default().push(i); + } + let mut pending: Vec = by_text.iter().filter(|(_, ids)| ids.len() > 1).map(|(text, _)| text.clone()).collect(); + while let Some(text) = pending.pop() { + let Some(ids) = by_text.get_mut(&text) else { continue }; + if ids.len() < 2 { + continue; } - if !changed { - break; + let (stay, moving): (Vec, Vec) = ids.iter().partition(|&&i| printers.get(i).is_some_and(|p| p.name == text)); + *ids = stay; + for i in moving { + let (Some(printer), Some(slot)) = (printers.get(i), shown.get_mut(i)) else { continue }; + slot.clone_from(&printer.name); + let ids = by_text.entry(printer.name.clone()).or_default(); + ids.push(i); + if ids.len() == 2 { + pending.push(printer.name.clone()); + } } } shown diff --git a/crates/ui/src/printing/tests.rs b/crates/ui/src/printing/tests.rs index 47f3587a..caddd58a 100644 --- a/crates/ui/src/printing/tests.rs +++ b/crates/ui/src/printing/tests.rs @@ -8,6 +8,68 @@ fn labelled(name: &str, label: &str) -> Printer { Printer { name: name.into(), label: label.into(), default: false } } +/// The pass-by-pass resolution `shown_names` replaced, to check they agree. +fn shown_names_by_passes(printers: &[Printer]) -> Vec { + let base: Vec<&str> = printers.iter().map(|p| if p.label.trim().is_empty() { p.name.as_str() } else { p.label.as_str() }).collect(); + let mut shown: Vec = printers + .iter() + .zip(&base) + .map(|(printer, text)| { + let shared = base.iter().filter(|other| *other == text).count() > 1; + if shared && *text != printer.name { format!("{text} ({})", printer.name) } else { (*text).to_owned() } + }) + .collect(); + loop { + let shared: Vec = shown.iter().map(|text| shown.iter().filter(|other| *other == text).count() > 1).collect(); + let mut changed = false; + for ((text, printer), shared) in shown.iter_mut().zip(printers).zip(shared) { + if shared && *text != printer.name { + text.clone_from(&printer.name); + changed = true; + } + } + if !changed { + return shown; + } + } +} + +/// Each printer's label is the name the one before it falls back to, so every pass of the +/// old resolution freed exactly one more printer. +fn cascade(n: usize) -> Vec { + let mut printers = vec![labelled("n0", "x"), labelled("n1", "x"), labelled("n2", "x (n0)")]; + printers.extend((3..n).map(|i| labelled(&format!("n{i}"), &format!("n{}", i - 1)))); + printers +} + +#[test] +fn collisions_resolve_like_the_pass_by_pass_rule() { + let names = ["a", "b", "c", "Office", "Office_1", "x"]; + let labels = ["", "a", "b", "Office", "Office (Office_1)", "x (a)", "a (b)", "c"]; + let mut seed = 7u32; + for _ in 0..2000 { + let printers: Vec = (0..5) + .map(|_| { + seed = seed.wrapping_mul(1_103_515_245).wrapping_add(12_345); + labelled(names[(seed >> 8) as usize % names.len()], labels[(seed >> 16) as usize % labels.len()]) + }) + .collect(); + assert_eq!(shown_names(&printers), shown_names_by_passes(&printers), "{printers:?}"); + } + assert_eq!(shown_names(&cascade(40)), shown_names_by_passes(&cascade(40))); +} + +#[test] +fn a_long_collision_cascade_stays_fast() { + let printers = cascade(20_000); + let started = std::time::Instant::now(); + let shown = shown_names(&printers); + assert!(started.elapsed() < std::time::Duration::from_secs(5), "{:?}", started.elapsed()); + assert_eq!(shown[0], "n0"); + assert_eq!(shown[1], "x (n1)"); + assert!(shown[2..].iter().zip(&printers[2..]).all(|(s, p)| *s == p.name)); +} + #[test] fn the_dialog_shows_a_printers_label_and_keeps_its_queue_name() { // CUPS can't put dots in a queue name: the printer added as 192.0.2.15 is `_192_0_2_15`.