aleleba/deckcraft
1
0
Fork 0
mirror of https://github.com/storytold/deckcraft.git synced 2026-10-28 04:27:06 -06:00
Code Issues Packages Projects Releases Wiki Activity

Model: an exhausted id space refuses growth instead of duplicating ids (#407)

Browse Source 
alloc_id bumped next_id with saturating_add: a presentation whose
next_id reached u32::MAX (a hostile file can arrive that way;
fix_next_id imports it) handed out that same id to every new slide,
shape, layout and media - duplicate ids, ambiguous references, silent
corruption on every later insert.

alloc_id now returns Option<u32> via checked_add, and every caller
refuses: the engine's insert/duplicate/media/layout commands answer
with a clear error naming the command, paste and duplicate re-id
helpers propagate it, media addition is fallible, and a fresh slide is
Option. The pptx export's synthesized blank layout is guarded the same
way. Loading keeps such a deck viewable: repair re-ids duplicate
slides while it can, and a duplicate it cannot re-id stays as it is -
growth is refused, and only that duplicate keeps its ambiguous id.

Renumbering the loaded graph was rejected: it needs the
cross-document reference remapping that issue #406 tracks.

Tests: model exhaustion (alloc_id None, fresh media refused, a new
slide refused), engine (slide.new, shape.insert, slide.duplicate and
edit.duplicate all refuse; the document holds no duplicate ids),
format repair (an unre-id-able duplicate stays visible).
This commit is contained in:
Sazid Al Bayazid
2026-10-13 21:13:25 +06:00
parent 457406a7a5
commit e70fce996b
25 changed files with 176 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/arrange.rs
+1 -1
View File
@@ -177,7 +177,7 @@ fn group(s: &mut Session, p: &Value) -> Result<Value> {
let xs: Vec<Xfrm> = members.iter().filter_map(|m| m.xfrm).collect();
let b = union_bounds(&xs).ok_or_else(|| bad("arrange.group", "nothing to group"))?;
let gx = Xfrm::new(b.x0, b.y0, b.width(), b.height());
let id = ShapeId(doc.alloc_id());
let id = ShapeId(doc.alloc_id().ok_or_else(|| bad("arrange.group", NO_ID_SPACE))?);
let n = super::current_shapes(doc, sel).map(|v| v.len()).unwrap_or(0);
let g = Shape {
id,
crates/engine/src/cmd/design.rs
+5 -5
View File
@@ -177,7 +177,7 @@ fn background(s: &mut Session, p: &Value) -> Result<Value> {
} else if p.get("picture").is_some() {
let (name, bytes) = super::insert::media_bytes(&with_param(p, "data", p.get("picture").cloned().unwrap_or_default()), "design.background")?;
let ct = super::insert::content_type(&name, &bytes);
let media = s.edit(|doc, _| Ok(doc.add_media(&name, ct, bytes)))?;
let media = s.edit(|doc, _| doc.add_media(&name, ct, bytes).ok_or_else(|| bad("design.background", NO_ID_SPACE)))?;
Some(Background::Fill { fill: Fill::Picture(deckcraft_model::style::PictureFill { media, ..Default::default() }) })
} else {
return Err(bad("design.background", "give `color`, `gradient`, `pattern`, `picture`, `style` or `reset`"));
@@ -325,7 +325,7 @@ fn header_footer(s: &mut Session, p: &Value) -> Result<Value> {
};
let mut new_ids = vec![];
for _ in 0..3 {
new_ids.push(ShapeId(doc.alloc_id()));
new_ids.push(ShapeId(doc.alloc_id().ok_or_else(|| bad("design.headerFooter", NO_ID_SPACE))?));
}
let Some(sl) = doc.slides.get_mut(i) else { continue };
let sl = Arc::make_mut(sl);
@@ -459,8 +459,8 @@ fn insert_layout(s: &mut Session, p: &Value) -> Result<Value> {
let name = str_param(p, "name").unwrap_or("Custom Layout").to_string();
s.edit(|doc, sel| {
let mi = master_index(doc, sel);
let id = deckcraft_model::LayoutId(doc.alloc_id());
let tid = ShapeId(doc.alloc_id());
let id = deckcraft_model::LayoutId(doc.alloc_id().ok_or_else(|| bad("master.insertLayout", NO_ID_SPACE))?);
let tid = ShapeId(doc.alloc_id().ok_or_else(|| bad("master.insertLayout", NO_ID_SPACE))?);
let size = doc.slide_size;
let m = Arc::make_mut(doc.masters.get_mut(mi).ok_or_else(|| bad("master.insertLayout", "no master"))?);
let title = m.shapes.iter().find(|x| x.ph_type() == Some(PhType::Title)).cloned().map(|mut t| {
@@ -552,7 +552,7 @@ fn insert_placeholder(s: &mut Session, p: &Value) -> Result<Value> {
text: Some(TextBody::default()),
..Default::default()
};
let id = super::insert::add_shape(s, shape, true)?;
let id = super::insert::add_shape(s, "master.insertPlaceholder", shape, true)?;
Ok(json!({"id": id}))
}
crates/engine/src/cmd/edit.rs
+16 -15
View File
@@ -126,33 +126,34 @@ fn cut(s: &mut Session, p: &Value) -> Result<Value> {
}
/// Give pasted shapes fresh ids (groups included) and map connector ends.
pub(crate) fn reid(doc: &mut deckcraft_model::Presentation, shapes: &mut [Shape]) {
fn rec(doc: &mut deckcraft_model::Presentation, shapes: &mut [Shape], depth: usize) {
pub(crate) fn reid(cmd: &str, doc: &mut deckcraft_model::Presentation, shapes: &mut [Shape]) -> Result<()> {
fn rec(cmd: &str, doc: &mut deckcraft_model::Presentation, shapes: &mut [Shape], depth: usize) -> Result<()> {
if depth > 64 {
return;
return Ok(());
}
for s in shapes {
s.id = ShapeId(doc.alloc_id());
s.id = ShapeId(doc.alloc_id().ok_or_else(|| bad(cmd, NO_ID_SPACE))?);
if let Some(ch) = s.children_mut() {
rec(doc, ch, depth + 1);
rec(cmd, doc, ch, depth + 1)?;
}
}
Ok(())
}
rec(doc, shapes, 0);
rec(cmd, doc, shapes, 0)
}
fn bring_media(doc: &mut deckcraft_model::Presentation, media: &[deckcraft_model::MediaItem]) {
fn bring_media(doc: &mut deckcraft_model::Presentation, media: &[deckcraft_model::MediaItem]) -> Result<()> {
for m in media {
if doc.media(m.id).is_none_or(|x| x.data != m.data) {
if doc.media(m.id).is_none() {
doc.media.push(m.clone());
} else {
// Id clash with different bytes: unusual (other deck); keep ours, add theirs as new.
let id = doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone());
let _ = id;
doc.add_media(&m.name, &m.content_type, m.data.as_ref().clone()).ok_or_else(|| bad("edit.paste", NO_ID_SPACE))?;
}
}
}
Ok(())
}
fn paste(s: &mut Session, p: &Value) -> Result<Value> {
@@ -174,12 +175,12 @@ fn paste(s: &mut Session, p: &Value) -> Result<Value> {
if !clip.slides.is_empty() {
let at = s.doc()?.selection.slide + 1;
return s.edit(|doc, sel| {
bring_media(doc, &clip.media);
bring_media(doc, &clip.media)?;
let mut new_ids = vec![];
for (k, sl) in clip.slides.iter().enumerate() {
let mut sl = sl.clone();
sl.id = SlideId(doc.alloc_id());
reid(doc, &mut sl.shapes);
sl.id = SlideId(doc.alloc_id().ok_or_else(|| bad("edit.paste", NO_ID_SPACE))?);
reid("edit.paste", doc, &mut sl.shapes)?;
if doc.layout(sl.layout).is_none() {
sl.layout = doc.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default();
}
@@ -200,9 +201,9 @@ fn paste(s: &mut Session, p: &Value) -> Result<Value> {
let same_slide = s.doc().ok().is_some_and(|d| clip.shapes.iter().any(|c| d.shape(c.id).is_some()));
let off = if same_slide { off } else { 0.0 };
return s.edit(|doc, sel| {
bring_media(doc, &clip.media);
bring_media(doc, &clip.media)?;
let mut shapes = clip.shapes.clone();
reid(doc, &mut shapes);
reid("edit.paste", doc, &mut shapes)?;
for sh in &mut shapes {
if let Some(x) = sh.xfrm.as_mut() {
x.x += off;
@@ -251,7 +252,7 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
let list = super::current_shapes(doc, sel).cloned().unwrap_or_default();
let mut copies: Vec<Shape> = list.iter().filter(|x| ids.contains(&x.id)).cloned().collect();
let boxes: Vec<_> = copies.iter().map(|c| xfrm_of(doc, sel, c)).collect();
reid(doc, &mut copies);
reid("edit.duplicate", doc, &mut copies)?;
for (c, b) in copies.iter_mut().zip(boxes) {
let mut x = b;
x.x += off;
crates/engine/src/cmd/insert.rs
+17 -14
View File
@@ -104,9 +104,9 @@ fn default_rect(s: &Session, w: f64, h: f64) -> Xfrm {
Xfrm::new((size.width - w) / 2.0, (size.height - h) / 2.0, w, h)
}
pub(crate) fn add_shape(s: &mut Session, mut shape: Shape, select: bool) -> Result<ShapeId> {
pub(crate) fn add_shape(s: &mut Session, cmd: &str, mut shape: Shape, select: bool) -> Result<ShapeId> {
s.edit(|doc, sel| {
shape.id = ShapeId(doc.alloc_id());
shape.id = ShapeId(doc.alloc_id().ok_or_else(|| bad(cmd, NO_ID_SPACE))?);
if shape.name.is_empty() {
let base = match &shape.kind {
ShapeKind::Picture { .. } => "Picture".to_string(),
@@ -176,7 +176,7 @@ fn insert_shape(s: &mut Session, p: &Value) -> Result<Value> {
if let (Some(look), false) = (&s.default_look, line_like) {
super::format::apply_painted(&mut shape, look);
}
let id = add_shape(s, shape, true)?;
let id = add_shape(s, "shape.insert", shape, true)?;
Ok(json!({"id": id}))
}
@@ -204,7 +204,7 @@ fn text_box(s: &mut Session, p: &Value) -> Result<Value> {
t.body.wrap = Some(false);
}
}
let id = add_shape(s, shape, true)?;
let id = add_shape(s, "insert.textBox", shape, true)?;
super::text::fit_text_box(s, id)?;
if text.is_empty() {
// Start typing immediately.
@@ -341,7 +341,7 @@ fn picture(s: &mut Session, p: &Value) -> Result<Value> {
}
_ => [0.0; 4],
};
let media = s.edit(|doc, _| Ok(doc.add_media(&name, ct, bytes)))?;
let media = s.edit(|doc, _| doc.add_media(&name, ct, bytes).ok_or_else(|| bad("insert.picture", NO_ID_SPACE)))?;
let shape = Shape {
xfrm: Some(rect),
kind: ShapeKind::Picture { fill: PictureFill { media, crop, ..Default::default() } },
@@ -368,7 +368,7 @@ fn picture(s: &mut Session, p: &Value) -> Result<Value> {
})?;
return Ok(json!({"id": ph_id}));
}
let id = add_shape(s, shape, true)?;
let id = add_shape(s, "insert.picture", shape, true)?;
Ok(json!({"id": id}))
}
@@ -389,7 +389,7 @@ fn table(s: &mut Session, p: &Value) -> Result<Value> {
}
}
}
let id = add_shape(s, Shape { xfrm: Some(rect), kind: ShapeKind::Table(t), ..Default::default() }, true)?;
let id = add_shape(s, "insert.table", Shape { xfrm: Some(rect), kind: ShapeKind::Table(t), ..Default::default() }, true)?;
Ok(json!({"id": id}))
}
@@ -446,7 +446,7 @@ fn chart(s: &mut Session, p: &Value) -> Result<Value> {
}
let size = s.doc()?.doc.slide_size;
let rect = rect_param(p, "rect").unwrap_or(Xfrm::new(size.width * 0.15, size.height * 0.15, size.width * 0.7, size.height * 0.7));
let id = add_shape(s, Shape { xfrm: Some(rect), kind: ShapeKind::Chart(Box::new(c)), ..Default::default() }, true)?;
let id = add_shape(s, "insert.chart", Shape { xfrm: Some(rect), kind: ShapeKind::Chart(Box::new(c)), ..Default::default() }, true)?;
Ok(json!({"id": id}))
}
@@ -498,12 +498,15 @@ fn media(s: &mut Session, p: &Value, video: bool) -> Result<Value> {
});
let data = std::sync::Arc::try_unwrap(bytes).unwrap_or_else(|a| (*a).clone());
let (media, poster) = s.edit(|doc, _| {
let media = doc.add_media(&name, ct, data);
let poster = poster_png.map(|png| doc.add_media(&format!("{name}-poster.png"), "image/png", png));
let media = doc.add_media(&name, ct, data).ok_or_else(|| bad("insert.media", NO_ID_SPACE))?;
let poster = match poster_png {
Some(png) => Some(doc.add_media(&format!("{name}-poster.png"), "image/png", png).ok_or_else(|| bad("insert.media", NO_ID_SPACE))?),
None => None,
};
Ok((media, poster))
})?;
let clip = MediaClip { media, video, poster, volume: 1.0, duration_ms, width, height, ..Default::default() };
let id = add_shape(s, Shape { xfrm: Some(rect), kind: ShapeKind::Media(clip), descr: name, ..Default::default() }, true)?;
let id = add_shape(s, "insert.media", Shape { xfrm: Some(rect), kind: ShapeKind::Media(clip), descr: name, ..Default::default() }, true)?;
if let Some(w) = &warning {
s.ui_requests.push(crate::UiRequest::Message { text: w.clone() });
}
@@ -545,7 +548,7 @@ fn word_art(s: &mut Session, p: &Value) -> Result<Value> {
}
}
}
let id = add_shape(s, shape, true)?;
let id = add_shape(s, "insert.wordArt", shape, true)?;
super::text::fit_text_box(s, id)?;
Ok(json!({"id": id}))
}
@@ -624,7 +627,7 @@ fn action_button(s: &mut Session, p: &Value) -> Result<Value> {
click: action.map(|a| Hyperlink { action: a, tooltip: String::new(), highlight_click: true }),
..Default::default()
};
let id = add_shape(s, shape, true)?;
let id = add_shape(s, "insert.actionButton", shape, true)?;
Ok(json!({"id": id}))
}
@@ -790,7 +793,7 @@ fn smart_art(s: &mut Session, p: &Value) -> Result<Value> {
}
}
sh.text = Some(body);
ids.push(add_shape(s, sh, false)?.0);
ids.push(add_shape(s, "insert.smartArt", sh, false)?.0);
}
let g = s.execute("arrange.group", &json!({"ids": ids}))?;
if let Some(gid) = g.get("id").and_then(Value::as_u64) {
crates/engine/src/cmd/media.rs
+1 -1
View File
@@ -139,7 +139,7 @@ fn poster_frame(s: &mut Session, p: &Value) -> Result<Value> {
let data = s.doc()?.doc.media(m.media).map(|i| i.data.clone()).ok_or_else(|| EngineError::Other("the media data is missing".into()))?;
let png = poster_png(&data, ms).ok_or_else(|| EngineError::Other("no frame could be decoded there".into()))?;
let name = format!("poster{id}.png");
let poster = s.edit(|doc, _| Ok(doc.add_media(&name, "image/png", png)))?;
let poster = s.edit(|doc, _| doc.add_media(&name, "image/png", png).ok_or_else(|| bad("media.posterFrame", NO_ID_SPACE)))?;
edit_shapes(s, &json!({"id": id}), "media.posterFrame", |sh| {
if let ShapeKind::Media(m) = &mut sh.kind {
m.poster = Some(poster);
crates/engine/src/cmd/merge.rs
+1 -1
View File
@@ -176,7 +176,7 @@ fn merge(s: &mut Session, p: &Value) -> Result<Value> {
made.push(sh);
}
for sh in &mut made {
sh.id = ShapeId(doc.alloc_id());
sh.id = ShapeId(doc.alloc_id().ok_or_else(|| bad("shape.merge", NO_ID_SPACE))?);
new_ids.push(sh.id);
}
let list = crate::shapes_mut(doc, sel).ok_or_else(|| bad("shape.merge", "no slide"))?;
crates/engine/src/cmd/mod.rs
+4
View File
@@ -152,6 +152,10 @@ pub fn find_command(id: &str) -> Option<&'static CommandSpec> {
// ---------- param helpers ----------
/// The deck has used every object id (a hostile file can arrive that way): it cannot grow, and
/// growing would duplicate ids (#407).
pub(crate) const NO_ID_SPACE: &str = "the presentation has used every object id and can hold no more";
pub(crate) fn bad(cmd: &str, msg: impl Into<String>) -> EngineError {
EngineError::BadParams { cmd: cmd.into(), msg: msg.into() }
}
crates/engine/src/cmd/shape.rs
+2 -2
View File
@@ -284,7 +284,7 @@ fn fill(s: &mut Session, p: &Value) -> Result<Value> {
} else if p.get("picture").is_some() {
let (name, bytes) = super::insert::media_bytes(&with_param(p, "data", p.get("picture").cloned().unwrap_or_default()), "shape.fill")?;
let ct = super::insert::content_type(&name, &bytes);
let media = s.edit(|doc, _| Ok(doc.add_media(&name, ct, bytes)))?;
let media = s.edit(|doc, _| doc.add_media(&name, ct, bytes).ok_or_else(|| bad("shape.fill", NO_ID_SPACE)))?;
Some(Some(Fill::Picture(PictureFill { media, ..Default::default() })))
} else {
None
@@ -674,7 +674,7 @@ fn picture_reset(s: &mut Session, p: &Value) -> Result<Value> {
fn picture_change(s: &mut Session, p: &Value) -> Result<Value> {
let (name, bytes) = super::insert::media_bytes(p, "picture.change")?;
let ct = super::insert::content_type(&name, &bytes);
let media = s.edit(|doc, _| Ok(doc.add_media(&name, ct, bytes)))?;
let media = s.edit(|doc, _| doc.add_media(&name, ct, bytes).ok_or_else(|| bad("picture.change", NO_ID_SPACE)))?;
edit_shapes(s, p, "picture.change", |sh| {
if let deckcraft_model::ShapeKind::Picture { fill } = &mut sh.kind {
fill.media = media;
crates/engine/src/cmd/slide.rs
+4 -4
View File
@@ -99,7 +99,7 @@ fn new(s: &mut Session, p: &Value) -> Result<Value> {
}
}
};
let mut slide = defaults::new_slide(doc, layout);
let mut slide = defaults::new_slide(doc, layout).ok_or_else(|| bad("slide.new", NO_ID_SPACE))?;
if let Some(t) = str_param(p, "title")
&& let Some(sh) = slide.shapes.iter_mut().find(|x| x.ph_type().is_some_and(|k| k.is_title()))
{
@@ -165,8 +165,8 @@ fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
for (k, i) in indices.iter().enumerate() {
let Some(src) = doc.slides.get(*i).map(|x| x.as_ref().clone()) else { continue };
let mut copy = src;
copy.id = SlideId(doc.alloc_id());
super::edit::reid(doc, &mut copy.shapes);
copy.id = SlideId(doc.alloc_id().ok_or_else(|| bad("slide.duplicate", NO_ID_SPACE))?);
super::edit::reid("slide.duplicate", doc, &mut copy.shapes)?;
// Animations follow the new shape ids by position.
let old_ids: Vec<_> = doc
.slides
@@ -364,7 +364,7 @@ pub fn apply_layout(doc: &mut Presentation, index: usize, layout: LayoutId) -> R
doc.layout(layout).map(|(_, l)| l.shapes.clone()).ok_or_else(|| bad("slide.layout", "no such layout"))?;
let mut fresh = vec![];
for ls in lay_shapes.iter().filter(|x| x.ph.as_ref().is_some_and(|p| !p.kind.is_footer_kind())) {
let id = deckcraft_model::ShapeId(doc.alloc_id());
let id = deckcraft_model::ShapeId(doc.alloc_id().ok_or_else(|| bad("slide.layout", NO_ID_SPACE))?);
fresh.push((id, ls.clone()));
}
let slide = doc.slides.get_mut(index).ok_or_else(|| bad("slide.layout", "no slide"))?;
crates/engine/src/tests.rs
+25
View File
@@ -835,3 +835,28 @@ fn slide_ranges_parse_like_powerpoints_print_box() {
let opts = cmd::file::pdf_options(&json!({"slides": [0, 7, 1]}), 2).unwrap();
assert_eq!(opts.slides, Some(vec![0, 1]), "indices past the end are dropped");
}
/// A deck whose ids are used up (a hostile file can arrive with next_id at the top of the
/// range) refuses to grow instead of handing out duplicate ids (#407).
#[test]
fn an_exhausted_id_space_refuses_growth() {
let mut s = session();
s.execute("slide.new", &json!({})).unwrap();
std::sync::Arc::make_mut(&mut s.doc_mut().unwrap().doc).next_id = u32::MAX;
let before = s.doc().unwrap().doc.clone();
assert!(s.execute("slide.new", &json!({})).is_err(), "a new slide is refused");
assert!(s.execute("shape.insert", &json!({"preset": "rect", "rect": [10, 10, 100, 50]})).is_err());
assert!(s.execute("slide.duplicate", &json!({})).is_err());
assert!(s.execute("edit.duplicate", &json!({})).is_err());
// What the deck holds is untouched: same slides, same ids, no duplicates.
let after = s.doc().unwrap().doc.clone();
assert_eq!(after.slides.len(), before.slides.len());
let mut ids = vec![];
deckcraft_model::walk(&after.slides[0].shapes, &mut |sh, _| ids.push(sh.id.0));
ids.push(after.slides[0].id.0);
ids.push(after.next_id);
let mut sorted = ids.clone();
sorted.sort_unstable();
sorted.dedup();
assert_eq!(sorted.len(), ids.len(), "no id repeats");
}
crates/engine/src/tools.rs
+2 -2
View File
@@ -929,7 +929,7 @@ impl Session {
let list = cmd::current_shapes(doc, sel).cloned().unwrap_or_default();
let mut copies: Vec<Shape> = list.iter().filter(|x| ids.contains(&x.id)).cloned().collect();
let boxes: Vec<Xfrm> = copies.iter().map(|c| xfrm_of(doc, sel, c)).collect();
cmd::edit::reid(doc, &mut copies);
cmd::edit::reid("edit.duplicate", doc, &mut copies)?;
for (c, b) in copies.iter_mut().zip(boxes) {
c.xfrm = Some(b);
c.ph = None;
@@ -1049,7 +1049,7 @@ impl Session {
let id = match existing {
Some(id) => id,
None => {
let id = ShapeId(doc.alloc_id());
let id = ShapeId(doc.alloc_id().ok_or_else(|| cmd::bad("ink.draw", cmd::NO_ID_SPACE))?);
let size = doc.slide_size;
let list = crate::shapes_mut(doc, sel).ok_or_else(|| cmd::bad("ink", "no slide"))?;
list.push(Shape {