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)