Merge PR #670 from NikitkaRa: Format: never save a .deckcraft that cannot be reopened over a NaN

Fix-up in the merge (review): reading older files that hold null numbers re-parses at most
256 MB in all and slices without indexing, so a hostile file can't hang the open.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/format/src/finite.rs

@@ -0,0 +1,185 @@
+//! A serde serializer adapter that writes non-finite floats as 0.
+//!
+//! serde_json writes NaN and ±infinity as `null`, which doesn't read back as a number: a single
+//! NaN anywhere in the model (a rotation, a crop, a stroke width) made a saved file fail to open
+//! as "damaged". [`Finite`] wraps any `Serialize` value and replaces those floats on the way out;
+//! everything else passes through unchanged.
+
+use serde::ser::{self, Serialize, Serializer};
+
+/// `T`, serialized with non-finite floats written as 0.
+pub struct Finite<'a, T: ?Sized>(pub &'a T);
+
+impl<T: ?Sized + Serialize> Serialize for Finite<'_, T> {
+    fn serialize<S: Serializer>(&self, s: S) -> Result<S::Ok, S::Error> {
+        self.0.serialize(Wrap(s))
+    }
+}
+
+struct Wrap<S>(S);
+
+macro_rules! pass {
+    ($($m:ident($t:ty)),* $(,)?) => {
+        $(fn $m(self, v: $t) -> Result<S::Ok, S::Error> { self.0.$m(v) })*
+    };
+}
+
+impl<S: Serializer> Serializer for Wrap<S> {
+    type Ok = S::Ok;
+    type Error = S::Error;
+    type SerializeSeq = Wrap<S::SerializeSeq>;
+    type SerializeTuple = Wrap<S::SerializeTuple>;
+    type SerializeTupleStruct = Wrap<S::SerializeTupleStruct>;
+    type SerializeTupleVariant = Wrap<S::SerializeTupleVariant>;
+    type SerializeMap = Wrap<S::SerializeMap>;
+    type SerializeStruct = Wrap<S::SerializeStruct>;
+    type SerializeStructVariant = Wrap<S::SerializeStructVariant>;
+
+    pass!(
+        serialize_bool(bool),
+        serialize_i8(i8),
+        serialize_i16(i16),
+        serialize_i32(i32),
+        serialize_i64(i64),
+        serialize_i128(i128),
+        serialize_u8(u8),
+        serialize_u16(u16),
+        serialize_u32(u32),
+        serialize_u64(u64),
+        serialize_u128(u128),
+        serialize_char(char),
+        serialize_str(&str),
+        serialize_bytes(&[u8]),
+    );
+
+    fn serialize_f32(self, v: f32) -> Result<S::Ok, S::Error> {
+        self.0.serialize_f32(if v.is_finite() { v } else { 0.0 })
+    }
+    fn serialize_f64(self, v: f64) -> Result<S::Ok, S::Error> {
+        self.0.serialize_f64(if v.is_finite() { v } else { 0.0 })
+    }
+    fn serialize_none(self) -> Result<S::Ok, S::Error> {
+        self.0.serialize_none()
+    }
+    fn serialize_some<T: ?Sized + Serialize>(self, v: &T) -> Result<S::Ok, S::Error> {
+        self.0.serialize_some(&Finite(v))
+    }
+    fn serialize_unit(self) -> Result<S::Ok, S::Error> {
+        self.0.serialize_unit()
+    }
+    fn serialize_unit_struct(self, name: &'static str) -> Result<S::Ok, S::Error> {
+        self.0.serialize_unit_struct(name)
+    }
+    fn serialize_unit_variant(self, name: &'static str, i: u32, variant: &'static str) -> Result<S::Ok, S::Error> {
+        self.0.serialize_unit_variant(name, i, variant)
+    }
+    fn serialize_newtype_struct<T: ?Sized + Serialize>(self, name: &'static str, v: &T) -> Result<S::Ok, S::Error> {
+        self.0.serialize_newtype_struct(name, &Finite(v))
+    }
+    fn serialize_newtype_variant<T: ?Sized + Serialize>(self, name: &'static str, i: u32, variant: &'static str, v: &T) -> Result<S::Ok, S::Error> {
+        self.0.serialize_newtype_variant(name, i, variant, &Finite(v))
+    }
+    fn serialize_seq(self, len: Option<usize>) -> Result<Self::SerializeSeq, S::Error> {
+        self.0.serialize_seq(len).map(Wrap)
+    }
+    fn serialize_tuple(self, len: usize) -> Result<Self::SerializeTuple, S::Error> {
+        self.0.serialize_tuple(len).map(Wrap)
+    }
+    fn serialize_tuple_struct(self, name: &'static str, len: usize) -> Result<Self::SerializeTupleStruct, S::Error> {
+        self.0.serialize_tuple_struct(name, len).map(Wrap)
+    }
+    fn serialize_tuple_variant(self, name: &'static str, i: u32, variant: &'static str, len: usize) -> Result<Self::SerializeTupleVariant, S::Error> {
+        self.0.serialize_tuple_variant(name, i, variant, len).map(Wrap)
+    }
+    fn serialize_map(self, len: Option<usize>) -> Result<Self::SerializeMap, S::Error> {
+        self.0.serialize_map(len).map(Wrap)
+    }
+    fn serialize_struct(self, name: &'static str, len: usize) -> Result<Self::SerializeStruct, S::Error> {
+        self.0.serialize_struct(name, len).map(Wrap)
+    }
+    fn serialize_struct_variant(
+        self,
+        name: &'static str,
+        i: u32,
+        variant: &'static str,
+        len: usize,
+    ) -> Result<Self::SerializeStructVariant, S::Error> {
+        self.0.serialize_struct_variant(name, i, variant, len).map(Wrap)
+    }
+    fn is_human_readable(&self) -> bool {
+        self.0.is_human_readable()
+    }
+}
+
+macro_rules! compound {
+    ($tr:ident, $m:ident) => {
+        impl<S: ser::$tr> ser::$tr for Wrap<S> {
+            type Ok = S::Ok;
+            type Error = S::Error;
+            fn $m<T: ?Sized + Serialize>(&mut self, v: &T) -> Result<(), S::Error> {
+                self.0.$m(&Finite(v))
+            }
+            fn end(self) -> Result<S::Ok, S::Error> {
+                self.0.end()
+            }
+        }
+    };
+}
+compound!(SerializeSeq, serialize_element);
+compound!(SerializeTuple, serialize_element);
+compound!(SerializeTupleStruct, serialize_field);
+compound!(SerializeTupleVariant, serialize_field);
+
+impl<S: ser::SerializeMap> ser::SerializeMap for Wrap<S> {
+    type Ok = S::Ok;
+    type Error = S::Error;
+    fn serialize_key<T: ?Sized + Serialize>(&mut self, k: &T) -> Result<(), S::Error> {
+        self.0.serialize_key(&Finite(k))
+    }
+    fn serialize_value<T: ?Sized + Serialize>(&mut self, v: &T) -> Result<(), S::Error> {
+        self.0.serialize_value(&Finite(v))
+    }
+    fn end(self) -> Result<S::Ok, S::Error> {
+        self.0.end()
+    }
+}
+
+macro_rules! fields {
+    ($tr:ident) => {
+        impl<S: ser::$tr> ser::$tr for Wrap<S> {
+            type Ok = S::Ok;
+            type Error = S::Error;
+            fn serialize_field<T: ?Sized + Serialize>(&mut self, key: &'static str, v: &T) -> Result<(), S::Error> {
+                self.0.serialize_field(key, &Finite(v))
+            }
+            fn skip_field(&mut self, key: &'static str) -> Result<(), S::Error> {
+                self.0.skip_field(key)
+            }
+            fn end(self) -> Result<S::Ok, S::Error> {
+                self.0.end()
+            }
+        }
+    };
+}
+fields!(SerializeStruct);
+fields!(SerializeStructVariant);
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn non_finite_floats_are_written_as_zero_and_nothing_else_changes() {
+        #[derive(serde::Serialize)]
+        struct T {
+            a: f64,
+            b: Option<f64>,
+            c: Vec<(f32, f64)>,
+            d: Option<String>,
+            e: std::collections::BTreeMap<String, f64>,
+        }
+        let t = T { a: f64::NAN, b: Some(f64::INFINITY), c: vec![(f32::NEG_INFINITY, 1.5)], d: None, e: [("k".to_string(), f64::NAN)].into() };
+        let s = serde_json::to_string(&Finite(&t)).unwrap_or_default();
+        assert_eq!(s, r#"{"a":0.0,"b":0.0,"c":[[0.0,1.5]],"d":null,"e":{"k":0.0}}"#);
+    }
+}

