From df75976882dd1ade585cbd76fbd1aad8b055ffec Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 7 Oct 2026 11:52:22 +0000 Subject: [PATCH] fix(arith): assign to array elements in ((...)), let and $((...)) ((a[i]++)), ((c[$k]+=n)), ((a[2]=5)), let a[1]++ now update the element instead of losing the write. Indexed subscripts are arithmetic (negative allowed); associative keys are literal with $name expanded and quotes dropped. Writes stay on the budgeted array writers. Claude-Session: https://claude.ai/code/session_019aFikmptPc91Fj4N2iDXQA --- crates/bashkit/src/interpreter/arithmetic.rs | 177 +++++++++++++++--- crates/bashkit/src/interpreter/mod.rs | 83 ++++---- crates/bashkit/src/parser/mod.rs | 4 +- .../tests/integration/threat_model_tests.rs | 21 +++ .../bash/arith-array-lvalue.test.sh | 85 +++++++++ knowledge/log.md | 1 + 6 files changed, 304 insertions(+), 67 deletions(-) create mode 100644 crates/bashkit/tests/spec_cases/bash/arith-array-lvalue.test.sh diff --git a/crates/bashkit/src/interpreter/arithmetic.rs b/crates/bashkit/src/interpreter/arithmetic.rs index 700d2bb24..90f500300 100644 --- a/crates/bashkit/src/interpreter/arithmetic.rs +++ b/crates/bashkit/src/interpreter/arithmetic.rs @@ -37,26 +37,18 @@ impl Interpreter { // Handle pre-increment/pre-decrement: ++var, --var if let Some(var_name) = expr.strip_prefix("++") { let var_name = var_name.trim(); - if is_valid_var_name(var_name) { + if is_arith_lvalue(var_name) { // THREAT[TM-DOS-043]: Bash integer side effects wrap at i64 bounds. - let val = self - .expand_variable(var_name) - .parse::() - .unwrap_or(0) - .wrapping_add(1); - self.set_variable(var_name.to_string(), val.to_string()); + let val = self.arith_lvalue_value(var_name).wrapping_add(1); + self.set_arith_lvalue(var_name, val.to_string()); return val; } } if let Some(var_name) = expr.strip_prefix("--") { let var_name = var_name.trim(); - if is_valid_var_name(var_name) { - let val = self - .expand_variable(var_name) - .parse::() - .unwrap_or(0) - .wrapping_sub(1); - self.set_variable(var_name.to_string(), val.to_string()); + if is_arith_lvalue(var_name) { + let val = self.arith_lvalue_value(var_name).wrapping_sub(1); + self.set_arith_lvalue(var_name, val.to_string()); return val; } } @@ -64,17 +56,17 @@ impl Interpreter { // Handle post-increment/post-decrement: var++, var-- if let Some(var_name) = expr.strip_suffix("++") { let var_name = var_name.trim(); - if is_valid_var_name(var_name) { - let old_val = self.expand_variable(var_name).parse::().unwrap_or(0); - self.set_variable(var_name.to_string(), old_val.wrapping_add(1).to_string()); + if is_arith_lvalue(var_name) { + let old_val = self.arith_lvalue_value(var_name); + self.set_arith_lvalue(var_name, old_val.wrapping_add(1).to_string()); return old_val; } } if let Some(var_name) = expr.strip_suffix("--") { let var_name = var_name.trim(); - if is_valid_var_name(var_name) { - let old_val = self.expand_variable(var_name).parse::().unwrap_or(0); - self.set_variable(var_name.to_string(), old_val.wrapping_sub(1).to_string()); + if is_arith_lvalue(var_name) { + let old_val = self.arith_lvalue_value(var_name); + self.set_arith_lvalue(var_name, old_val.wrapping_sub(1).to_string()); return old_val; } } @@ -113,13 +105,13 @@ impl Interpreter { ("", "") }; - if is_valid_var_name(var_name) { + if is_arith_lvalue(var_name) { let rhs = &expr[eq_pos + 1..]; let rhs_val = self.evaluate_arithmetic(rhs); let value = if op.is_empty() { rhs_val } else { - let lhs_val = self.expand_variable(var_name).parse::().unwrap_or(0); + let lhs_val = self.arith_lvalue_value(var_name); // THREAT[TM-DOS-043]: wrapping to prevent overflow panic match op { "+" => lhs_val.wrapping_add(rhs_val), @@ -147,7 +139,7 @@ impl Interpreter { _ => rhs_val, } }; - self.set_variable(var_name.to_string(), value.to_string()); + self.set_arith_lvalue(var_name, value.to_string()); return value; } } @@ -156,6 +148,77 @@ impl Interpreter { self.evaluate_arithmetic(expr) } + /// Current integer value of an arithmetic lvalue (`name` or `name[sub]`). + pub(super) fn arith_lvalue_value(&self, lvalue: &str) -> i64 { + let Some(bracket) = lvalue.find('[') else { + return self.evaluate_arithmetic(lvalue); + }; + let resolved = self.resolve_nameref(&lvalue[..bracket]); + let value = match self.scoped.assoc_arrays.get(resolved) { + Some(arr) => { + let key = self.arith_assoc_key(&lvalue[bracket + 1..lvalue.len() - 1]); + arr.get(&key).cloned().unwrap_or_default() + } + None => self.expand_name_or_array_element(&unquote_arith_subscript(lvalue)), + }; + let value = value.trim(); + value + .parse::() + .unwrap_or_else(|_| self.evaluate_arithmetic(value)) + } + + /// Store an arithmetic result into `name` or `name[sub]`. Array elements + /// go through the same budgeted writer as `${a[i]:=v}`. + pub(super) fn set_arith_lvalue(&mut self, lvalue: &str, value: String) { + if let Some(bracket) = lvalue.find('[') { + let resolved = self.resolve_nameref(&lvalue[..bracket]).to_string(); + if self.scoped.assoc_arrays.contains_key(&resolved) { + let key = self.arith_assoc_key(&lvalue[bracket + 1..lvalue.len() - 1]); + self.set_assoc_element_checked(resolved, key, value); + } else { + let target = unquote_arith_subscript(lvalue); + self.set_parameter_expansion_target(&target, value); + } + } else { + self.set_variable(lvalue.to_string(), value); + } + } + + /// Associative key inside `((...))`: quotes dropped, `$name`/`${name}` + /// expanded once (`c[k$i]` -> `k0`), everything else literal. + fn arith_assoc_key(&self, raw: &str) -> String { + let raw = strip_subscript_quotes(raw); + if !raw.contains('$') { + return raw.to_string(); + } + let mut out = String::new(); + let mut rest = raw; + while let Some(pos) = rest.find('$') { + out.push_str(&rest[..pos]); + let after = &rest[pos + 1..]; + let (name, consumed) = if let Some(inner) = after.strip_prefix('{') { + match inner.find('}') { + Some(end) => (&inner[..end], end + 2), + None => ("", 0), + } + } else { + let end = after + .find(|c: char| !c.is_ascii_alphanumeric() && c != '_') + .unwrap_or(after.len()); + (&after[..end], end) + }; + if name.is_empty() { + out.push('$'); + rest = after; + } else { + out.push_str(&self.expand_variable(name)); + rest = &after[consumed..]; + } + } + out.push_str(rest); + out + } + /// Evaluate a simple arithmetic expression pub(super) fn evaluate_arithmetic(&self, expr: &str) -> i64 { self.evaluate_arithmetic_depth(expr, 0) @@ -542,15 +605,14 @@ impl Interpreter { { let arr_name = &name[..bracket]; let resolved = self.resolve_nameref(arr_name); - let idx_str = &name[bracket + 1..name.len() - 1]; + let idx_str = strip_subscript_quotes(&name[bracket + 1..name.len() - 1]); if let Some(arr) = self.scoped.assoc_arrays.get(resolved) { let key = self.expand_variable_or_literal(idx_str); return arr.get(&key).cloned().unwrap_or_default(); } if let Some(arr) = self.scoped.arrays.get(resolved) { - let idx_val = self.evaluate_arithmetic(idx_str); - let idx_usize: usize = idx_val.try_into().unwrap_or(0); - return arr.get(&idx_usize).cloned().unwrap_or_default(); + let idx = self.resolve_indexed_array_subscript(resolved, idx_str); + return arr.get(&idx).cloned().unwrap_or_default(); } return String::new(); } @@ -1002,3 +1064,64 @@ impl Interpreter { result } } + +/// `name` or `name[subscript]` — a target `((...))`/`let` may assign to. +pub(super) fn is_arith_lvalue(s: &str) -> bool { + match s.find('[') { + Some(bracket) if s.ends_with(']') => { + let sub = &s[bracket + 1..s.len() - 1]; + is_valid_var_name(&s[..bracket]) + && !sub.trim().is_empty() + && sub.matches('[').count() == sub.matches(']').count() + } + _ => is_valid_var_name(s), + } +} + +/// Drop one layer of matching quotes around a subscript: `c["x y"]` -> key `x y`. +fn strip_subscript_quotes(sub: &str) -> &str { + let t = sub.trim(); + if t.len() >= 2 + && ((t.starts_with('"') && t.ends_with('"')) || (t.starts_with('\'') && t.ends_with('\''))) + { + &t[1..t.len() - 1] + } else { + sub + } +} + +/// Rebuild `name[sub]` with the subscript unquoted for the array writer. +fn unquote_arith_subscript(lvalue: &str) -> String { + match lvalue.find('[') { + Some(bracket) => format!( + "{}[{}]", + &lvalue[..bracket], + strip_subscript_quotes(&lvalue[bracket + 1..lvalue.len() - 1]) + ), + None => lvalue.to_string(), + } +} + +/// Byte length of the leading `name` or `name[...]` in `s` (0 when none). +pub(super) fn arith_lvalue_end(s: &str) -> usize { + let name_end = s + .find(|c: char| !c.is_ascii_alphanumeric() && c != '_') + .unwrap_or(s.len()); + if name_end == 0 || !s[name_end..].starts_with('[') { + return name_end; + } + let mut depth = 0usize; + for (i, c) in s[name_end..].char_indices() { + match c { + '[' => depth += 1, + ']' => { + depth -= 1; + if depth == 0 { + return name_end + i + 1; + } + } + _ => {} + } + } + name_end +} diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index e9ee31c64..25e14f422 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -11,6 +11,7 @@ #![allow(clippy::unwrap_used)] mod arithmetic; +use arithmetic::{arith_lvalue_end, is_arith_lvalue}; mod brace_expansion; mod expansion; mod glob; @@ -4398,7 +4399,7 @@ impl Interpreter { let rhs_value = self.execute_arithmetic_with_side_effects(effective_rhs); let final_value = if let Some(op) = op { - let current = self.evaluate_arithmetic(var_name); + let current = self.arith_lvalue_value(var_name); // THREAT[TM-DOS-043]: wrapping to prevent overflow panic match op { '+' => current.wrapping_add(rhs_value), @@ -4424,7 +4425,10 @@ impl Interpreter { rhs_value }; - self.set_variable(var_name.to_string(), final_value.to_string()); + if !is_arith_lvalue(var_name) { + return self.evaluate_arithmetic(expr); + } + self.set_arith_lvalue(var_name, final_value.to_string()); return final_value; } } @@ -4433,15 +4437,13 @@ impl Interpreter { if let Some(stripped) = expr.strip_prefix("++") { let trimmed = stripped.trim_start(); // Extract the variable name (leading identifier chars) - let var_end = trimmed - .find(|c: char| !c.is_ascii_alphanumeric() && c != '_') - .unwrap_or(trimmed.len()); + let var_end = arith_lvalue_end(trimmed); let var_name = &trimmed[..var_end]; - if !var_name.is_empty() && is_valid_var_name(var_name) { - let current = self.evaluate_arithmetic(var_name); + if !var_name.is_empty() && is_arith_lvalue(var_name) { + let current = self.arith_lvalue_value(var_name); // THREAT[TM-DOS-043]: Bash integer side effects wrap at i64 bounds. let new_value = current.wrapping_add(1); - self.set_variable(var_name.to_string(), new_value.to_string()); + self.set_arith_lvalue(var_name, new_value.to_string()); let rest = trimmed[var_end..].trim(); if rest.is_empty() { return new_value; @@ -4454,14 +4456,12 @@ impl Interpreter { } if let Some(stripped) = expr.strip_prefix("--") { let trimmed = stripped.trim_start(); - let var_end = trimmed - .find(|c: char| !c.is_ascii_alphanumeric() && c != '_') - .unwrap_or(trimmed.len()); + let var_end = arith_lvalue_end(trimmed); let var_name = &trimmed[..var_end]; - if !var_name.is_empty() && is_valid_var_name(var_name) { - let current = self.evaluate_arithmetic(var_name); + if !var_name.is_empty() && is_arith_lvalue(var_name) { + let current = self.arith_lvalue_value(var_name); let new_value = current.wrapping_sub(1); - self.set_variable(var_name.to_string(), new_value.to_string()); + self.set_arith_lvalue(var_name, new_value.to_string()); let rest = trimmed[var_end..].trim(); if rest.is_empty() { return new_value; @@ -4474,19 +4474,19 @@ impl Interpreter { // Handle post-increment/decrement: var++ or var-- if let Some(stripped) = expr.strip_suffix("++") { let var_name = stripped.trim(); - if is_valid_var_name(var_name) { - let current = self.evaluate_arithmetic(var_name); + if is_arith_lvalue(var_name) { + let current = self.arith_lvalue_value(var_name); let new_value = current.wrapping_add(1); - self.set_variable(var_name.to_string(), new_value.to_string()); + self.set_arith_lvalue(var_name, new_value.to_string()); return current; // Return old value for post-increment } } if let Some(stripped) = expr.strip_suffix("--") { let var_name = stripped.trim(); - if is_valid_var_name(var_name) { - let current = self.evaluate_arithmetic(var_name); + if is_arith_lvalue(var_name) { + let current = self.arith_lvalue_value(var_name); let new_value = current.wrapping_sub(1); - self.set_variable(var_name.to_string(), new_value.to_string()); + self.set_arith_lvalue(var_name, new_value.to_string()); return current; // Return old value for post-decrement } } @@ -10492,6 +10492,29 @@ impl Interpreter { } } + /// Budgeted write of one associative-array element (key already expanded). + fn set_assoc_element_checked(&mut self, resolved_name: String, key: String, value: String) { + let old_len = self + .scoped + .assoc_arrays + .get(&resolved_name) + .and_then(|a| a.get(&key)) + .map(String::len); + let is_new_entry = old_len.is_none(); + let added = if is_new_entry { + key.len() + value.len() + } else { + value.len() + }; + if !self.admit_array_write(usize::from(is_new_entry), added, old_len.unwrap_or(0)) { + return; + } + self.assoc_arrays_mut() + .entry(resolved_name) + .or_default() + .insert(key, value); + } + /// Set a parameter expansion assignment target (`:=`), including array elements. fn set_parameter_expansion_target(&mut self, name: &str, value: String) { if let Some(bracket) = name.find('[') @@ -10503,25 +10526,7 @@ impl Interpreter { if self.scoped.assoc_arrays.contains_key(&resolved_name) { let expanded_key = self.expand_variable_or_literal(key); - let old_len = self - .scoped - .assoc_arrays - .get(&resolved_name) - .and_then(|a| a.get(&expanded_key)) - .map(String::len); - let is_new_entry = old_len.is_none(); - let added = if is_new_entry { - expanded_key.len() + value.len() - } else { - value.len() - }; - if !self.admit_array_write(usize::from(is_new_entry), added, old_len.unwrap_or(0)) { - return; - } - self.assoc_arrays_mut() - .entry(resolved_name) - .or_default() - .insert(expanded_key, value); + self.set_assoc_element_checked(resolved_name, expanded_key, value); return; } diff --git a/crates/bashkit/src/parser/mod.rs b/crates/bashkit/src/parser/mod.rs index 71a442f28..9bcac8a8e 100644 --- a/crates/bashkit/src/parser/mod.rs +++ b/crates/bashkit/src/parser/mod.rs @@ -1876,7 +1876,9 @@ impl<'a> Parser<'a> { if !expr.is_empty() && !expr.ends_with(' ') && !expr.ends_with('(') { expr.push(' '); } - expr.push_str(w); + // Quotes are removed inside `((...))`; drop the lexer's + // quote-boundary markers so `c["x y"]` keys stay clean. + expr.extend(w.chars().filter(|&c| c != '\u{1e}' && c != '\u{1f}')); self.advance(); } Some(tokens::Token::Semicolon) => { diff --git a/crates/bashkit/tests/integration/threat_model_tests.rs b/crates/bashkit/tests/integration/threat_model_tests.rs index d182568c3..9f87e126a 100644 --- a/crates/bashkit/tests/integration/threat_model_tests.rs +++ b/crates/bashkit/tests/integration/threat_model_tests.rs @@ -3894,6 +3894,27 @@ printf "%s %s\n" "${#arr[@]}" "${#extra[@]}" assert_eq!(result.stdout.trim(), "3 0"); } + /// TM-DOS-060: arithmetic writes to array elements are charged to the budget. + #[tokio::test] + async fn tm_dos_060_arithmetic_array_writes_respect_budget() { + let mem = MemoryLimits::new().max_array_entries(3); + let mut bash = Bash::builder() + .memory_limits(mem) + .session_limits(SessionLimits::unlimited()) + .build(); + + let script = r#" +declare -A c +for ((i = 0; i < 10; i++)); do ((c[k$i]+=1)); done +for ((i = 0; i < 10; i++)); do ((a[i]++)); done +printf "%s %s\n" "${#c[@]}" "${#a[@]}" +"#; + let result = bash.exec(script).await.unwrap(); + + assert_eq!(result.exit_code, 0); + assert_eq!(result.stdout.trim(), "3 0"); + } + /// TM-DOS-060: saved local array shadows must stay charged to array budget. #[tokio::test] async fn tm_dos_060_local_array_shadow_preserves_array_budget() { diff --git a/crates/bashkit/tests/spec_cases/bash/arith-array-lvalue.test.sh b/crates/bashkit/tests/spec_cases/bash/arith-array-lvalue.test.sh new file mode 100644 index 000000000..cdf727061 --- /dev/null +++ b/crates/bashkit/tests/spec_cases/bash/arith-array-lvalue.test.sh @@ -0,0 +1,85 @@ +# Arithmetic assignment to array elements: ((a[i]++)), ((c[k]+=n)), let a[i]=v + +### arr_postinc_indexed +a=(1 2); ((a[1]++)); echo "${a[1]}" +### expect +3 +### end + +### arr_postinc_assoc_bare_key +declare -A c; ((c[x]++)); ((c[x]++)); echo "${c[x]}" +### expect +2 +### end + +### arr_compound_assoc_var_key +declare -A c; k=foo; ((c[$k]+=3)); echo "${c[foo]}" +### expect +3 +### end + +### arr_assign_new_index +a=(1 3); ((a[2]=5)); echo "${a[*]}" +### expect +1 3 5 +### end + +### arr_let_postinc +a=(1 3); let a[1]++; echo "${a[1]}" +### expect +4 +### end + +### arr_var_index_compound +a=(1 2); i=0; ((a[i]+=5)); echo "${a[0]}" +### expect +6 +### end + +### arr_quoted_assoc_key +declare -A c; ((c["x y"]++)); echo "${c[x y]}" +### expect +1 +### end + +### arr_expansion_inc_value +a=(0 4); echo $((a[1]++)) $((++a[1])) ${a[1]} +### expect +4 6 6 +### end + +### arr_assign_creates_array +((b[3]=7)); declare -p b +### expect +declare -a b=([3]="7") +### end + +### arr_index_expression +a=(1 6); i=0; ((a[i+1]*=2)); echo "${a[1]}" +### expect +12 +### end + +### arr_predec +a=(6 1); ((--a[0])); echo "${a[0]}" +### expect +5 +### end + +### arr_assoc_postdec_value +declare -A c; c[x]=4; echo $((c[x]--)) ${c[x]} +### expect +4 3 +### end + +### arr_negative_index +a=(5 12 5); ((a[-1]+=1)); echo "${a[*]}" +### expect +5 12 6 +### end + +### arr_assoc_key_embedded_var +declare -A c; for ((i = 0; i < 3; i++)); do ((c[k$i]+=1)); done; ((c[${i}x]++)); echo "${!c[@]}" | tr ' ' '\n' | sort | tr '\n' ' '; echo +### expect +3x k0 k1 k2 +### end diff --git a/knowledge/log.md b/knowledge/log.md index 439e03810..d3927f9e6 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -2,6 +2,7 @@ ## 2026-10-07 +* **Fix**: Arithmetic assigns to array elements. `((a[i]++))`, `((c[$k]+=3))`, `((a[2]=5))`, `((--a[0]))`, `let a[1]++` and `$((c[x]--))` read and write the element (indexed subscripts are arithmetic and accept negative indexes; associative keys are literal, quotes dropped as in `((c["x y"]++))`). Before, the update was lost or written to a scalar. Writes go through the budgeted `${a[i]:=v}` writer, so array memory limits still apply. Found by gap telemetry. * **Fix**: Gaps from the replayed agent calls: `set -n` / `set -o noexec` now stops running the rest of the script, as in bash (it was accepted and ignored); `find -ls` prints GNU's `ls -dils` layout; `tar -c` stores absolute paths without the leading `/` (with GNU's `Removing leading` notice) and `tar -cv` lists members on stdout unless the archive goes to stdout. Specs: `set-noexec.test.sh`, `find.test.sh`, `tar.test.sh`. * **Fix**: Quotes and patterns inside `${...}` operands. Unquoted `${x:-'d'}` keeps no quotes and `'$y'` stays literal; `"${x:-'d'}"` keeps them, as in bash; a `}` or `/` inside quotes no longer ends the operand. `${x/pattern/rep}` now expands `$var`, quotes and `\c` in the pattern (it used them verbatim) and matches globs leftmost-longest with `?`, `[...]` and any number of `*`; extglob patterns follow bash's empty-match scan. Globs compile to a size-capped regex, extglob search is budgeted (TM-DOS-127). Known gap: a quoted default with spaces (`${x:-'a b'}`) is still word-split. Spec: `param-operand-quotes.test.sh`. * **Feature**: Gap telemetry (`bashkit-replay`, `just gaps`). Replays the 5,206 recorded eval tool calls (1,493 sessions) on the current bashkit and ranks missing commands, unknown options, unsupported features and parse errors; `/benches` shows the top 20 and the history. First report: calls hitting a gap fell from 319 recorded to 125, non-zero exits from 723 to 231. Top remaining: heredoc `expected 'done'` and `unterminated command substitution` parse errors, `tar --strip-components`, `grep -I`, `join -o`, `find -ls`, plus probes for host tools (`busybox`, `gawk`, `sudo`, `python3`).