Merge pull request #3938 from dylanmtaylor/security/review-findings-render

Security: render and PDF export bounds for symbol expansion, dotted guides, blending groups and pinned images

crates/doc/src/depth.rs

@@ -25,6 +25,12 @@ use crate::{Document, Node, NodeKind};
 /// can be made can be saved and opened again.
 pub const MAX_DEPTH: usize = 32;
 
+/// Most objects the symbol instances drawn in one frame or written in one export may hold
+/// together. Instances of symbols that hold instances of others draw every level for each instance
+/// above it, so a few levels of a few instances each multiply past what [`MAX_DEPTH`] stops; past
+/// it, further instances aren't drawn.
+pub const MAX_SYMBOL_NODES: u64 = 16_000_000;
+
 /// The deepest an object may be drawn: as [`MAX_DEPTH`], the art of a symbol counted under its
 /// instance and a pattern's tile under the object it paints (and what those use, under them).
 /// Symbols a few objects deep nest in one another dozens deep within it.

crates/doc/src/lib.rs

@@ -93,7 +93,7 @@ pub use appearance::{
 };
 pub use assets::ExportAsset;
 pub use corners::LiveCorners;
-pub use depth::MAX_DEPTH;
+pub use depth::{MAX_DEPTH, MAX_SYMBOL_NODES};
 pub use graph::{
     ColumnDesign, ColumnType, Fractions, GraphDesign, GraphKind, GraphSpec, LabelStyles, MAX_GRAPH_CATEGORIES, MAX_GRAPH_SERIES, PiePosition,
     PieSort, SeriesPaint, TickLength, ValueAxisSide,

crates/pdf/src/export.rs

@@ -626,6 +626,9 @@ pub(crate) struct Exporter<'a> {
     /// Keys of the images left out because the writer couldn't decode them
     /// ([`leaving_out_undecodable`]).
     pub left_out: HashSet<String>,
+    /// Objects the symbol instances written held, together
+    /// ([`vectorcraft_doc::MAX_SYMBOL_NODES`]).
+    symbol_nodes: u64,
 }
 
 /// How deep inline graphics nest before they are left out.