crates/format/src/lib.rs

@@ -4,6 +4,8 @@
 #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)]
 #![forbid(unsafe_code)]
 
+mod finite;
+
 use std::io::{Cursor, Read, Write};
 use std::sync::Arc;
 
@@ -66,7 +68,7 @@ pub fn save(p: &Presentation) -> Result<Vec<u8>> {
             generator: format!("DeckCraft {}", env!("CARGO_PKG_VERSION")),
             presentation: p.clone(),
         };
-        let json = serde_json::to_vec(&m).map_err(|e| FormatError::Write(e.to_string()))?;
+        let json = serde_json::to_vec(&finite::Finite(&m)).map_err(|e| FormatError::Write(e.to_string()))?;
         z.start_file("presentation.json", deflate).map_err(w)?;
         z.write_all(&json).map_err(|e| FormatError::Write(e.to_string()))?;
         for item in &p.media {
@@ -132,7 +134,7 @@ pub fn load_checked(bytes: &[u8]) -> Result<(Presentation, Vec<String>)> {
 fn load_within(bytes: &[u8], budget: Budget) -> Result<(Presentation, Vec<String>)> {
     let mut z = zip::ZipArchive::new(Cursor::new(bytes)).map_err(|e| FormatError::NotOurs(e.to_string()))?;
     let json = read_entry(&mut z, "presentation.json").map_err(|_| FormatError::NotOurs("no presentation.json".into()))?;
-    let m: Manifest = serde_json::from_slice(&json).map_err(|e| FormatError::Damaged(e.to_string()))?;
+    let m = parse_manifest(&json)?;
     if m.format != "deckcraft" {
         return Err(FormatError::NotOurs(m.format));
     }
@@ -196,6 +198,7 @@ pub fn repair(p: &mut Presentation) {
     if !(p.notes_size.width.is_finite() && p.notes_size.width > 0.0 && p.notes_size.height.is_finite() && p.notes_size.height > 0.0) {
         p.notes_size = Presentation::default().notes_size;
     }
+    sane_numbers(p);
 }
 
 /// Smallest and largest slide side, in points: PowerPoint's 1–56 in, as Design › Slide Size allows.
@@ -236,6 +239,168 @@ fn add_outline_slides(p: &mut Presentation, slides: OutlineSlides) -> usize {
     made
 }
 
+/// Most `null`s standing in for numbers that [`parse_manifest`] patches in one file, and the
+/// candidates it tries for each.
+const MAX_NULL_FIXES: usize = 256;
+const MAX_TRIES_PER_FIX: usize = 32;
+/// Bytes [`parse_manifest`] may re-parse in all while patching, so a hostile file full of `null`s
+/// can't keep it busy: past this the file is reported damaged.
+const MAX_REPARSE_BYTES: usize = 256 * 1024 * 1024;
+
+/// Where a parse failed, comparable: later is progress.
+fn failure(src: &[u8]) -> std::result::Result<Manifest, (String, (usize, usize))> {
+    serde_json::from_slice::<Manifest>(src).map_err(|e| (e.to_string(), (e.line(), e.column())))
+}
+
+/// Parse `presentation.json`. serde_json writes a NaN or infinite number as `null`, which then
+/// fails to read back as a number ("invalid type: null, expected f64"), so DeckCraft could save
+/// a file it couldn't open. Each such `null` is read as 0 instead ([`repair`] then brings the
+/// value into range); anything else that doesn't parse is damage.
+///
+/// serde reports the failing `null` exactly, except inside tagged enums (buffered), where it
+/// reports the end of the enclosing object: there the `null`s before that point are tried
+/// nearest first, and a patch is kept only if the parse then gets further. (Files saved since
+/// [`finite::Finite`] never contain such `null`s; this reads older ones.)
+fn parse_manifest(json: &[u8]) -> Result<Manifest> {
+    let mut src = json.to_vec();
+    let mut last = match failure(&src) {
+        Ok(m) => return Ok(m),
+        Err(f) => f,
+    };
+    let mut budget = MAX_REPARSE_BYTES;
+    'fixes: for _ in 0..MAX_NULL_FIXES {
+        let (msg, pos) = &last;
+        let number = msg.starts_with("invalid type: null, expected f") || msg.starts_with("invalid type: null, expected a number");
+        if !number {
+            break;
+        }
+        let end = offset(&src, pos.0, pos.1).unwrap_or(src.len());
+        let mut progressed = false;
+        let candidates: Vec<usize> = value_nulls_before(&src, end + 4).take(MAX_TRIES_PER_FIX).collect();
+        for at in candidates {
+            let (Some(head), Some(tail)) = (src.get(..at), src.get(at.saturating_add(4)..)) else { continue };
+            let Some(left) = budget.checked_sub(src.len()) else { break 'fixes };
+            budget = left;
+            let mut next = Vec::with_capacity(src.len());
+            next.extend_from_slice(head);
+            next.push(b'0');
+            next.extend_from_slice(tail);
+            match failure(&next) {
+                Ok(m) => return Ok(m),
+                // The patch shortened the text by 3 bytes before the error: compare positions
+                // allowing for that.
+                Err(f) if f.1 > (pos.0, pos.1.saturating_sub(3)) => {
+                    src = next;
+                    last = f;
+                    progressed = true;
+                    break;
+                }
+                Err(_) => {}
+            }
+        }
+        if !progressed {
+            break;
+        }
+    }
+    Err(FormatError::Damaged(last.0))
+}
+
+/// Byte offset of a 1-based `line`:`column`.
+fn offset(src: &[u8], line: usize, column: usize) -> Option<usize> {
+    let mut start = 0;
+    for _ in 1..line {
+        start += src.get(start..)?.iter().position(|b| *b == b'\n')? + 1;
+    }
+    Some((start + column).min(src.len()))
+}
+
+/// Starts of `null` tokens in value position (after `:`, `[` or `,`) before `end`, nearest first.
+fn value_nulls_before(src: &[u8], end: usize) -> impl Iterator<Item = usize> + '_ {
+    let end = end.min(src.len());
+    (0..end.saturating_sub(3)).rev().filter(move |&i| {
+        src.get(i..i + 4) == Some(b"null")
+            && src[..i].iter().rev().find(|b| !b.is_ascii_whitespace()).is_some_and(|b| matches!(b, b':' | b'[' | b','))
+            && src.get(i + 4).is_none_or(|b| !b.is_ascii_alphanumeric())
+    })
+}
+
+/// Largest coordinate or size kept, in points (about 350 m; PowerPoint's own limit is far smaller).
+const MAX_COORD: f64 = 1.0e6;
+
+/// Numbers no view can draw or no file should keep: non-finite positions, sizes and angles become
+/// 0, huge ones are clamped, angles are brought into 0–360, crops into a sane range, font sizes
+/// into 1–4000 pt and line widths into 0–1584 pt (PowerPoint's limits).
+fn sane_xfrm(x: &mut deckcraft_model::Xfrm) {
+    let fin = |v: f64| if v.is_finite() { v } else { 0.0 };
+    x.x = fin(x.x).clamp(-MAX_COORD, MAX_COORD);
+    x.y = fin(x.y).clamp(-MAX_COORD, MAX_COORD);
+    x.w = fin(x.w).clamp(0.0, MAX_COORD);
+    x.h = fin(x.h).clamp(0.0, MAX_COORD);
+    x.rot = fin(x.rot).rem_euclid(360.0);
+    if x.rot >= 360.0 {
+        x.rot = 0.0;
+    }
+}
+
+fn sane_run(p: &mut deckcraft_model::text::RunProps) {
+    p.size = p.size.filter(|v| v.is_finite() && *v > 0.0).map(|v| v.clamp(1.0, 4000.0));
+}
+
+fn sane_shapes(shapes: &mut [deckcraft_model::Shape], depth: usize) {
+    if depth > 64 {
+        return;
+    }
+    for sh in shapes {
+        if let Some(x) = sh.xfrm.as_mut() {
+            sane_xfrm(x);
+        }
+        if let Some(l) = sh.line.as_mut() {
+            l.width = l.width.filter(|v| v.is_finite() && *v >= 0.0).map(|v| v.min(1584.0));
+        }
+        if let Some(t) = sh.text.as_mut() {
+            for para in &mut t.paragraphs {
+                sane_run(&mut para.end_props);
+                for r in &mut para.runs {
+                    sane_run(&mut r.props);
+                }
+            }
+        }
+        match &mut sh.kind {
+            deckcraft_model::ShapeKind::Picture { fill } => {
+                for c in &mut fill.crop {
+                    *c = if c.is_finite() { c.clamp(-10.0, 1.0) } else { 0.0 };
+                }
+            }
+            deckcraft_model::ShapeKind::Group { children, child } => {
+                sane_xfrm(child);
+                sane_shapes(children, depth + 1);
+            }
+            _ => {}
+        }
+    }
+}
+
+/// [`sane_shapes`] over every slide, layout and master; slides are only copied when they change.
+pub fn sane_numbers(p: &mut Presentation) {
+    for slide in &mut p.slides {
+        let mut fixed = (**slide).clone();
+        sane_shapes(&mut fixed.shapes, 0);
+        if fixed != **slide {
+            *slide = Arc::new(fixed);
+        }
+    }
+    for m in p.masters.iter_mut().chain(p.notes_master.iter_mut()).chain(p.handout_master.iter_mut()) {
+        let mut fixed = (**m).clone();
+        sane_shapes(&mut fixed.shapes, 0);
+        for l in &mut fixed.layouts {
+            sane_shapes(&mut l.shapes, 0);
+        }
+        if fixed != **m {
+            *m = Arc::new(fixed);
+        }
+    }
+}
+
 /// Outline text: one slide per unindented line; tab-indented lines become body bullets at their
 /// level (Insert ▸ Slides from Outline, View ▸ Outline copy).
 pub fn outline_to_slides(p: &mut Presentation, text: &str) -> usize {
@@ -449,6 +614,91 @@ mod tests {
         assert!(load(&b).is_err());
     }
 
+    /// Non-finite numbers are written as `null` by serde_json; the file still opens, with each
+    /// such number repaired (it failed as "invalid type: null, expected f64").
+    #[test]
+    fn a_file_saved_with_non_finite_numbers_reopens() {
+        use deckcraft_model::{Shape, ShapeKind, Xfrm};
+        let mut p = Presentation::default();
+        let nan = f64::NAN;
+        let inf = f64::INFINITY;
+        let mut pic = Shape { xfrm: Some(Xfrm { x: nan, y: -inf, w: 1e300, h: -4.0, rot: 725.0, ..Default::default() }), ..Default::default() };
+        pic.kind = ShapeKind::Picture { fill: deckcraft_model::style::PictureFill { crop: [nan, inf, 0.25, -1e9], ..Default::default() } };
+        pic.line = Some(deckcraft_model::Line { width: Some(1e9), ..Default::default() });
+        let mut text = Shape { xfrm: Some(Xfrm { rot: inf, w: 100.0, h: 50.0, ..Default::default() }), ..Default::default() };
+        let mut body = deckcraft_model::text::TextBody::default();
+        let mut para = deckcraft_model::text::Paragraph::default();
+        para.runs.push(deckcraft_model::text::Run::with("x", deckcraft_model::text::RunProps { size: Some(-3.0), ..Default::default() }));
+        para.end_props.size = Some(1e9);
+        body.paragraphs.push(para);
+        text.text = Some(body);
+        let group = Shape {
+            xfrm: Some(Xfrm { w: 10.0, h: 10.0, ..Default::default() }),
+            kind: ShapeKind::Group { children: vec![pic, text], child: Xfrm { x: nan, w: inf, h: 10.0, ..Default::default() } },
+            ..Default::default()
+        };
+        Arc::make_mut(&mut p.slides[0]).shapes.push(group);
+        let bytes = save(&p).unwrap();
+        let back = load(&bytes).unwrap_or_else(|e| panic!("{e}"));
+        let g = back.slides[0].shapes.last().unwrap();
+        let ShapeKind::Group { children, child } = &g.kind else { panic!("group") };
+        assert_eq!((child.x, child.w), (0.0, 0.0));
+        let x = children[0].xfrm.unwrap();
+        assert_eq!((x.x, x.y, x.w, x.h, x.rot), (0.0, 0.0, MAX_COORD, 0.0, 5.0));
+        let ShapeKind::Picture { fill } = &children[0].kind else { panic!("picture") };
+        assert_eq!(fill.crop, [0.0, 0.0, 0.25, -10.0]);
+        assert_eq!(children[0].line.as_ref().unwrap().width, Some(1584.0));
+        assert_eq!(children[1].xfrm.unwrap().rot, 0.0);
+        let para = &children[1].text.as_ref().unwrap().paragraphs[0];
+        assert_eq!(para.runs[0].props.size, None);
+        assert_eq!(para.end_props.size, Some(4000.0));
+        // Saved again, it is clean.
+        assert_eq!(load(&save(&back).unwrap()).unwrap(), back);
+    }
+
+    /// Files written before [`finite::Finite`] (plain serde_json: NaN as `null`) open too,
+    /// including a `null` inside a tagged enum, where serde reports only the enclosing object.
+    #[test]
+    fn older_files_with_null_numbers_open() {
+        use deckcraft_model::{Shape, ShapeKind, Xfrm};
+        let nan = f64::NAN;
+        for case in 0..3 {
+            let mut p = Presentation::default();
+            let sh = match case {
+                0 => Shape { xfrm: Some(Xfrm { x: nan, rot: nan, w: 5.0, h: 5.0, ..Default::default() }), ..Default::default() },
+                1 => Shape {
+                    kind: ShapeKind::Group { children: vec![], child: Xfrm { x: nan, w: 1.0, h: 1.0, ..Default::default() } },
+                    ..Default::default()
+                },
+                _ => Shape {
+                    kind: ShapeKind::Picture { fill: deckcraft_model::style::PictureFill { crop: [0.1, nan, 0.0, 0.0], ..Default::default() } },
+                    ..Default::default()
+                },
+            };
+            Arc::make_mut(&mut p.slides[0]).shapes.push(sh);
+            let m = Manifest { format: "deckcraft".into(), version: VERSION, generator: "old".into(), presentation: p };
+            let json = serde_json::to_vec(&m).unwrap();
+            assert!(json.windows(4).any(|w| w == b"null"));
+            let back = parse_manifest(&json).unwrap_or_else(|e| panic!("case {case}: {e}"));
+            assert_eq!(back.presentation.slides[0].shapes.len(), m.presentation.slides[0].shapes.len(), "case {case}");
+        }
+    }
+
+    /// Only `null`s where numbers belong are patched: other damage still fails.
+    #[test]
+    fn other_damage_is_still_reported() {
+        let ok = serde_json::to_vec(&serde_json::json!({"format": "deckcraft", "version": 1, "generator": "t", "presentation": {}})).unwrap();
+        assert!(parse_manifest(&ok).is_ok());
+        let bad = br#"{"format": null, "version": 1, "generator": "t", "presentation": {}}"#;
+        assert!(parse_manifest(bad).is_err());
+        let truncated = &ok[..ok.len() - 3];
+        assert!(parse_manifest(truncated).is_err());
+        // A null number on a later line (pretty-printed by another tool) is found too.
+        let pretty = b"{\n  \"format\": \"deckcraft\",\n  \"version\": 1,\n  \"generator\": \"t\",\n  \"presentation\": {\"slides\": [{\"shapes\": [{\"xfrm\": {\"x\": null, \"y\": 1, \"w\": 2, \"h\": 3}}]}]}\n}";
+        let m = parse_manifest(pretty).unwrap_or_else(|e| panic!("{e}"));
+        assert_eq!(m.presentation.slides[0].shapes[0].xfrm.unwrap().x, 0.0);
+    }
+
     #[test]
     fn degenerate_slide_sizes_are_repaired() {
         use deckcraft_model::Size;