From 7aa00b41da99696ad0befb88bdd17dfaca7bacbc Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:28:49 +0700 Subject: [PATCH 1/4] Fix #1718: ids at their largest value in a file no longer exhaust the counters A native file (or a pasted document) whose node, slice or asset ids or whose next-id counter sit at u64::MAX made the next allocation overflow: a panic with overflow checks, a wrapped counter handing out ids already in use without. fix_next_id now leaves ids and counters from 2^62 up out of the count, so new ids start under them; alloc_id saturates. Artboard and graphic style ids take the smallest free id when the largest is u32::MAX instead of wrapping to the reserved 0. --- crates/doc/src/lib.rs | 85 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 76 insertions(+), 9 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 54af60a9..8c3ad1a8 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -775,7 +775,9 @@ impl Document { /// Allocate a fresh node id. pub fn alloc_id(&mut self) -> NodeId { let id = NodeId(self.next_id); - self.next_id += 1; + // Never reached from a counter [`Self::fix_next_id`] has seen (it stays under + // [`MAX_NEXT_ID`]); saturating so that no counter can overflow. + self.next_id = self.next_id.saturating_add(1); id } pub fn peek_next_id(&self) -> u64 { @@ -783,14 +785,22 @@ impl Document { } /// Ensure `next_id` is above every id in the tree (after deserializing foreign data). pub fn fix_next_id(&mut self) { + // Ids from [`MAX_NEXT_ID`] up (a damaged or hand-made file: #1718) are left out, as is a + // counter up there: new ids start under them and never reach them, where counting on from + // the largest would run out and hand out ids the document already uses. let mut max = 0; + let mut see = |id: u64| { + if id < MAX_NEXT_ID { + max = max.max(id); + } + }; for l in &self.layers { - l.walk(&mut |n| max = max.max(n.id.0)); + l.walk(&mut |n| see(n.id.0)); } - max = self.slices.iter().fold(max, |m, s| m.max(s.id.0)); - max = self.assets.iter().fold(max, |m, a| m.max(a.id)); - // An id of u64::MAX (a damaged file) can't overflow: ids after it are reused at worst. - self.next_id = self.next_id.max(max.saturating_add(1)); + self.slices.iter().for_each(|s| see(s.id.0)); + self.assets.iter().for_each(|a| see(a.id)); + let counter = Some(self.next_id).filter(|n| *n <= MAX_NEXT_ID).unwrap_or(0); + self.next_id = counter.max(max.saturating_add(1)); } /// Find a node anywhere in the tree. @@ -1376,7 +1386,7 @@ impl Document { self.guides.extend(copies); } pub fn next_artboard_id(&self) -> u32 { - self.artboards.iter().map(|a| a.id).max().unwrap_or(0) + 1 + free_id(self.artboards.iter().map(|a| a.id)) } pub fn pattern(&self, name: &str) -> Option<&PatternDef> { self.patterns.iter().find(|p| p.name == name) @@ -1460,7 +1470,7 @@ impl Document { } /// An id no graphic style has. pub fn next_graphic_style_id(&self) -> u32 { - self.graphic_styles.iter().map(|g| g.id).max().unwrap_or(0) + 1 + free_id(self.graphic_styles.iter().map(|g| g.id)) } /// The id of graphic style `index`, assigning one if it has none yet. pub fn graphic_style_id(&mut self, index: usize) -> u32 { @@ -1499,11 +1509,29 @@ fn default_graphic_styles() -> Vec { .collect() } +/// Node, slice and asset ids are handed out below this: no document gets near it by counting, so +/// larger ids in a file are damage (or made by hand) and the counter never follows them (#1718). +const MAX_NEXT_ID: u64 = 1 << 62; + +/// An id (never 0) none of `used` has: one past the largest, or, when that is the largest there +/// is (a damaged file), the smallest one free. +fn free_id(used: impl Iterator) -> u32 { + let mut used: Vec = used.collect(); + if let Some(next) = used.iter().max().copied().unwrap_or(0).checked_add(1) { + return next; + } + used.sort_unstable(); + used.dedup(); + // Ids 1, 2, 3… as far as they are all taken: the first that isn't follows them. + let taken = used.iter().filter(|&&id| id != 0).zip(1u32..).take_while(|&(&id, n)| id == n).count(); + u32::try_from(taken).map_or(u32::MAX, |n| n.saturating_add(1)) +} + impl Document { /// Make the next id allocated at least `next`: ids handed out outside the layer tree (such /// as an importer's opacity mask and pattern art), which [`Document::fix_next_id`] doesn't see. pub fn reserve_ids(&mut self, next: u64) { - self.next_id = self.next_id.max(next); + self.next_id = self.next_id.max(next.min(MAX_NEXT_ID)); } } @@ -1628,6 +1656,45 @@ mod tests { assert_eq!(d.layer_color(l2), LAYER_COLORS[1].1); } + /// #1718: a file whose ids or counters are at their type's largest value still hands out ids + /// nothing in it uses (and never 0), instead of overflowing. + #[test] + fn exhausted_ids_in_a_file_dont_run_out() { + let (d, a, b) = doc_with_rects(); + let mut v = serde_json::to_value(&d).unwrap(); + v["next_id"] = serde_json::json!(u64::MAX); + // Object `a` gets the largest id there is. + fn renumber(v: &mut serde_json::Value, from: u64) { + match v { + serde_json::Value::Object(o) => { + if o.get("id").and_then(serde_json::Value::as_u64) == Some(from) { + o.insert("id".into(), serde_json::json!(u64::MAX)); + } + o.values_mut().for_each(|c| renumber(c, from)); + } + serde_json::Value::Array(a) => a.iter_mut().for_each(|c| renumber(c, from)), + _ => {} + } + } + renumber(&mut v["layers"], a.0); + let mut d: Document = serde_json::from_value(v).unwrap(); + assert!(d.node(NodeId(u64::MAX)).is_some(), "the file's largest id is kept"); + d.fix_next_id(); + let fresh: Vec = (0..3).map(|_| d.alloc_id()).collect(); + assert!(fresh.iter().all(|id| d.node(*id).is_none() && id.0 > b.0 && id.0 < MAX_NEXT_ID), "{fresh:?}"); + assert!(fresh[0] != fresh[1] && fresh[1] != fresh[2]); + // A counter left at the end by other means stays there rather than wrapping to 0. + d.next_id = u64::MAX; + assert_eq!((d.alloc_id(), d.peek_next_id()), (NodeId(u64::MAX), u64::MAX)); + d.reserve_ids(u64::MAX); + // Artboards and graphic styles: one past the largest, else the smallest id free. + assert_eq!(free_id([1, 2, 7].into_iter()), 8); + assert_eq!(free_id([u32::MAX, 0, 1, 2, 4].into_iter()), 3); + assert_eq!(free_id([u32::MAX].into_iter()), 1); + d.artboards[0].id = u32::MAX; + assert_eq!(d.next_artboard_id(), 1); + } + #[test] fn serde_roundtrip() { let (d, _, _) = doc_with_rects(); From 355bb5c4f6ea271377dce4ddc38e9eddd449a651 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:29:36 +0700 Subject: [PATCH 2/4] Fix #1697: a failed command.batch restores each document whole The rollback undid the batch's interaction, which holds the art, the selection and panel state only: a Save As step's new file, format and saved state stayed, and an Undo step (which ends the interaction) left the art and the history undone. The batch now keeps the documents as they were and puts them back, and settles a drag or typing in progress first, as Undo does, instead of running inside it and keeping its baseline as the batch's step. Files the steps wrote stay written. --- crates/engine/src/cmd/edit.rs | 2 +- crates/engine/src/cmd/fileio/batch.rs | 41 ++++++++++++++------------- crates/engine/tests/model_based.rs | 38 +++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index b439766b..57ddb369 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -265,7 +265,7 @@ fn undo_group_end(s: &mut Session, p: &Value) -> Result { /// under an interaction that would then preview on, or cancel to, the document from before (#1719). /// A typing session also ends in the Type tool, whose caret and marked text (IME) must follow the /// document. -fn settle_interaction(s: &mut Session) -> Result<()> { +pub(crate) fn settle_interaction(s: &mut Session) -> Result<()> { let typing = typing_in_progress(s); s.commit_interaction()?; if typing { diff --git a/crates/engine/src/cmd/fileio/batch.rs b/crates/engine/src/cmd/fileio/batch.rs index dd836846..99d16667 100644 --- a/crates/engine/src/cmd/fileio/batch.rs +++ b/crates/engine/src/cmd/fileio/batch.rs @@ -13,14 +13,22 @@ pub(super) fn batch(s: &mut Session, p: &Value) -> Result { // What an error rolls back besides the documents: session-level state a step may change // (paint defaults, drawing mode, clipboard, the Untitled-N count). let saved = (s.paint.clone(), s.fill_active, s.draw_mode, s.draw_inside, s.clipboard.clone(), s.untitled_counter); - let (start, active): (Vec, _) = (s.docs.iter().map(|d| d.uid).collect(), s.active); + // A drag or typing still in progress is a step of its own, as before Undo: the batch's steps + // would otherwise run inside it and take its baseline as theirs (#1697). + if s.active().is_some() { + super::super::edit::settle_interaction(s)?; + } + // The documents as they are now, whole: what a failed step brings back. Their art, history, + // file, format and saved state share their data with the open ones until a step changes them. + let start = s.docs.clone(); + let active = s.active; s.batch_stash = Some(vec![]); let r = run_steps(s, &label, artboard, &mut cmds); let stash = s.batch_stash.take().unwrap_or_default(); let results = match r { Ok(results) => results, Err(e) => { - roll_back(s, &start, stash); + roll_back(s, start, &stash); s.active = active; (s.paint, s.fill_active, s.draw_mode, s.draw_inside, s.clipboard, s.untitled_counter) = saved; s.pending_paint = None; @@ -81,22 +89,17 @@ fn run_steps(s: &mut Session, label: &str, artboard: Option, cmds: &mut [Va Ok(results) } -/// The documents as they were before the batch (`start`: their uids in order): the ones it opened -/// close, the ones it closed or replaced (`stash`) come back, and each one's interaction is undone. -fn roll_back(s: &mut Session, start: &[u64], mut stash: Vec) { - let mut open = std::mem::take(&mut s.docs); - for &uid in start { - // The newest revision the document reached, so the restored one redraws. - let newest = open.iter().chain(&stash).filter(|d| d.uid == uid).map(|d| d.revision).max().unwrap_or_default(); - // Its state before the batch: the first one a step closed or replaced, else the open one. - let st = match stash.iter().position(|d| d.uid == uid) { - Some(i) => Some(stash.remove(i)), - None => open.iter().position(|d| d.uid == uid).map(|i| open.remove(i)), - }; - if let Some(mut st) = st { - st.undo_interaction(); - st.revision = newest.saturating_add(1); - s.docs.push(st); - } +/// The documents as they were before the batch (`start`), whatever the steps did to them: opened +/// ones close, closed or replaced ones (`stash`) come back, and each has its art, selection, +/// history, file, format and saved state again (#1697: undoing the batch's interaction alone left +/// a Save As step's new file and an Undo step's history in place). Files the steps wrote stay +/// written. +fn roll_back(s: &mut Session, start: Vec, stash: &[DocState]) { + let open = std::mem::take(&mut s.docs); + s.docs = start; + for st in &mut s.docs { + // Past the newest revision the document reached, so the restored one redraws. + let newest = open.iter().chain(stash).filter(|d| d.uid == st.uid).map(|d| d.revision).max().unwrap_or(st.revision); + st.revision = newest.saturating_add(1); } } diff --git a/crates/engine/tests/model_based.rs b/crates/engine/tests/model_based.rs index af91b249..13a21032 100644 --- a/crates/engine/tests/model_based.rs +++ b/crates/engine/tests/model_based.rs @@ -121,6 +121,44 @@ fn batch_is_one_undo_step_and_rolls_back_on_error() { assert_eq!(s.doc().unwrap().doc.node_count(), 1); } +/// #1697: a failed batch brings back the whole document, not only its art: the file a Save As +/// step named and the history an Undo step moved; and a batch leaves a drag in progress its own +/// step. +#[test] +fn a_failed_batch_restores_the_file_the_history_and_a_drag_in_progress() { + let mut s = fixtures::session(); + let rect = |x: i32| json!({"command": "shape.rectangle", "params": {"x": x, "y": 0, "width": 10, "height": 10}}); + let fail = json!({"command": "no.such.command", "params": {}}); + exec(&mut s, "shape.rectangle", json!({"x": 50, "y": 0, "width": 10, "height": 10})); + let before = (doc_json(&s.doc().unwrap().doc), s.doc().unwrap().history.undo.len(), s.doc().unwrap().history.redo.len()); + let state = |s: &vectorcraft_engine::Session| { + let st = s.doc().unwrap(); + (doc_json(&st.doc), st.history.undo.len(), st.history.redo.len()) + }; + // Save As, then a failing step: the document keeps no file and stays modified. + let path = vectorcraft_testkit::temp_dir("batch-rollback").join("named.vectorcraft").to_string_lossy().to_string(); + let save = json!({"command": "file.saveAs", "params": {"path": path}}); + assert!(s.execute("command.batch", &json!({"commands": [rect(0), save, fail]})).is_err()); + assert_eq!(state(&s), before); + assert_eq!((s.doc().unwrap().path.as_deref(), s.doc().unwrap().is_dirty()), (None, true)); + // Undo, then a failing step: the art and both history stacks are as before. + let undo = json!({"command": "edit.undo", "params": {}}); + assert!(s.execute("command.batch", &json!({"commands": [undo, fail]})).is_err()); + assert_eq!(state(&s), before); + // A batch during a drag: the drag is kept as a step, the batch is the next one. + exec(&mut s, "select.all", json!({})); + s.begin_interaction("Move").unwrap(); + s.preview("object.move", &json!({"dx": 5, "dy": 0})).unwrap(); + let moved = doc_json(&s.doc().unwrap().doc); + exec(&mut s, "command.batch", json!({"commands": [rect(100)]})); + assert!(!s.in_interaction()); + assert_eq!(s.doc().unwrap().history.undo.len(), before.1 + 2); + exec(&mut s, "edit.undo", json!({})); + assert_eq!(doc_json(&s.doc().unwrap().doc), moved, "undoing the batch leaves the drag's move"); + exec(&mut s, "edit.undo", json!({})); + assert_eq!(doc_json(&s.doc().unwrap().doc), before.0); +} + #[test] fn empty_batch_is_harmless() { let mut s = fixtures::session(); From ea30e411e716fe24db0c7a0642223ae10ca1770d Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:31:02 +0700 Subject: [PATCH 3/4] Fix #1725: a control request its caller timed out on is withdrawn, not run later The worker answered "timeout" after 60 s and left the request on the queue, so the app ran it whenever it next drained the queue: an edit nobody was waiting for, and a second one if the caller had retried. A request now leaves the queue once: taken by the app, or given up by its caller, whichever comes first. The timeout reply says which ("will not run" / "still running"). --- apps/vectorcraft/src/control_server.rs | 31 +++++++++++++++++++++++- crates/ui-egui/src/control.rs | 33 +++++++++++++++++++++++++- crates/ui-egui/src/lib.rs | 7 +++++- docs/control-protocol.md | 6 +++++ 4 files changed, 74 insertions(+), 3 deletions(-) diff --git a/apps/vectorcraft/src/control_server.rs b/apps/vectorcraft/src/control_server.rs index 846ebc46..21eacd61 100644 --- a/apps/vectorcraft/src/control_server.rs +++ b/apps/vectorcraft/src/control_server.rs @@ -29,6 +29,21 @@ pub const MAX_CONNECTIONS: usize = 16; /// Size in bytes of the buffer that [`write_line`] writes a reply line through. const REPLY_CHUNK: usize = 64 * 1024; +/// How long a request waits for the app's reply. +const REPLY_WAIT: Duration = Duration::from_secs(60); + +/// The reply to a request the app didn't answer in [`REPLY_WAIT`]. One it had not started is +/// withdrawn, so it never runs after its caller was told it failed; one it is running can't be +/// stopped, and the reply says its outcome isn't known. +fn timed_out(ticket: &vectorcraft_ui_egui::ControlTicket) -> Value { + let error = if ticket.give_up() { + "timeout: the app did not start the request in time; it was withdrawn and will not run" + } else { + "timeout: the app is still running the request; it may yet complete" + }; + json!({"ok": false, "error": error}) +} + #[derive(Clone, Copy)] struct ConnectionTimeouts { idle: Duration, @@ -106,9 +121,10 @@ fn spawn_connection( let _slot = slot; serve_socket(stream, timeouts, |method, params| { let (req, rrx) = ControlRequest::new(method, params); + let ticket = req.ticket(); tx.send(req).ok()?; ctx.request_repaint(); - Some(rrx.recv_timeout(Duration::from_secs(60)).unwrap_or_else(|_| json!({"ok": false, "error": "timeout"}))) + Some(rrx.recv_timeout(REPLY_WAIT).unwrap_or_else(|_| timed_out(&ticket))) }); }) } @@ -455,6 +471,19 @@ mod tests { json!({"ok": true, "result": {"layers": [{"children": paths, "id": 1, "kind": "Layer", "name": "Layer 1"}]}}) } + /// #1725: a request its caller gave up before the app took it never runs; one the app took + /// first can't be withdrawn, and the timeout says so. + #[test] + fn a_timed_out_request_is_withdrawn_unless_it_started() { + let (waiting, _rx) = ControlRequest::new("engine.execute", json!({})); + assert!(timed_out(&waiting.ticket())["error"].as_str().unwrap().contains("will not run")); + assert!(!waiting.claim(), "the app skips a withdrawn request"); + let (running, _rx) = ControlRequest::new("engine.execute", json!({})); + assert!(running.claim()); + assert!(timed_out(&running.ticket())["error"].as_str().unwrap().contains("still running")); + assert!(!running.claim(), "a request is taken once"); + } + #[test] fn requests_are_answered_in_order_with_ids() { let (r, calls) = run(b"{\"id\":1,\"method\":\"a\"}\n\n{\"id\":2,\"method\":\"b\",\"params\":{}}\r\n{\"method\":\"c\"}"); diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index ef921fdb..4cb60e21 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -24,6 +24,8 @@ //! - `app.export {path?, format?, …document.export options}`: encoded by the engine, written through //! the host; the document keeps its path. No path → `{dataBase64, format, bytes}` (as headless) +use std::sync::Arc; +use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::mpsc::Sender; use serde_json::{Value, json}; @@ -63,12 +65,41 @@ pub struct ControlRequest { pub method: String, pub params: Value, pub reply: Sender, + state: Arc, +} + +/// A request's state: waiting to run, taken by the app ([`ControlRequest::claim`]), or given up +/// by its caller before that ([`Ticket::give_up`]). It leaves `QUEUED` once, one way or the other. +const QUEUED: u8 = 0; +const STARTED: u8 = 1; +const GIVEN_UP: u8 = 2; + +/// The caller's side of a queued [`ControlRequest`]. +pub struct Ticket(Arc); + +impl Ticket { + /// Stop waiting for the request → `true` when it had not started, and so never runs: a caller + /// told "timeout" can then retry without the edit being applied twice, or applied later when + /// nobody expects it (#1725). `false`: the app is running it (or has); its outcome is unknown. + pub fn give_up(&self) -> bool { + self.0.compare_exchange(QUEUED, GIVEN_UP, Ordering::SeqCst, Ordering::SeqCst).is_ok() + } } impl ControlRequest { pub fn new(method: impl Into, params: Value) -> (Self, std::sync::mpsc::Receiver) { let (tx, rx) = std::sync::mpsc::channel(); - (Self { method: method.into(), params, reply: tx }, rx) + (Self { method: method.into(), params, reply: tx, state: Arc::new(AtomicU8::new(QUEUED)) }, rx) + } + + /// The caller's handle on this request, to give it up with. + pub fn ticket(&self) -> Ticket { + Ticket(self.state.clone()) + } + + /// Take the request to run it → `false` when its caller gave it up meanwhile (don't run it). + pub fn claim(&self) -> bool { + self.state.compare_exchange(QUEUED, STARTED, Ordering::SeqCst, Ordering::SeqCst).is_ok() } } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index d211c5c2..e3e953cc 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -162,7 +162,7 @@ use vectorcraft_engine::cmd::fileio; use vectorcraft_engine::file_access::{self, AutomationRoots}; use vectorcraft_engine::{Session, ViewInfo}; -pub use control::{ControlRequest, ControlResponse}; +pub use control::{ControlRequest, ControlResponse, Ticket as ControlTicket}; pub use state::{UiState, View}; pub use sysclip::{ClipboardProbeFactory, SystemClipboard}; @@ -769,6 +769,11 @@ impl VectorcraftApp { let mut handled = 0; while handled < control::MAX_CONTROL_REQUESTS_PER_FRAME { let Ok(req) = rx.try_recv() else { break }; + // Its caller stopped waiting (the app was busy or its window not drawn for too long) + // and was told it won't run: an edit applied now would come from nowhere (#1725). + if !req.claim() { + continue; + } handled += 1; let reply = req.reply.clone(); // Guarded per request: a panic must not drop the channel (taken out of `self` above). diff --git a/docs/control-protocol.md b/docs/control-protocol.md index bbbaf648..742f627e 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -43,6 +43,12 @@ and does not append a partial gesture. Clients can retry once earlier input has scheduled. These limits cover synthetic input admission, separately from socket transport and command-processing timeouts. +A request the app hasn't answered after **60 s** gets `{"ok": false, "error": "timeout: …"}`. If the +app had not started it (it was busy, or its window wasn't being drawn), the request is withdrawn +and never runs, and the error says "will not run": retrying is safe. If the app was already +running it, the error says "still running": it may yet complete, so check the document before +retrying. + ## Confining file access ```sh From 0cb55bc0aa56f31d16fa943139db731bdafb5b7d Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:31:04 +0700 Subject: [PATCH 4/4] Fix #1717: sampling and recolouring check an image's RGBA size before decoding The decoder's 512 MB limit is on the file's samples; a grey image within it is four times that once converted to RGBA for the Eyedropper or a recolour, which the web build can't allocate. The size is now read from the header and held to the allowance the importers and the renderer use. --- crates/doc/src/lib.rs | 1 + crates/doc/src/pixels.rs | 32 ++++++++++++++++++++++++++-- crates/engine/src/cmd/fileio/load.rs | 2 +- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 8c3ad1a8..7d7a33e5 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -31,6 +31,7 @@ pub mod overprint; pub mod pattern; pub mod perspective; mod pixels; +pub use pixels::MAX_RASTER_ALLOC; pub mod placed_document; pub mod profiles; pub mod puppet; diff --git a/crates/doc/src/pixels.rs b/crates/doc/src/pixels.rs index 219b6a81..525014fc 100644 --- a/crates/doc/src/pixels.rs +++ b/crates/doc/src/pixels.rs @@ -6,6 +6,22 @@ use std::sync::Arc; use crate::ImageBlob; +/// The most memory the decoded RGBA pixels of one image may take: the allowance the importers +/// and the renderer decode within (a 60 in sheet at 300 ppi is ~475 MB). The web app has 4 GB to +/// address and aborts when an allocation fails, so it keeps the decoder's own 512 MB. +pub const MAX_RASTER_ALLOC: u64 = if cfg!(target_arch = "wasm32") { 512 << 20 } else { 2 << 30 }; + +/// The pixels of encoded image `bytes` as RGBA; `None` when they can't be decoded or would take +/// more than `max` bytes as RGBA. The size is read from the header first: the decoder's own limit +/// is on the file's samples, and a grey image is a quarter of what it becomes here (#1717). +fn rgba_within(bytes: &[u8], max: u64) -> Option { + let (w, h) = image::ImageReader::new(Cursor::new(bytes)).with_guessed_format().ok()?.into_dimensions().ok()?; + if u64::from(w) * u64::from(h) * 4 > max { + return None; + } + Some(image::load_from_memory(bytes).ok()?.to_rgba8()) +} + impl ImageBlob { /// A PNG blob of `png` bytes. pub fn png(png: Vec) -> Self { @@ -21,7 +37,7 @@ impl ImageBlob { /// pixels left alone, re-encoded as PNG. `f` runs once per distinct colour. `None` when the /// image can't be decoded or no pixel changed. pub fn map_rgb(&self, mut f: impl FnMut([u8; 3]) -> [u8; 3]) -> Option { - let mut img = image::load_from_memory(&self.bytes).ok()?.to_rgba8(); + let mut img = rgba_within(&self.bytes, MAX_RASTER_ALLOC)?; let mut memo: HashMap<[u8; 3], [u8; 3]> = HashMap::new(); let mut changed = false; for px in img.pixels_mut().filter(|p| p.0[3] > 0) { @@ -67,7 +83,7 @@ impl ImageBlob { LAST.with_borrow_mut(|last| match last { Some((k, img)) if *k == key => Some(img.clone()), _ => { - let img = Arc::new(image::load_from_memory(&self.bytes).ok()?.to_rgba8()); + let img = Arc::new(rgba_within(&self.bytes, MAX_RASTER_ALLOC)?); *last = Some((key, img.clone())); Some(img) } @@ -115,6 +131,18 @@ mod tests { image::load_from_memory(&b.bytes).unwrap().to_rgba8().pixels().map(|p| p.0).collect() } + /// #1717: the RGBA size is checked from the header, before anything is decoded: a grey image + /// the decoder's own limit lets through is four times that as RGBA. + #[test] + fn pixels_past_the_allowance_are_not_decoded() { + let grey = image::GrayImage::from_raw(4, 2, vec![7; 8]).unwrap(); + let mut png = Vec::new(); + grey.write_to(&mut Cursor::new(&mut png), image::ImageFormat::Png).unwrap(); + assert!(rgba_within(&png, 31).is_none(), "8 grey bytes are 32 as RGBA"); + assert_eq!(rgba_within(&png, 32).map(|i| i.dimensions()), Some((4, 2))); + assert!(rgba_within(b"not an image", u64::MAX).is_none()); + } + #[test] fn samples_a_pixel_or_the_average_around_it() { let b = blob(&[[255, 0, 0, 255], [0, 255, 0, 255], [0, 0, 255, 255], [9, 9, 9, 0]], 4, 1); diff --git a/crates/engine/src/cmd/fileio/load.rs b/crates/engine/src/cmd/fileio/load.rs index 1ac08df1..891a4307 100644 --- a/crates/engine/src/cmd/fileio/load.rs +++ b/crates/engine/src/cmd/fileio/load.rs @@ -419,7 +419,7 @@ pub(super) fn new_from_template(s: &mut Session, p: &Value) -> Result { /// The most memory decoding one image may take: a 60 in print sheet at 300 ppi (18000 × 6600 px) /// is ~475 MB as RGBA, past the decoder's 512 MB default once it is converted. The web app keeps /// that default: wasm32 has 4 GB to address and aborts when an allocation fails. -pub(crate) const MAX_RASTER_ALLOC: u64 = if cfg!(target_arch = "wasm32") { 512 << 20 } else { 2 << 30 }; +pub(crate) use vectorcraft_doc::MAX_RASTER_ALLOC; /// What to say of encoded image `bytes` that need more than [`MAX_RASTER_ALLOC`] to decode (its /// header says so; the pixels aren't read): the renderer, which has the same allowance, draws