@@ -665,6 +668,7 @@ impl<'a> Exporter<'a> {
             layers: false,
             layer_index: crate::forms::pdf_layers(doc).iter().enumerate().map(|(i, l)| (address(l.node), i)).collect(),
             in_layer: 0,
+            symbol_nodes: 0,
             overprint: false,
             overprinted: false,
             invisible: false,
@@ -1219,7 +1223,12 @@ impl Exporter<'_> {
             NodeKind::Text(t) => self.text(s, n, t, page),
             NodeKind::Image(im) => self.image(s, im, alpha),
             NodeKind::SymbolInstance { symbol, xf } => {
-                if let Some(sym) = self.doc.symbols.iter().find(|x| &x.name == symbol) {
+                // Within the objects a frame may draw of symbol instances, as on screen: nested
+                // instances multiply.
+                let sym = self.doc.symbols.iter().find(|x| &x.name == symbol);
+                let total = sym.map_or(0, |sym| self.symbol_nodes.saturating_add(sym.art.count() as u64));
+                if let Some(sym) = sym.filter(|_| total <= vectorcraft_doc::MAX_SYMBOL_NODES) {
+                    self.symbol_nodes = total;
                     let mut art = (*sym.art).clone();
                     art.transform(*xf, false);
                     self.node(s, &art, page, true);

crates/render/src/group.rs

@@ -49,6 +49,10 @@ pub(crate) struct Region {
     pub h: u16,
 }
 
+/// Groups drawn twice ([`Renderer::group_offscreen`]: blending with their backdrop) that may be
+/// nested in each other before deeper ones are drawn isolated, once.
+const MAX_TWICE_DRAWN: u32 = 4;
+
 impl Renderer {
     /// Draw a group's content with `draw` and composite it as `c` says. Blending content is drawn
     /// offscreen: over a copy of the backdrop when the group isn't isolated (possible only with
@@ -110,6 +114,12 @@ impl Renderer {
 
     fn group_offscreen(&mut self, ctx: &mut RenderContext, f: &Frame, c: Composite, r: Region, draw: &mut Content) {
         let Composite { clip, blend, opacity, mask, isolated, .. } = c;
+        // A group that blends with its backdrop is drawn twice (over the backdrop, and alone for
+        // its coverage), and so is everything in it, so such groups nested in each other cost 2ⁿ.
+        // Past [`MAX_TWICE_DRAWN`] of them, a deeper one is drawn as an isolated group.
+        let twice = !isolated && blend != Blend::Normal;
+        let isolated = isolated || twice && self.twice_drawn >= MAX_TWICE_DRAWN;
+        let twice = twice && !isolated;
         if self.offscreen_live == 0 {
             self.offscreen_budget = OFFSCREEN_FRAMES * u64::from(ctx.width()) * u64::from(ctx.height());
         }
@@ -117,6 +127,7 @@ impl Renderer {
         self.offscreen_live = self.offscreen_live.saturating_add(cost);
         let sub = frame_over(f, r);
         let backdrop = (!isolated).then(|| self.flatten(ctx, r));
+        self.twice_drawn += u32::from(twice);
         let content = self.offscreen(r, &sub, backdrop.clone(), draw);
         let at = (r.x as f64, r.y as f64);
         let push = |ctx: &mut RenderContext, mode: BlendMode| {
@@ -162,6 +173,7 @@ impl Renderer {
             }
         }
         ctx.pop_layer();
+        self.twice_drawn -= u32::from(twice);
         self.offscreen_live = self.offscreen_live.saturating_sub(cost);
     }
 

crates/render/src/images.rs

@@ -157,6 +157,12 @@ impl ImageStore {
         }
     }
 
+    /// Bytes of the entries someone besides the store holds (images being painted): eviction can't
+    /// free them.
+    pub(crate) fn pinned_bytes(&self) -> usize {
+        self.entries.values().filter(|e| Arc::strong_count(&e.pm) > 1).map(|e| pixmap_bytes(&e.pm)).sum()
+    }
+
     pub(crate) fn set_budget(&mut self, budget: usize) {
         self.budget = budget;
         self.evict();

crates/render/src/lib.rs

@@ -537,6 +537,10 @@ pub struct Renderer {
     /// at the start of every frame.
     offscreen_live: u64,
     offscreen_budget: u64,
+    /// Objects the symbol instances drawn in this frame held, together ([`vectorcraft_doc::MAX_SYMBOL_NODES`]).
+    symbol_nodes: u64,
+    /// Groups drawn twice (blending with their backdrop) that hold what is being drawn.
+    twice_drawn: u32,
     /// What clipping paths paint, per clipping path (see [`Self::clip_paint_of`]).
     clip_paints: FrameCache<usize, ClipPaintEntry>,
     /// Gradients along or across strokes (see [`Self::fill_path_gradient`]), keyed by
@@ -665,6 +669,8 @@ impl Renderer {
             picture: None,
             offscreen_live: 0,
             offscreen_budget: 0,
+            symbol_nodes: 0,
+            twice_drawn: 0,
             clip_paints: FrameCache::new(lru::CLIP_BUDGET, 256),
             stroke_slices: PtrMap::default(),
             adjusts: HashMap::new(),
@@ -751,6 +757,8 @@ impl Renderer {
         self.inline_depth = 0;
         // Nothing is drawn offscreen yet (even if an earlier frame stopped part way).
         (self.offscreen_live, self.offscreen_budget) = (0, 0);
+        self.symbol_nodes = 0;
+        self.twice_drawn = 0;
     }
 
     /// End a frame: drop cache entries not used for a few frames (every render, thumbnails too,
@@ -1339,9 +1347,13 @@ impl Renderer {
                 let bp = self.outline_path(f, n).unwrap_or_else(|| Arc::new(path.to_bezpath()));
                 if *guide {
                     let ([r, g, b], dots) = f.opts.guide;
-                    if dots {
-                        let mut screen = BezPath::clone(&bp);
-                        screen.apply_affine(f.view);
+                    let mut screen = BezPath::clone(&bp);
+                    screen.apply_affine(f.view);
+                    // The rasterizer makes every dash of the whole path before clipping: a guide
+                    // running far off screen (from a file) asked for millions of them. Past
+                    // [`MAX_GUIDE_DASHES`] (by its control polygon, which is at least as long) it
+                    // is drawn solid.
+                    if dots && polygon_length(&screen) / 4.0 <= MAX_GUIDE_DASHES {
                         ctx.set_transform(Affine::IDENTITY);
                         ctx.set_stroke(kurbo::Stroke::new(1.5).with_dashes(0.0, [1.5, 2.5]));
                         ctx.set_paint(f.ink.fixed([r, g, b, 255]));
@@ -1368,7 +1380,9 @@ impl Renderer {
             NodeKind::Text(t) => self.draw_text(ctx, f, n, t),
             NodeKind::Image(im) => self.draw_image(ctx, f, im),
             NodeKind::SymbolInstance { symbol, xf } => {
-                if let Some(sym) = f.doc.symbols.iter().find(|s| &s.name == symbol) {
+                if let Some(sym) = f.doc.symbols.iter().find(|s| &s.name == symbol)
+                    && self.charge_symbol(&sym.art)
+                {
                     let (art, kept) = self.placed_art(&sym.art, *xf, false, Some(n));
                     self.draw_placed_art(ctx, f, &art, kept);
                 }
@@ -1385,6 +1399,13 @@ impl Renderer {
         }
     }
 
+    /// Count symbol art `art`'s objects against the frame's [`vectorcraft_doc::MAX_SYMBOL_NODES`]: `false` (the
+    /// instance isn't drawn) once they would pass it.
+    fn charge_symbol(&mut self, art: &Node) -> bool {
+        let total = self.symbol_nodes.saturating_add(art.count() as u64);
+        (total <= vectorcraft_doc::MAX_SYMBOL_NODES).then(|| self.symbol_nodes = total).is_some()
+    }
+
     /// Draw symbol art `art` as [`Self::placed_art`] placed it for an instance or an inline graphic
     /// (`kept`: kept between frames, see [`Self::draw_kept_node`]).
     fn draw_placed_art(&mut self, ctx: &mut RenderContext, f: &Frame, art: &Arc<Node>, kept: bool) {
@@ -2379,6 +2400,33 @@ fn stain_bits(stain: Option<(vectorcraft_color::Color, f32)>) -> [u32; 6] {
 /// Preferred rasterizer thread count set by the app (Preferences → Performance); negative = automatic.
 static THREADS_OVERRIDE: std::sync::atomic::AtomicI32 = std::sync::atomic::AtomicI32::new(-1);
 
+/// Most dashes a dotted guide is drawn with (1.5 on, 2.5 off on screen); a longer one is solid.
+const MAX_GUIDE_DASHES: f64 = 100_000.0;
+
+/// The length of `p`'s control polygon: at least its own length (infinite when a point is).
+fn polygon_length(p: &BezPath) -> f64 {
+    let (mut last, mut len): (Option<kurbo::Point>, f64) = (None, 0.0);
+    for el in p.elements() {
+        let points = match *el {
+            kurbo::PathEl::MoveTo(q) => {
+                last = Some(q);
+                continue;
+            }
+            kurbo::PathEl::LineTo(q) => [Some(q), None, None],
+            kurbo::PathEl::QuadTo(a, b) => [Some(a), Some(b), None],
+            kurbo::PathEl::CurveTo(a, b, c) => [Some(a), Some(b), Some(c)],
+            kurbo::PathEl::ClosePath => continue,
+        };
+        for q in points.into_iter().flatten() {
+            if let Some(l) = last {
+                len += l.distance(q);
+            }
+            last = Some(q);
+        }
+    }
+    if len.is_nan() { f64::INFINITY } else { len }
+}
+
 /// Override the worker-thread count used by renderers created from now on (`None` = automatic).
 pub fn set_default_threads(n: Option<u16>) {
     THREADS_OVERRIDE.store(n.map_or(-1, i32::from), std::sync::atomic::Ordering::Relaxed);

crates/render/src/paint.rs

@@ -102,7 +102,11 @@ pub(crate) fn decode_within_allowance(bytes: &[u8]) -> image::ImageResult<image:
         Ok(r)
     };
     let (w, h) = reader()?.into_dimensions()?;
-    if u64::from(w) * u64::from(h) > MAX_DECODE_ALLOC / 4 {
+    let bytes = (u64::from(w) * u64::from(h)).saturating_mul(4);
+    // The images paints hold can't be evicted from the cache, so a document of many images held
+    // them all at once: a new one is decoded only while those and it fit twice the allowance.
+    let pinned = crate::images::store().pinned_bytes() as u64;
+    if bytes > MAX_DECODE_ALLOC || bytes.saturating_add(pinned) > 2 * MAX_DECODE_ALLOC {
         return Err(image::ImageError::Limits(image::error::LimitError::from_kind(image::error::LimitErrorKind::InsufficientMemory)));
     }
     reader()?.decode()