From c85f7425a412a41db04e3c9cd1807faa1dfb6d67 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 4 Nov 2026 04:35:43 +0000 Subject: [PATCH] address review: build unvar value only when an instance exists; check interpolation length before copying; bound the limit-error preview --- crates/engine/src/cmd/variables.rs | 11 ++++--- crates/engine/src/script.rs | 51 ++++++++++++++++++------------ 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/cmd/variables.rs b/crates/engine/src/cmd/variables.rs index b1c5ff59b..28fd6d37a 100644 --- a/crates/engine/src/cmd/variables.rs +++ b/crates/engine/src/cmd/variables.rs @@ -369,10 +369,13 @@ fn unvar(st: &mut designcraft_doc::Story, i: usize, n: usize, value: &str, depth at }; let target = var_char(i).unwrap_or(' ').to_string(); - let value = var_char(i).map_or_else(|| value.to_string(), |c| value.replace(c, "")); - for pos in instances(&st.text, &target) { - st.replace(pos..pos + target.len(), &value); - on_replace(pos, target.len()); + let at = instances(&st.text, &target); + if !at.is_empty() { + let value = var_char(i).map_or_else(|| value.to_string(), |c| value.replace(c, "")); + for pos in at { + st.replace(pos..pos + target.len(), &value); + on_replace(pos, target.len()); + } } for k in i + 1..n { let (from, to) = (var_char(k).unwrap_or(' ').to_string(), var_char(k - 1).unwrap_or(' ').to_string()); diff --git a/crates/engine/src/script.rs b/crates/engine/src/script.rs index de2589d9f..52efddf78 100644 --- a/crates/engine/src/script.rs +++ b/crates/engine/src/script.rs @@ -75,8 +75,8 @@ fn step_of(v: &Value) -> Result { Ok(Step { command, params }) } -/// The value a reference (`N.path`, `last.path`) points at. -fn lookup(r: &str, results: &[Value]) -> Result { +/// The value a reference (`N.path`, `last.path`) points at, borrowed from `results`. +fn lookup<'a>(r: &str, results: &'a [Value]) -> Result<&'a Value, String> { let mut parts = r.split('.'); let head = parts.next().unwrap_or(""); let idx = if head == "last" { @@ -84,14 +84,14 @@ fn lookup(r: &str, results: &[Value]) -> Result { } else { head.parse::().map_err(|_| format!("bad reference `${r}`"))? }; - let mut v = results.get(idx).ok_or_else(|| format!("`${r}`: step {idx} hasn't run (only {} have)", results.len()))?.clone(); + let mut v = results.get(idx).ok_or_else(|| format!("`${r}`: step {idx} hasn't run (only {} have)", results.len()))?; for p in parts { - v = match (&v, p.parse::()) { - (Value::Array(a), Ok(i)) => a.get(i).cloned(), - (Value::Object(o), _) => o.get(p).cloned(), + v = match (v, p.parse::()) { + (Value::Array(a), Ok(i)) => a.get(i), + (Value::Object(o), _) => o.get(p), _ => None, } - .ok_or_else(|| format!("`${r}`: no `{p}` in {}", short(&v)))?; + .ok_or_else(|| format!("`${r}`: no `{p}` in {}", short(v)))?; } Ok(v) } @@ -114,12 +114,18 @@ fn is_reference(r: &str) -> bool { /// story, then interpolates that story into the next, could otherwise double its size each step. pub const MAX_INTERPOLATED_BYTES: usize = 16 << 20; +fn too_long(s: &str) -> String { + // Cut at a char boundary: a preview of a long Unicode input mustn't copy all of it. + let preview = &s[..s.floor_char_boundary(40)]; + format!("the interpolated text of {preview} is longer than {MAX_INTERPOLATED_BYTES} bytes").replace('\n', " ") +} + /// Replace references in `params` with values from earlier `results`. pub fn resolve(params: &Value, results: &[Value]) -> Result { Ok(match params { Value::String(s) => { if let Some(r) = s.strip_prefix('$').filter(|r| is_reference(r)) { - return lookup(r, results); + return lookup(r, results).cloned(); } if !s.contains("${") && !s.contains("$$") { return Ok(params.clone()); @@ -134,26 +140,25 @@ pub fn resolve(params: &Value, results: &[Value]) -> Result { rest = tail; } else if let Some(inner) = after.strip_prefix('{') { let end = inner.find('}').ok_or_else(|| format!("unclosed `${{` in {s:?}"))?; - match lookup(&inner[..end], results)? { - Value::String(t) => out.push_str(&t), - other => out.push_str(&other.to_string()), - } - if out.len() > MAX_INTERPOLATED_BYTES { - return Err(format!("the interpolated text of {} is longer than {MAX_INTERPOLATED_BYTES} bytes", s.get(..40).unwrap_or(s)) - .replace('\n', " ")); + // The length is checked before the value is copied in. + let piece = match lookup(&inner[..end], results)? { + Value::String(t) => std::borrow::Cow::Borrowed(t.as_str()), + other => std::borrow::Cow::Owned(other.to_string()), + }; + if out.len().saturating_add(piece.len()) > MAX_INTERPOLATED_BYTES { + return Err(too_long(s)); } + out.push_str(&piece); rest = &inner[end + 1..]; } else { out.push('$'); rest = after; } } - out.push_str(rest); - if out.len() > MAX_INTERPOLATED_BYTES { - return Err( - format!("the interpolated text of {} is longer than {MAX_INTERPOLATED_BYTES} bytes", s.get(..40).unwrap_or(s)).replace('\n', " ") - ); + if out.len().saturating_add(rest.len()) > MAX_INTERPOLATED_BYTES { + return Err(too_long(s)); } + out.push_str(rest); Value::String(out) } Value::Array(a) => Value::Array(a.iter().map(|v| resolve(v, results)).collect::>()?), @@ -227,6 +232,12 @@ impl crate::Session { #[cfg(test)] mod tests { + #[test] + fn the_limit_error_previews_a_multibyte_input_in_bounds() { + let e = super::too_long(&"€".repeat(1000)); + assert!(e.len() < 150, "{e}"); + } + use super::*; #[test]