Skip to content

Commit b4212af

Browse files
authored
fix(interpreter): Box::pin expand_word to prevent stack overflow in nested $() (#1109)
## Summary - Box::pin `expand_word` and extract `execute_cmd_subst` helper to prevent stack overflow from nested command substitutions - Each `$(...)` level previously inlined the large `expand_word` async state machine into the caller's stack frame, causing SIGABRT at ~20-30 levels - With both futures boxed, each level uses constant stack space — depth 32 (default `max_subst_depth`) now completes safely ## Changes - `interpreter/mod.rs`: convert `expand_word` from `async fn` to `fn -> Pin<Box<Future>>` wrapper over `expand_word_inner`; add `execute_cmd_subst` Box::pin-ed helper for the command substitution body - New `stack_overflow_regression_tests.rs`: 2 tests exercising depth 32 and nested arithmetic substitution - `specs/006-threat-model.md`: add TM-DOS-089 ## Test plan - [x] `cargo test --test stack_overflow_regression_tests` — 2 new tests pass (previously SIGABRT) - [x] `cargo test --all-features -- --skip ssh_supabase` — all pass - [x] `cargo clippy --all-targets --all-features -- -D warnings` — clean Closes #1089
1 parent 6b9c38c commit b4212af

File tree

3 files changed

+133
-51
lines changed

3 files changed

+133
-51
lines changed

crates/bashkit/src/interpreter/mod.rs

Lines changed: 74 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -6160,7 +6160,75 @@ impl Interpreter {
61606160
}
61616161
}
61626162

