mirror of
https://github.com/storytold/photocraft.git
synced 2026-11-10 20:32:31 +00:00
Reject geometry params that would silently wrap through i64->i32 narrowing (#803)
`commands::int` returns an unbounded i64, and the geometry call sites narrowed it with `as i32`, which truncates by multiples of 2^32: layer.translate moved a layer 50 px for `dx = 2^32 + 50` and to a large negative offset for `dx = 3e9`; select.rect and document.pixel read unrelated coordinates; image.crop cropped a rectangle unrelated to the numbers passed; artboard rects landed elsewhere. No panic, no Err - the document was silently mutated and then autosaved, and the behaviour is identical in debug and release, so panic_hunt cannot see it either. - new `commands::int_i32(cmd, p, key)` narrows with a bad-params error instead of a wrapping cast; select.rect, document.pixel, layer.translate, image.crop and the artboard rect keys go through it - image.crop builds its rectangle with saturating_add (x + w overflow) - edit.contentAwareFill's `area` parser returns a bad-params error for malformed arrays (wrong length, non-numbers, non-finite) and saturating arithmetic for huge ones - `[1e30, 0, 1e30, 10]` overflowed the i32 additions before (a debug-build panic, a garbage sampling window in release) - filters' integer helper clamps out-of-range values like the neighbouring filter params instead of wrapping (`3e9` came back as a negative offset)
This commit is contained in:
1 parent
9d8a78a17e
commit
ec64eb773c
8 files changed
+118
-17
No files matched your search
@@ -13,7 +13,7 @@ use photocraft_geom::Rect;
|
||||
use photocraft_raster::Surface;
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use crate::commands::{CommandSpec, color_param, int};
|
||||
use crate::commands::{CommandSpec, color_param, int, int_i32};
|
||||
use crate::file_cmds::{SaveOpts, encode, f64_param, join, native_doc, sanitize, save_doc, stem, str_param, write_file};
|
||||
use crate::{EngineError, Result, Session};
|
||||
|
||||
@@ -109,19 +109,22 @@ fn background_param(p: &Value, cmd: &str) -> Result<Option<ArtboardBackground>>
|
||||
/// `rect` as `[x, y, w, h]` or `x`/`y`/`width`/`height` keys over `base`.
|
||||
fn rect_param(p: &Value, base: Rect, cmd: &str) -> Result<Rect> {
|
||||
if let Some(Value::Array(a)) = p.get("rect") {
|
||||
let v: Vec<i32> = a.iter().filter_map(|x| x.as_f64().map(|f| f.round() as i32)).collect();
|
||||
let v: Vec<i32> = a.iter().filter_map(|x| x.as_f64().filter(|f| f.is_finite()).map(|f| f.round() as i32)).collect();
|
||||
if v.len() != 4 || v[2] <= 0 || v[3] <= 0 {
|
||||
return Err(bad(cmd, "\"rect\" must be [x, y, width, height] with a positive size"));
|
||||
}
|
||||
return Ok(Rect::from_xywh(v[0], v[1], v[2] as u32, v[3] as u32));
|
||||
}
|
||||
let x = int(p, "x").map_or(base.x0, |v| v as i32);
|
||||
let y = int(p, "y").map_or(base.y0, |v| v as i32);
|
||||
let x = int_i32(cmd, p, "x")?.unwrap_or(base.x0);
|
||||
let y = int_i32(cmd, p, "y")?.unwrap_or(base.y0);
|
||||
let w = int(p, "width").unwrap_or(i64::from(base.width()));
|
||||
let h = int(p, "height").unwrap_or(i64::from(base.height()));
|
||||
if w <= 0 || h <= 0 {
|
||||
return Err(bad(cmd, "the artboard size must be positive"));
|
||||
}
|
||||
if w > i32::MAX as i64 || h > i32::MAX as i64 {
|
||||
return Err(bad(cmd, "the artboard size is too large for a 32-bit canvas"));
|
||||
}
|
||||
Ok(Rect::from_xywh(x, y, w as u32, h as u32))
|
||||
}
|
||||
|
||||
|
||||
@@ -121,6 +121,17 @@ pub(crate) fn int(p: &Value, key: &str) -> Option<i64> {
|
||||
p.get(key).and_then(|v| v.as_i64().or_else(|| v.as_f64().filter(|f| f.is_finite()).map(|f| f.round() as i64)))
|
||||
}
|
||||
|
||||
/// [`int`] narrowed to `i32` for geometry: `None` when absent, and a bad-params error when the
|
||||
/// value would wrap through `as i32` — out-of-range integers truncate by multiples of 2^32,
|
||||
/// which silently relocates geometry (`dx = 2^32 + 50` moves 50 px, `3e9` goes negative)
|
||||
/// instead of erroring.
|
||||
pub(crate) fn int_i32(cmd: &str, p: &Value, key: &str) -> Result<Option<i32>> {
|
||||
match int(p, key) {
|
||||
None => Ok(None),
|
||||
Some(v) => i32::try_from(v).map(Some).map_err(|_| bad(cmd, format!("`{key}` = {v} is outside the 32-bit coordinate range"))),
|
||||
}
|
||||
}
|
||||
|
||||
fn f32_or(p: &Value, key: &str, default: f32) -> f32 {
|
||||
p.get(key).and_then(Value::as_f64).map(|v| v as f32).unwrap_or(default)
|
||||
}
|
||||
@@ -324,8 +335,8 @@ fn build() -> Vec<CommandSpec> {
|
||||
r##"{"x":i32,"y":i32,"width":u32,"height":u32,"mode":"replace|add|subtract|intersect"="replace","ellipse":bool=false,"antiAlias":bool=true,"feather":px=0}"##,
|
||||
has_doc,
|
||||
|s, p| {
|
||||
let get = |k: &str| int(p, k).ok_or_else(|| bad("select.rect", format!("missing `{k}`")));
|
||||
let r = Rect::from_xywh(get("x")? as i32, get("y")? as i32, get("width")?.max(0) as u32, get("height")?.max(0) as u32);
|
||||
let get = |k: &str| int_i32("select.rect", p, k).and_then(|v| v.ok_or_else(|| bad("select.rect", format!("missing `{k}`"))));
|
||||
let r = Rect::from_xywh(get("x")?, get("y")?, get("width")?.max(0) as u32, get("height")?.max(0) as u32);
|
||||
let mode = p.get("mode").and_then(Value::as_str).unwrap_or("replace").to_string();
|
||||
let ellipse = p.get("ellipse").and_then(Value::as_bool).unwrap_or(false);
|
||||
// Options bar: anti-aliased ellipse edges (4x4 supersampled) and Feather (applied to the new shape only).
|
||||
|
||||
@@ -303,10 +303,20 @@ fn channel_mask<'a>(doc: &'a Document, v: &Value) -> Option<&'a Surface> {
|
||||
}
|
||||
}
|
||||
|
||||
fn rect_param(p: &Value, key: &str) -> Option<Rect> {
|
||||
let a = p.get(key)?.as_array()?;
|
||||
let n = |i: usize| a.get(i).and_then(Value::as_f64).map(|v| v.round() as i32);
|
||||
Some(Rect::new(n(0)?, n(1)?, n(0)? + n(2)?.max(0), n(1)? + n(3)?.max(0)))
|
||||
/// `key` as an `[x, y, w, h]` rectangle. The saturating float→int casts bound each component;
|
||||
/// the additions saturate too — `area: [1e30, 0, 1e30, 10]` is a whole-canvas window, not an
|
||||
/// overflow (it panicked in debug builds before the `saturating_add`).
|
||||
fn rect_param(p: &Value, key: &str, cmd: &str) -> Result<Rect> {
|
||||
let a = p.get(key).and_then(Value::as_array).ok_or_else(|| bad(cmd, format!("`{key}` must be [x, y, w, h]")))?;
|
||||
let n = |i: usize| {
|
||||
a.get(i)
|
||||
.and_then(Value::as_f64)
|
||||
.filter(|f| f.is_finite())
|
||||
.map(|v| v.round() as i32)
|
||||
.ok_or_else(|| bad(cmd, format!("`{key}` must be four finite numbers")))
|
||||
};
|
||||
let (x, y, w, h) = (n(0)?, n(1)?, n(2)?, n(3)?);
|
||||
Ok(Rect::new(x, y, x.saturating_add(w.max(0)), y.saturating_add(h.max(0))))
|
||||
}
|
||||
|
||||
fn content_aware_fill(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
@@ -323,7 +333,7 @@ fn content_aware_fill(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
let ext = hb.width().max(hb.height()) as i32;
|
||||
let sampling = str_or(p, "sampling", "auto").to_string();
|
||||
let custom_mask: Option<Surface> = p.get("channel").and_then(|v| channel_mask(&doc, v)).cloned();
|
||||
let custom_rect = rect_param(p, "area");
|
||||
let custom_rect = if p.get("area").is_some() { Some(rect_param(p, "area", cmd)?) } else { None };
|
||||
let window = match sampling.as_str() {
|
||||
"auto" => hb.inflate((ext * 3 / 4).max(32)),
|
||||
"rectangular" => hb.inflate(int(p, "margin").map_or(ext.max(16), |m| m.clamp(0, 100_000) as i32)),
|
||||
|
||||
@@ -101,6 +101,21 @@ fn blob_session(depth: u32) -> Session {
|
||||
s
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn content_aware_fill_huge_area_neither_panics_nor_wraps() {
|
||||
let mut s = blob_session(8);
|
||||
// `[1e30, 0, 1e30, 10]` overflowed the i32 additions in the area parser (a debug-build
|
||||
// panic, a garbage sampling window in release); the additions saturate now, so this is
|
||||
// simply a whole-canvas window.
|
||||
let r = s.execute("edit.contentAwareFill", json!({"sampling": "custom", "area": [1e30, 0.0, 1e30, 10.0], "colorAdaptation": "none"})).unwrap();
|
||||
assert!(r["filled"].as_u64().unwrap() > 100);
|
||||
// A malformed area names the problem instead of silently falling back.
|
||||
for area in [json!([0.0, 0.0, null, 10.0]), json!([0.0, 0.0, 10.0]), json!("nope")] {
|
||||
let err = s.execute("edit.contentAwareFill", json!({"sampling": "custom", "area": area})).unwrap_err();
|
||||
assert!(err.to_string().contains("`area`"), "{err}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn content_aware_fill_removes_object_at_all_depths() {
|
||||
for depth in [8, 16, 32] {
|
||||
|
||||
@@ -15,7 +15,9 @@ fn f(p: &Value, k: &str, d: f32) -> f32 {
|
||||
p.get(k).and_then(Value::as_f64).map_or(d, |v| v as f32)
|
||||
}
|
||||
fn i(p: &Value, k: &str, d: i32) -> i32 {
|
||||
p.get(k).and_then(|v| v.as_i64().or_else(|| v.as_f64().map(|f| f.round() as i64))).map_or(d, |v| v as i32)
|
||||
// Clamped like the neighbouring filter params: an out-of-range value saturates instead of
|
||||
// wrapping through `as i32` (`3e9` used to come back as a negative offset).
|
||||
p.get(k).and_then(|v| v.as_i64().or_else(|| v.as_f64().map(|f| f.round() as i64))).map_or(d, |v| v.clamp(i64::from(i32::MIN), i64::from(i32::MAX)) as i32)
|
||||
}
|
||||
fn b(p: &Value, k: &str, d: bool) -> bool {
|
||||
p.get(k).and_then(Value::as_bool).unwrap_or(d)
|
||||
@@ -498,6 +500,17 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn huge_offsets_clamp_instead_of_wrapping() {
|
||||
// 3e9 wrapped to a negative offset through `as i32`; it saturates now, so it
|
||||
// equals the maximal in-range offset instead of shifting the other way.
|
||||
let mut a = session();
|
||||
a.execute("filter.other.offset", json!({"horizontal": 3_000_000_000_i64, "vertical": 0})).unwrap();
|
||||
let mut b = session();
|
||||
b.execute("filter.other.offset", json!({"horizontal": 2_147_483_647_i64, "vertical": 0})).unwrap();
|
||||
assert_eq!(active_pixels(&a), active_pixels(&b));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn last_filter_repeats_with_same_params() {
|
||||
let mut s = session();
|
||||
|
||||
@@ -250,8 +250,15 @@ fn canvas_size(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
/// Image → Crop (to the selection bounds).
|
||||
fn crop(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
let delete = p.get("deleteCroppedPixels").and_then(Value::as_bool).unwrap_or(true);
|
||||
let explicit = match (crate::commands::int(p, "x"), crate::commands::int(p, "y"), crate::commands::int(p, "width"), crate::commands::int(p, "height")) {
|
||||
(Some(x), Some(y), Some(w), Some(h)) if w > 0 && h > 0 => Some(Rect::new(x as i32, y as i32, (x + w) as i32, (y + h) as i32)),
|
||||
// Values beyond i32 would wrap through the narrowing casts into a rectangle unrelated to
|
||||
// the numbers passed; reject them (see `commands::int_i32`).
|
||||
let explicit = match (
|
||||
crate::commands::int_i32("image.crop", p, "x")?,
|
||||
crate::commands::int_i32("image.crop", p, "y")?,
|
||||
crate::commands::int_i32("image.crop", p, "width")?,
|
||||
crate::commands::int_i32("image.crop", p, "height")?,
|
||||
) {
|
||||
(Some(x), Some(y), Some(w), Some(h)) if w > 0 && h > 0 => Some(Rect::new(x, y, x.saturating_add(w), y.saturating_add(h))),
|
||||
_ => None,
|
||||
};
|
||||
// An explicit rectangle (the Crop tool) may extend past the canvas; a selection crop is clamped.
|
||||
@@ -434,6 +441,17 @@ pub fn specs() -> Vec<CommandSpec> {
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn crop_rejects_rectangles_that_would_wrap() {
|
||||
let mut s = session();
|
||||
// 2^32 + 100 wrapped to `x = 100` through the narrowing casts: a crop rectangle
|
||||
// somewhere unrelated to the numbers passed.
|
||||
let err = s.execute("image.crop", json!({"x": 4_294_967_396_i64, "y": 0, "width": 10, "height": 10})).unwrap_err();
|
||||
assert!(err.to_string().contains("32-bit"), "{err}");
|
||||
// In-range rectangles past the canvas still work (clamped by the crop itself).
|
||||
s.execute("image.crop", json!({"x": -10, "y": -10, "width": 1000, "height": 1000})).unwrap();
|
||||
}
|
||||
|
||||
fn session() -> Session {
|
||||
let mut s = Session::new();
|
||||
s.execute("file.new", json!({"width": 40, "height": 20})).unwrap();
|
||||
|
||||
@@ -10,7 +10,7 @@ use photocraft_doc::{Document, Layer, LayerContent, LayerId};
|
||||
use photocraft_geom::Rect;
|
||||
use serde_json::{Value, json};
|
||||
|
||||
use crate::commands::{CommandSpec, int};
|
||||
use crate::commands::{CommandSpec, int_i32};
|
||||
use crate::{DocState, EngineError, Result, Session};
|
||||
|
||||
// ---------- selection state ----------
|
||||
@@ -237,8 +237,8 @@ pub(crate) fn move_layers(doc: &mut Document, moves: &[(LayerId, i32, i32)]) ->
|
||||
|
||||
/// `layer.translate`: the explicit layer, or every selected layer, plus their linked layers.
|
||||
pub fn translate(s: &mut Session, p: &Value) -> Result<Value> {
|
||||
let dx = int(p, "dx").unwrap_or(0) as i32;
|
||||
let dy = int(p, "dy").unwrap_or(0) as i32;
|
||||
let dx = int_i32("layer.translate", p, "dx")?.unwrap_or(0);
|
||||
let dy = int_i32("layer.translate", p, "dy")?.unwrap_or(0);
|
||||
if dx == 0 && dy == 0 {
|
||||
return Ok(Value::Null);
|
||||
}
|
||||
@@ -1204,6 +1204,21 @@ mod tests {
|
||||
assert_eq!(sel(&s), before);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn translate_rejects_offsets_that_would_wrap() {
|
||||
let mut s = session(8);
|
||||
let a = rect_layer(&mut s, Rect::new(0, 0, 5, 5));
|
||||
select_all(&mut s, &[a]);
|
||||
// 2^32 + 50 wrapped to `dx = 50` and 3e9 to a negative offset through `as i32`.
|
||||
for dx in [4_294_967_346_i64, 3_000_000_000_i64] {
|
||||
let err = s.execute("layer.translate", json!({"dx": dx, "dy": 0})).unwrap_err();
|
||||
assert!(err.to_string().contains("32-bit"), "{err}");
|
||||
}
|
||||
assert_eq!(bounds(&s, a), Rect::new(0, 0, 5, 5), "the layer never moved");
|
||||
// Large in-range offsets still work.
|
||||
s.execute("layer.translate", json!({"dx": -200_000, "dy": 200_000})).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn linked_layers_move_together_and_undo() {
|
||||
for depth in [8, 16, 32] {
|
||||
|
||||
@@ -531,6 +531,22 @@ mod tests {
|
||||
assert!(s.active().unwrap().doc.selection.is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn geometry_params_that_would_wrap_are_rejected() {
|
||||
let mut s = session();
|
||||
// 2^32 + 50 wrapped to `x = 50` and 3e9 to a negative coordinate through `as i32`;
|
||||
// both are bad-params errors now, whatever the front door (UI, CLI, control, MCP).
|
||||
for x in [4_294_967_346_i64, 3_000_000_000_i64] {
|
||||
let err = s.execute("select.rect", json!({"x": x, "y": 0, "width": 10, "height": 10})).unwrap_err();
|
||||
assert!(err.to_string().contains("32-bit"), "{err}");
|
||||
}
|
||||
let err = s.execute("document.pixel", json!({"x": 4_294_967_346_i64, "y": 0})).unwrap_err();
|
||||
assert!(err.to_string().contains("32 bits"), "{err}");
|
||||
// In-range coordinates, including negative and past-canvas ones, are unaffected.
|
||||
s.execute("select.rect", json!({"x": -100, "y": -100, "width": 500, "height": 500})).unwrap();
|
||||
s.execute("document.pixel", json!({"x": 0, "y": 0})).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn grow_and_similar() {
|
||||
let mut s = session();
|
||||
|
||||
Reference in new issue
Block a user