From ccee4eeb357877d0f67675142b3b297e101beff6 Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Mon, 6 Apr 2026 10:17:01 +0000 Subject: [PATCH] fix(interpreter): Box::pin expand_word to prevent stack overflow in nested $() Each command substitution level previously inlined the expand_word async state machine into the caller's stack frame. The state machine is large (many WordPart match arms with their locals) and at ~20-30 nesting levels the call stack overflows with SIGABRT. Fix by boxing two key futures: - expand_word: wraps the inner implementation in Box::pin so each call allocates its future on the heap instead of the parent's stack - execute_cmd_subst: new helper that Box::pin-s the command substitution body (state snapshot, command loop, state restore), keeping those large locals off the stack With both futures boxed, each nesting level uses constant stack space regardless of depth. The default max_subst_depth of 32 now completes without overflow (previously crashed at ~20). Closes #1089 --- crates/bashkit/src/interpreter/mod.rs | 125 +++++++++++------- .../tests/stack_overflow_regression_tests.rs | 58 ++++++++ specs/006-threat-model.md | 1 + 3 files changed, 133 insertions(+), 51 deletions(-) create mode 100644 crates/bashkit/tests/stack_overflow_regression_tests.rs diff --git a/crates/bashkit/src/interpreter/mod.rs b/crates/bashkit/src/interpreter/mod.rs index a3ab3588..d426e7b6 100644 --- a/crates/bashkit/src/interpreter/mod.rs +++ b/crates/bashkit/src/interpreter/mod.rs @@ -6160,7 +6160,75 @@ impl Interpreter { } } - async fn expand_word(&mut self, word: &Word) -> Result { + // THREAT[TM-DOS-089]: Command substitution body extracted into a Box::pin-ed + // helper to cap per-level stack usage. Without this, each $(...) nesting level + // adds the full expand_word state machine to the call stack, causing overflow + // at moderate depths despite the logical depth limit. + fn execute_cmd_subst<'a>( + &'a mut self, + commands: &'a [Command], + ) -> std::pin::Pin> + Send + 'a>> { + Box::pin(async move { + // Command substitution runs in a subshell: snapshot all + // mutable state so mutations don't leak to the parent. + let saved_traps = self.traps.clone(); + let saved_functions = self.functions.clone(); + let saved_vars = self.variables.clone(); + let saved_arrays = self.arrays.clone(); + let saved_assoc = self.assoc_arrays.clone(); + let saved_aliases = self.aliases.clone(); + let saved_cwd = self.cwd.clone(); + let saved_memory_budget = self.memory_budget.clone(); + let mut stdout = String::new(); + for cmd in commands { + let cmd_result = self.execute_command(cmd).await?; + stdout.push_str(&cmd_result.stdout); + self.last_exit_code = cmd_result.exit_code; + if matches!(cmd_result.control_flow, ControlFlow::Exit(_)) { + break; + } + } + // Fire EXIT trap set inside the command substitution + if let Some(trap_cmd) = self.traps.get("EXIT").cloned() + && saved_traps.get("EXIT") != Some(&trap_cmd) + && let Ok(trap_script) = Parser::with_limits( + &trap_cmd, + self.limits.max_ast_depth, + self.limits.max_parser_operations, + ) + .parse() + && let Ok(trap_result) = self.execute_command_sequence(&trap_script.commands).await + { + stdout.push_str(&trap_result.stdout); + } + // Restore parent state + self.traps = saved_traps; + self.functions = saved_functions; + self.variables = saved_vars; + self.arrays = saved_arrays; + self.assoc_arrays = saved_assoc; + self.aliases = saved_aliases; + self.cwd = saved_cwd; + self.memory_budget = saved_memory_budget; + self.counters.pop_subst(); + self.subst_generation += 1; + let trimmed = stdout.trim_end_matches('\n'); + Ok(trimmed.to_string()) + }) + } + + // THREAT[TM-DOS-089]: Box::pin the expand_word future to cap per-level + // stack usage. Without this, the async state machine of expand_word (which + // contains all WordPart match arms) is inlined into the caller's future, + // causing stack overflow at moderate command substitution depths. + fn expand_word<'a>( + &'a mut self, + word: &'a Word, + ) -> std::pin::Pin> + Send + 'a>> { + Box::pin(async move { self.expand_word_inner(word).await }) + } + + async fn expand_word_inner(&mut self, word: &Word) -> Result { let mut result = String::new(); let mut is_first_part = true; @@ -6212,61 +6280,16 @@ impl Interpreter { } } WordPart::CommandSubstitution(commands) => { - // THREAT[TM-DOS-088]: Track substitution depth separately from - // function depth. Each level clones the full interpreter state, - // so memory ≈ depth × state_size. A tighter limit than - // max_function_depth prevents OOM. + // THREAT[TM-DOS-088]: Track substitution depth to prevent OOM. if self.counters.push_subst(&self.limits).is_err() { return Err(crate::error::Error::Execution( "maximum command substitution depth exceeded".to_string(), )); } - // Command substitution runs in a subshell: snapshot all - // mutable state so mutations don't leak to the parent. - let saved_traps = self.traps.clone(); - let saved_functions = self.functions.clone(); - let saved_vars = self.variables.clone(); - let saved_arrays = self.arrays.clone(); - let saved_assoc = self.assoc_arrays.clone(); - let saved_aliases = self.aliases.clone(); - let saved_cwd = self.cwd.clone(); - let saved_memory_budget = self.memory_budget.clone(); - let mut stdout = String::new(); - for cmd in commands { - let cmd_result = self.execute_command(cmd).await?; - stdout.push_str(&cmd_result.stdout); - self.last_exit_code = cmd_result.exit_code; - if matches!(cmd_result.control_flow, ControlFlow::Exit(_)) { - break; - } - } - // Fire EXIT trap set inside the command substitution - if let Some(trap_cmd) = self.traps.get("EXIT").cloned() - && saved_traps.get("EXIT") != Some(&trap_cmd) - && let Ok(trap_script) = Parser::with_limits( - &trap_cmd, - self.limits.max_ast_depth, - self.limits.max_parser_operations, - ) - .parse() - && let Ok(trap_result) = - self.execute_command_sequence(&trap_script.commands).await - { - stdout.push_str(&trap_result.stdout); - } - // Restore parent state - self.traps = saved_traps; - self.functions = saved_functions; - self.variables = saved_vars; - self.arrays = saved_arrays; - self.assoc_arrays = saved_assoc; - self.aliases = saved_aliases; - self.cwd = saved_cwd; - self.memory_budget = saved_memory_budget; - self.counters.pop_subst(); - self.subst_generation += 1; - let trimmed = stdout.trim_end_matches('\n'); - result.push_str(trimmed); + // THREAT[TM-DOS-089]: Delegate to Box::pin-ed helper to + // prevent stack growth proportional to nesting depth. + let trimmed = self.execute_cmd_subst(commands).await?; + result.push_str(&trimmed); } WordPart::ArithmeticExpansion(expr) => { let expanded_expr = if expr.contains("$(") { diff --git a/crates/bashkit/tests/stack_overflow_regression_tests.rs b/crates/bashkit/tests/stack_overflow_regression_tests.rs new file mode 100644 index 00000000..77bb8242 --- /dev/null +++ b/crates/bashkit/tests/stack_overflow_regression_tests.rs @@ -0,0 +1,58 @@ +// THREAT[TM-DOS-089]: Stack overflow regression tests for nested command substitution. +// Before the Box::pin fix, each $(...) level inlined its async state machine into the +// caller's stack frame, causing overflow at moderate depths. The fix moves the command +// substitution body to the heap via Box::pin, keeping stack usage constant per level. + +use bashkit::{Bash, ExecutionLimits}; + +/// Default max_subst_depth (32) must not cause stack overflow. +/// Before the Box::pin fix, this would SIGABRT at ~20-30 levels. +/// Regression test for issue #1089. +#[tokio::test] +async fn depth_32_no_stack_overflow() { + let depth = 32; + let mut cmd = "echo hello".to_string(); + for _ in 0..depth { + cmd = format!("echo $({})", cmd); + } + + let mut bash = Bash::builder() + .limits( + ExecutionLimits::new() + .max_commands(500) + .max_subst_depth(32) + .timeout(std::time::Duration::from_secs(5)), + ) + .build(); + + // Must not stack-overflow — should return a result (possibly truncated by depth limit) + let result = bash.exec(&cmd).await; + if let Ok(r) = result { + // Depth limit error is also acceptable (Err case) + assert!(!r.stdout.is_empty() || r.exit_code != 0); + } +} + +/// Deeply nested $() in arithmetic context (the fuzz crash vector). +/// Regression test for the specific #1089 crash input pattern. +#[tokio::test] +async fn nested_subst_in_arithmetic_no_overflow() { + // Simulates the crash input: nested $( inside $((...)) + let mut inner = "echo 1".to_string(); + for _ in 0..15 { + inner = format!("echo $({})", inner); + } + let script = format!("echo $(($({})))", inner); + + let mut bash = Bash::builder() + .limits( + ExecutionLimits::new() + .max_commands(500) + .max_subst_depth(20) + .timeout(std::time::Duration::from_secs(5)), + ) + .build(); + + // Must not panic or SIGABRT + let _ = bash.exec(&script).await; +} diff --git a/specs/006-threat-model.md b/specs/006-threat-model.md index e37800c1..cc03bf8c 100644 --- a/specs/006-threat-model.md +++ b/specs/006-threat-model.md @@ -1229,6 +1229,7 @@ This section maps former vulnerability IDs to the new threat ID scheme and track | 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 | | 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 | | 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 | +| 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 | ### Accepted (Low Priority)