From 15f6fa7d4c37a81c7d2cadfc1eed0d22d8667103 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 11:55:59 -0400 Subject: [PATCH 1/2] Never crash: forms scripts can't overflow the stack or the date arithmetic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two crashes that a PDF can trigger, each with a regression test that failed before this change: - Deeply nested arguments in a field's format script (`[[[[…` or `new Array(new Array(…`) and deeply nested simplified field notation (`((((…` or `----…1`) recursed until the stack overflowed, which aborts the whole app. Nesting is now capped at 64 levels; deeper input is rejected like any other unparseable script. - A date with a huge year (`1/1/2147483647`) overflowed `weekday`, and a `DateTime` with month 0 indexed before the month names. The weekday math is now done in i64, years beyond i32 don't parse, and month and day names are looked up with `get`. Also removes the crate's remaining `expect`/`unreachable!` calls and denies the panic lints. Co-Authored-By: Claude Opus 5.5 --- crates/forms/src/af.rs | 130 +++++++++++++++++++++++++++++-------- crates/forms/src/author.rs | 5 +- crates/forms/src/lib.rs | 10 +-- 3 files changed, 114 insertions(+), 31 deletions(-) diff --git a/crates/forms/src/af.rs b/crates/forms/src/af.rs index bcaaaacb4..db757fb8f 100644 --- a/crates/forms/src/af.rs +++ b/crates/forms/src/af.rs @@ -148,9 +148,14 @@ impl Arg { } } +/// How deeply script arguments and field notation may nest; deeper input is rejected rather +/// than recursed into (a hostile file could otherwise overflow the stack). +const MAX_NESTING: usize = 64; + struct Parser<'a> { s: &'a [u8], i: usize, + depth: usize, } impl Parser<'_> { @@ -191,7 +196,7 @@ impl Parser<'_> { b'"' | b'\'' => self.string().map(Arg::Str), b'[' => { self.i += 1; - self.list(b']').map(Arg::List) + self.nested(b']').map(Arg::List) } _ if self.s[self.i..].starts_with(b"new Array") => { self.i += b"new Array".len(); @@ -200,7 +205,7 @@ impl Parser<'_> { return None; } self.i += 1; - self.list(b')').map(Arg::List) + self.nested(b')').map(Arg::List) } _ if self.s[self.i..].starts_with(b"true") => { self.i += 4; @@ -220,6 +225,17 @@ impl Parser<'_> { } } + /// A list inside another, at most [`MAX_NESTING`] deep. + fn nested(&mut self, close: u8) -> Option> { + if self.depth >= MAX_NESTING { + return None; + } + self.depth += 1; + let out = self.list(close); + self.depth -= 1; + out + } + fn list(&mut self, close: u8) -> Option> { let mut out = Vec::new(); loop { @@ -249,7 +265,7 @@ fn call(js: &str, name: &str) -> Option> { if js[..at].chars().last().is_some_and(|c| c.is_alphanumeric() || c == '_') { return None; } - let mut p = Parser { s: js.as_bytes(), i: at + name.len() + 1 }; + let mut p = Parser { s: js.as_bytes(), i: at + name.len() + 1, depth: 0 }; p.list(b')') } @@ -478,10 +494,11 @@ fn days_in(y: i32, m: u32) -> u32 { } fn weekday(y: i32, m: u32, d: u32) -> usize { - // Sakamoto's method. - let t = [0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4]; - let y = if m < 3 { y - 1 } else { y }; - ((y + y / 4 - y / 100 + y / 400 + t[(m - 1) as usize] + d as i32).rem_euclid(7)) as usize + // Sakamoto's method, in i64 so no year or day can overflow it. + const T: [i64; 12] = [0, 3, 2, 5, 0, 3, 5, 1, 4, 6, 2, 4]; + let y = i64::from(y) - i64::from(m < 3); + let t = T.get((m as usize).wrapping_sub(1)).copied().unwrap_or(0); + (y + y / 4 - y / 100 + y / 400 + t + i64::from(d)).rem_euclid(7) as usize } /// The order of the year/month/day fields in a format, as letters. @@ -552,12 +569,16 @@ pub fn parse_date(text: &str, fmt: &str) -> Option { let mut have_y = false; for k in order(fmt) { match k { - 'm' if month_name.is_some() => dt.m = month_name.expect("checked"), - 'm' => dt.m = it.next()?, + 'm' => { + dt.m = match month_name { + Some(m) => m, + None => it.next()?, + } + } 'd' => dt.d = it.next()?, 'y' => { - let y = it.next()?; - dt.y = if y < 100 { if y < 50 { 2000 + y as i32 } else { 1900 + y as i32 } } else { y as i32 }; + let y = i32::try_from(it.next()?).ok()?; + dt.y = if y < 100 { if y < 50 { 2000 + y } else { 1900 + y } } else { y }; have_y = true; } 'H' => dt.hh = it.next().unwrap_or(0), @@ -607,16 +628,23 @@ pub fn format_date(dt: DateTime, fmt: &str) -> String { ('y', _) => { let _ = write!(out, "{:02}", dt.y.rem_euclid(100)); } - ('m', 4..) => out.push_str(MONTHS[(dt.m - 1) as usize]), - ('m', 3) => out.push_str(&MONTHS[(dt.m - 1) as usize][..3]), + ('m', 3..) => match MONTHS.get((dt.m as usize).wrapping_sub(1)) { + Some(name) if n >= 4 => out.push_str(name), + Some(name) => out.push_str(name.get(..3).unwrap_or(name)), + None => { + let _ = write!(out, "{}", dt.m); + } + }, ('m', 2) => { let _ = write!(out, "{:02}", dt.m); } ('m', _) => { let _ = write!(out, "{}", dt.m); } - ('d', 4..) => out.push_str(DAYS[weekday(dt.y, dt.m, dt.d)]), - ('d', 3) => out.push_str(&DAYS[weekday(dt.y, dt.m, dt.d)][..3]), + ('d', 3..) => { + let name = DAYS.get(weekday(dt.y, dt.m, dt.d)).copied().unwrap_or_default(); + out.push_str(if n >= 4 { name } else { name.get(..3).unwrap_or(name) }); + } ('d', 2) => { let _ = write!(out, "{:02}", dt.d); } @@ -813,26 +841,29 @@ fn eval_notation(src: &str, value_of: &dyn Fn(&str) -> f64) -> Option { toks.push(T::N(value_of(&name))); } } - // Recursive descent. - fn expr(t: &[T], i: &mut usize) -> Option { - let mut v = term(t, i)?; + // Recursive descent, `d` levels deep (at most MAX_NESTING). + fn expr(t: &[T], i: &mut usize, d: usize) -> Option { + let mut v = term(t, i, d)?; while let Some(T::Op(o @ ('+' | '-'))) = t.get(*i) { *i += 1; - let r = term(t, i)?; + let r = term(t, i, d)?; v = if *o == '+' { v + r } else { v - r }; } Some(v) } - fn term(t: &[T], i: &mut usize) -> Option { - let mut v = factor(t, i)?; + fn term(t: &[T], i: &mut usize, d: usize) -> Option { + let mut v = factor(t, i, d)?; while let Some(T::Op(o @ ('*' | '/'))) = t.get(*i) { *i += 1; - let r = factor(t, i)?; + let r = factor(t, i, d)?; v = if *o == '*' { v * r } else { v / r }; } Some(v) } - fn factor(t: &[T], i: &mut usize) -> Option { + fn factor(t: &[T], i: &mut usize, d: usize) -> Option { + if d > MAX_NESTING { + return None; + } match t.get(*i)? { T::N(n) => { *i += 1; @@ -840,11 +871,11 @@ fn eval_notation(src: &str, value_of: &dyn Fn(&str) -> f64) -> Option { } T::Op('-') => { *i += 1; - factor(t, i).map(|v| -v) + factor(t, i, d + 1).map(|v| -v) } T::Op('(') => { *i += 1; - let v = expr(t, i)?; + let v = expr(t, i, d + 1)?; (t.get(*i) == Some(&T::Op(')'))).then(|| *i += 1)?; Some(v) } @@ -852,7 +883,7 @@ fn eval_notation(src: &str, value_of: &dyn Fn(&str) -> f64) -> Option { } } let mut i = 0; - let v = expr(&toks, &mut i)?; + let v = expr(&toks, &mut i, 0)?; (i == toks.len() && v.is_finite()).then_some(v) } @@ -1068,3 +1099,50 @@ mod button_tests { assert!(matches!(button_script("var x = 1; doStuff(x);"), ButtonAction::Script(_))); } } + +#[cfg(test)] +mod never_crash_tests { + use super::*; + + /// A field value or typed text with an absurd year used to overflow the weekday arithmetic. + #[test] + fn huge_years_format_without_panicking() { + let f = Format::Date("dddd, mmmm d, yyyy".into()); + for v in ["1/1/2147483647", "1/1/4294967295", "12/31/3000000000"] { + let _ = format_value(&f, v); + let _ = keystroke(&f, "date", v); + } + } + + /// Deeply nested arrays in a field's format script used to recurse until the stack overflowed. + #[test] + fn deeply_nested_script_arguments_are_rejected() { + let n = 200_000; + let js = format!("AFNumber_Format({}0{});", "[".repeat(n), "]".repeat(n)); + assert_eq!(parse_format(&js), None); + let js = format!("AFNumber_Format({}0{});", "new Array(".repeat(n), ")".repeat(n)); + assert_eq!(parse_format(&js), None); + } + + /// Deeply nested simplified field notation (from a field's calculate script) likewise. + #[test] + fn deeply_nested_notation_is_rejected() { + let n = 200_000; + let deep = format!("{}1{}", "(".repeat(n), ")".repeat(n)); + assert_eq!(eval_notation(&deep, &|_| 0.0), None); + let negated = format!("{}1", "-".repeat(n)); + assert_eq!(eval_notation(&negated, &|_| 0.0), None); + assert_eq!(eval_notation("((1 + 2)) * -(3)", &|_| 0.0), Some(-9.0)); + } + + /// `DateTime` is public: an out-of-range month or day must not index past the name tables. + #[test] + fn out_of_range_dates_format_without_panicking() { + for m in [0, 13, u32::MAX] { + let dt = DateTime { y: 2024, m, d: 1, hh: 0, mm: 0, ss: 0 }; + let _ = format_date(dt, "dddd mmmm mmm d yyyy"); + } + let dt = DateTime { y: i32::MAX, m: 1, d: u32::MAX, hh: 0, mm: 0, ss: 0 }; + let _ = format_date(dt, "dddd ddd"); + } +} diff --git a/crates/forms/src/author.rs b/crates/forms/src/author.rs index 0179e2cb5..18c9b4fde 100644 --- a/crates/forms/src/author.rs +++ b/crates/forms/src/author.rs @@ -293,7 +293,10 @@ fn add_to_fields(doc: &mut Document, af: ObjRef, field: ObjRef) -> Result<(), Fo /// The first free "N" name. fn free_name(existing: &[Field], base: &str) -> String { - (1..).map(|n| format!("{base}{n}")).find(|c| !existing.iter().any(|f| f.name == *c || f.name.starts_with(&format!("{c}.")))).expect("unbounded") + (1..=u64::MAX) + .map(|n| format!("{base}{n}")) + .find(|c| !existing.iter().any(|f| f.name == *c || f.name.starts_with(&format!("{c}.")))) + .unwrap_or_else(|| base.to_string()) } fn widget_dict(page: ObjRef, rect: [f64; 4]) -> Dict { diff --git a/crates/forms/src/lib.rs b/crates/forms/src/lib.rs index 3bd15dd9f..601e940e0 100644 --- a/crates/forms/src/lib.rs +++ b/crates/forms/src/lib.rs @@ -11,6 +11,8 @@ //! Not yet: JavaScript actions (format, keystroke, validate, calculate — M6.4/M6.5) and rich text //! values (`/RV`, which is removed when a value is set so it can't contradict `/V`). +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + use printcraft_cos::{Dict, Document, ObjRef, Object, PdfString}; mod actions; @@ -438,8 +440,8 @@ fn rank_tabs(doc: &Document, pages: &[ObjRef], annot_index: &std::collections::H (it.3[0] - first.3[0]).abs() < ((first.3[2] - first.3[0]) / 2.0).max(2.0) } }); - if fits { - groups.last_mut().expect("checked").push(it); + if fits && let Some(g) = groups.last_mut() { + g.push(it); } else { groups.push(vec![it]); } @@ -747,7 +749,7 @@ fn write_value(doc: &mut Document, f: &Field, value: &FieldValue, scripts: &mut FieldValue::Choice(v) => v.clone(), FieldValue::Text(t) if t.is_empty() => Vec::new(), FieldValue::Text(t) => vec![t.clone()], - _ => unreachable!("matched above"), + other => return invalid(format!("{:?} can't take {other:?}", f.name)), }; if vals.len() > 1 && !(f.kind == FieldKind::List && f.has(flags::MULTI_SELECT)) { return invalid(format!("{:?} takes a single value", f.name)); @@ -878,7 +880,7 @@ pub fn move_in_tab_order(doc: &mut Document, name: &str, earlier: bool) -> Resul all.iter().filter_map(|g| g.widgets.iter().filter(|w| w.page == Some(page)).map(|w| w.tab).min().map(|t| (t, g.name.as_str()))).collect(); order.sort(); let names: Vec<&str> = order.iter().map(|(_, n)| *n).collect(); - let i = names.iter().position(|n| *n == name).expect("on this page"); + let Some(i) = names.iter().position(|n| *n == name) else { return invalid(format!("{name} is not on a page")) }; let j = if earlier { i.checked_sub(1) } else { Some(i + 1).filter(|j| *j < names.len()) }; let Some(j) = j else { return Ok(()) }; let mut names = names.into_iter().map(str::to_string).collect::>(); From b834c6c063c85f4371a4f54169922738de6bc6dc Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 13:34:59 -0400 Subject: [PATCH 2/2] Root clippy.toml: tests may unwrap, expect, panic and index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same file as the steering PR (#11), so this PR stands on its own. The crate-level `#![deny(clippy::unwrap_used, …)]` applies to production code only. Co-Authored-By: Claude Opus 5.5 --- clippy.toml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 clippy.toml diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 000000000..4b6c79f55 --- /dev/null +++ b/clippy.toml @@ -0,0 +1,4 @@ +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true +allow-indexing-slicing-in-tests = true