6163-
async fn expand_word(&mut self, word: &Word) -> Result<String> {
6163+
// THREAT[TM-DOS-089]: Command substitution body extracted into a Box::pin-ed
6164+
// helper to cap per-level stack usage. Without this, each $(...) nesting level
6165+
// adds the full expand_word state machine to the call stack, causing overflow
6166+
// at moderate depths despite the logical depth limit.
6167+
fn execute_cmd_subst<'a>(
6168+
&'a mut self,
6169+
commands: &'a [Command],
6170+
) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<String>> + Send + 'a>> {
6171+
Box::pin(async move {
6172+
// Command substitution runs in a subshell: snapshot all
6173+
// mutable state so mutations don't leak to the parent.
6174+
let saved_traps = self.traps.clone();
6175+
let saved_functions = self.functions.clone();
6176+
let saved_vars = self.variables.clone();
6177+
let saved_arrays = self.arrays.clone();
6178+
let saved_assoc = self.assoc_arrays.clone();
6179+
let saved_aliases = self.aliases.clone();
6180+
let saved_cwd = self.cwd.clone();
6181+
let saved_memory_budget = self.memory_budget.clone();
6182+
let mut stdout = String::new();
6183+
for cmd in commands {
6184+
let cmd_result = self.execute_command(cmd).await?;
6185+
stdout.push_str(&cmd_result.stdout);
6186+
self.last_exit_code = cmd_result.exit_code;
6187+
if matches!(cmd_result.control_flow, ControlFlow::Exit(_)) {
6188+
break;
6189+
}
6190+
}
6191+
// Fire EXIT trap set inside the command substitution
6192+
if let Some(trap_cmd) = self.traps.get("EXIT").cloned()
6193+
&& saved_traps.get("EXIT") != Some(&trap_cmd)
6194+
&& let Ok(trap_script) = Parser::with_limits(
6195+
&trap_cmd,
6196+
self.limits.max_ast_depth,
6197+
self.limits.max_parser_operations,
6198+
)
6199+
.parse()
6200+
&& let Ok(trap_result) = self.execute_command_sequence(&trap_script.commands).await
6201+
{
6202+
stdout.push_str(&trap_result.stdout);
6203+
}
6204+
// Restore parent state
6205+
self.traps = saved_traps;
6206+
self.functions = saved_functions;
6207+
self.variables = saved_vars;
6208+
self.arrays = saved_arrays;
6209+
self.assoc_arrays = saved_assoc;
6210+
self.aliases = saved_aliases;
6211+
self.cwd = saved_cwd;
6212+
self.memory_budget = saved_memory_budget;
6213+
self.counters.pop_subst();
6214+
self.subst_generation += 1;
6215+
let trimmed = stdout.trim_end_matches('\n');
6216+
Ok(trimmed.to_string())
6217+
})
6218+
}
6219+
6220+
// THREAT[TM-DOS-089]: Box::pin the expand_word future to cap per-level
6221+
// stack usage. Without this, the async state machine of expand_word (which
6222+
// contains all WordPart match arms) is inlined into the caller's future,
6223+
// causing stack overflow at moderate command substitution depths.
6224+
fn expand_word<'a>(
6225+
&'a mut self,
6226+
word: &'a Word,
6227+
) -> std::pin::Pin<Box<dyn std::future::Future<Output = Result<String>> + Send + 'a>> {
6228+
Box::pin(async move { self.expand_word_inner(word).await })
6229+
}
6230+
6231+
async fn expand_word_inner(&mut self, word: &Word) -> Result<String> {
61646232
let mut result = String::new();
61656233
let mut is_first_part = true;
61666234

@@ -6212,61 +6280,16 @@ impl Interpreter {
62126280
}
62136281
}
62146282
WordPart::CommandSubstitution(commands) => {
6215-
// THREAT[TM-DOS-088]: Track substitution depth separately from
6216-
// function depth. Each level clones the full interpreter state,
6217-
// so memory ≈ depth × state_size. A tighter limit than
6218-
// max_function_depth prevents OOM.
6283+
// THREAT[TM-DOS-088]: Track substitution depth to prevent OOM.
62196284
if self.counters.push_subst(&self.limits).is_err() {
62206285
return Err(crate::error::Error::Execution(
62216286
"maximum command substitution depth exceeded".to_string(),
62226287
));
62236288
}
6224-
// Command substitution runs in a subshell: snapshot all
6225-
// mutable state so mutations don't leak to the parent.
6226-
let saved_traps = self.traps.clone();
6227-
let saved_functions = self.functions.clone();
6228-
let saved_vars = self.variables.clone();
6229-
let saved_arrays = self.arrays.clone();
6230-
let saved_assoc = self.assoc_arrays.clone();
6231-
let saved_aliases = self.aliases.clone();
6232-
let saved_cwd = self.cwd.clone();
6233-
let saved_memory_budget = self.memory_budget.clone();
6234-
let mut stdout = String::new();
6235-
for cmd in commands {
6236-
let cmd_result = self.execute_command(cmd).await?;
6237-
stdout.push_str(&cmd_result.stdout);
6238-
self.last_exit_code = cmd_result.exit_code;
6239-
if matches!(cmd_result.control_flow, ControlFlow::Exit(_)) {
6240-
break;
6241-
}
6242-
}
6243-
// Fire EXIT trap set inside the command substitution
6244-
if let Some(trap_cmd) = self.traps.get("EXIT").cloned()
6245-
&& saved_traps.get("EXIT") != Some(&trap_cmd)
6246-
&& let Ok(trap_script) = Parser::with_limits(
6247-
&trap_cmd,
6248-
self.limits.max_ast_depth,
6249-
self.limits.max_parser_operations,
6250-
)
6251-
.parse()
6252-
&& let Ok(trap_result) =
6253-
self.execute_command_sequence(&trap_script.commands).await
6254-
{
6255-
stdout.push_str(&trap_result.stdout);
6256-
}
6257-
// Restore parent state
6258-
self.traps = saved_traps;
6259-
self.functions = saved_functions;
6260-
self.variables = saved_vars;
6261-
self.arrays = saved_arrays;
6262-
self.assoc_arrays = saved_assoc;
6263-
self.aliases = saved_aliases;
6264-
self.cwd = saved_cwd;
6265-
self.memory_budget = saved_memory_budget;
6266-
self.counters.pop_subst();
6267-
self.subst_generation += 1;
6268-
let trimmed = stdout.trim_end_matches('\n');
6269-
result.push_str(trimmed);
6289+
// THREAT[TM-DOS-089]: Delegate to Box::pin-ed helper to
6290+
// prevent stack growth proportional to nesting depth.
6291+
let trimmed = self.execute_cmd_subst(commands).await?;
6292+
result.push_str(&trimmed);
62706293
}
62716294
WordPart::ArithmeticExpansion(expr) => {
62726295
let expanded_expr = if expr.contains("$(") {
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
// THREAT[TM-DOS-089]: Stack overflow regression tests for nested command substitution.
2+
// Before the Box::pin fix, each $(...) level inlined its async state machine into the
3+
// caller's stack frame, causing overflow at moderate depths. The fix moves the command
4+
// substitution body to the heap via Box::pin, keeping stack usage constant per level.
5+
6+
use bashkit::{Bash, ExecutionLimits};
7+
8+
/// Default max_subst_depth (32) must not cause stack overflow.
9+
/// Before the Box::pin fix, this would SIGABRT at ~20-30 levels.
10+
/// Regression test for issue #1089.
11+
#[tokio::test]
12+
async fn depth_32_no_stack_overflow() {
13+
let depth = 32;
14+
let mut cmd = "echo hello".to_string();
15+
for _ in 0..depth {
16+
cmd = format!("echo $({})", cmd);
17+
}
18+
19+
let mut bash = Bash::builder()
20+
.limits(
21+
ExecutionLimits::new()
22+
.max_commands(500)
23+
.max_subst_depth(32)
24+
.timeout(std::time::Duration::from_secs(5)),
25+
)
26+
.build();
27+
28+
// Must not stack-overflow — should return a result (possibly truncated by depth limit)
29+
let result = bash.exec(&cmd).await;
30+
if let Ok(r) = result {
31+
// Depth limit error is also acceptable (Err case)
32+
assert!(!r.stdout.is_empty() || r.exit_code != 0);
33+
}
34+
}
35+
36+
/// Deeply nested $() in arithmetic context (the fuzz crash vector).
37+
/// Regression test for the specific #1089 crash input pattern.
38+
#[tokio::test]
39+
async fn nested_subst_in_arithmetic_no_overflow() {
40+
// Simulates the crash input: nested $( inside $((...))
41+
let mut inner = "echo 1".to_string();
42+
for _ in 0..15 {
43+
inner = format!("echo $({})", inner);
44+
}
45+
let script = format!("echo $(($({})))", inner);
46+
47+
let mut bash = Bash::builder()
48+
.limits(
49+
ExecutionLimits::new()
50+
.max_commands(500)
51+
.max_subst_depth(20)
52+
.timeout(std::time::Duration::from_secs(5)),
53+
)
54+
.build();
55+
56+
// Must not panic or SIGABRT
57+
let _ = bash.exec(&script).await;
58+
}

specs/006-threat-model.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,6 +1229,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track
12291229
| TM-INT-007 | `/dev/urandom` empty with `head -c` | Weak randomness — `head -c 16 /dev/urandom` returns empty string | Fix virtual device pipe handling in head builtin |
12301230
| TM-DOS-044 | Nested `$()` stack overflow (regression) | Process crash (SIGABRT) at depth ~50 despite #492 fix | Interpreter execution path may need separate depth tracking from lexer fix |
12311231
| TM-DOS-088 | Command substitution OOM via state cloning | OOM at depth N (memory ≈ N × state_size) | Dedicated `max_subst_depth` limit (default 32), separate from `max_function_depth`**FIXED** via #1088 |
1232+
| TM-DOS-089 | Command substitution stack overflow via inlined futures | SIGABRT at ~20-30 nested $() levels | Box::pin `expand_word` and `execute_cmd_subst` to cap per-level stack — **FIXED** via #1089 |
12321233

12331234
### Accepted (Low Priority)
12341235

0 commit comments

Comments
 (0)