From 4b91ed57dd11dceda1e98c58b3e031696db3ba8f Mon Sep 17 00:00:00 2001 From: TrMen <31260183+TrMen@users.noreply.github.com> Date: Wed, 7 Feb 2024 23:42:24 +0100 Subject: [PATCH] Enforce call stack depth limit for all calls (#11729) # Description Previously, only direcly-recursive calls were checked for recursion depth. But most recursive calls in nushell are mutually recursive since expressions like `for`, `where`, `try` and `do` all execute a separte block. ```nushell def f [] { do { f } } ``` Calling `f` would crash nushell with a stack overflow. I think the only general way to prevent such a stack overflow is to enforce a maximum call stack depth instead of only disallowing directly recursive calls. This commit also moves that logic into `eval_call()` instead of `eval_block()` because the recursion limit is tracked in the `Stack`, but not all blocks are evaluated in a new stack. Incrementing the recursion depth of the caller's stack would permanently increment that for all future calls. Fixes #11667 # User-Facing Changes Any function call can now fail with `recursion_limit_reached` instead of just directly recursive calls. Mutually-recursive calls no longer crash nushell. # After Submitting --- crates/nu-command/tests/commands/try_.rs | 12 +++++++ crates/nu-engine/src/eval.rs | 31 ++++++++--------- crates/nu-parser/src/parse_keywords.rs | 44 +----------------------- crates/nu-protocol/src/ast/block.rs | 4 --- src/tests/test_custom_commands.rs | 12 +++++++ 5 files changed, 40 insertions(+), 63 deletions(-) diff --git a/crates/nu-command/tests/commands/try_.rs b/crates/nu-command/tests/commands/try_.rs index 81f5493e2c..6dc35d6840 100644 --- a/crates/nu-command/tests/commands/try_.rs +++ b/crates/nu-command/tests/commands/try_.rs @@ -81,3 +81,15 @@ fn catch_block_can_use_error_object() { let output = nu!("try {1 / 0} catch {|err| print ($err | get msg)}"); assert_eq!(output.out, "Division by zero.") } + +// This test is disabled on Windows because they cause a stack overflow in CI (but not locally!). +// For reasons we don't understand, the Windows CI runners are prone to stack overflow. +// TODO: investigate so we can enable on Windows +#[cfg(not(target_os = "windows"))] +#[test] +fn can_catch_infinite_recursion() { + let actual = nu!(r#" + def bang [] { try { bang } catch { "Caught infinite recursion" } }; bang + "#); + assert_eq!(actual.out, "Caught infinite recursion"); +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 5168794e8a..05c6358cd7 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -42,6 +42,21 @@ pub fn eval_call( let mut callee_stack = caller_stack.gather_captures(engine_state, &block.captures); + // Rust does not check recursion limits outside of const evaluation. + // But nu programs run in the same process as the shell. + // To prevent a stack overflow in user code from crashing the shell, + // we limit the recursion depth of function calls. + // Picked 50 arbitrarily, should work on all architectures. + const MAXIMUM_CALL_STACK_DEPTH: u64 = 50; + callee_stack.recursion_count += 1; + if callee_stack.recursion_count > MAXIMUM_CALL_STACK_DEPTH { + callee_stack.recursion_count = 0; + return Err(ShellError::RecursionLimitReached { + recursion_limit: MAXIMUM_CALL_STACK_DEPTH, + span: block.span, + }); + } + for (param_idx, (param, required)) in decl .signature() .required_positional @@ -635,22 +650,6 @@ pub fn eval_block( redirect_stdout: bool, redirect_stderr: bool, ) -> Result { - // if Block contains recursion, make sure we don't recurse too deeply (to avoid stack overflow) - if let Some(recursive) = block.recursive { - // picked 50 arbitrarily, should work on all architectures - const RECURSION_LIMIT: u64 = 50; - if recursive { - if stack.recursion_count >= RECURSION_LIMIT { - stack.recursion_count = 0; - return Err(ShellError::RecursionLimitReached { - recursion_limit: RECURSION_LIMIT, - span: block.span, - }); - } - stack.recursion_count += 1; - } - } - let num_pipelines = block.len(); for (pipeline_idx, pipeline) in block.pipelines.iter().enumerate() { diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index b9dd06f227..86ad56badc 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -598,8 +598,6 @@ pub fn parse_def( *declaration = signature.clone().into_block_command(block_id); let block = working_set.get_block_mut(block_id); - let calls_itself = block_calls_itself(block, decl_id); - block.recursive = Some(calls_itself); block.signature = signature; block.redirect_env = has_env; @@ -758,10 +756,7 @@ pub fn parse_extern( } else { *declaration = signature.clone().into_block_command(block_id); - let block = working_set.get_block_mut(block_id); - let calls_itself = block_calls_itself(block, decl_id); - block.recursive = Some(calls_itself); - block.signature = signature; + working_set.get_block_mut(block_id).signature = signature; } } else { let decl = KnownExternal { @@ -799,43 +794,6 @@ pub fn parse_extern( }]) } -fn block_calls_itself(block: &Block, decl_id: usize) -> bool { - block.pipelines.iter().any(|pipeline| { - pipeline - .elements - .iter() - .any(|pipe_element| match pipe_element { - PipelineElement::Expression( - _, - Expression { - expr: Expr::Call(call_expr), - .. - }, - ) => { - if call_expr.decl_id == decl_id { - return true; - } - call_expr.arguments.iter().any(|arg| match arg { - Argument::Positional(Expression { expr, .. }) => match expr { - Expr::Keyword(.., expr) => { - let expr = expr.as_ref(); - let Expression { expr, .. } = expr; - match expr { - Expr::Call(call_expr2) => call_expr2.decl_id == decl_id, - _ => false, - } - } - Expr::Call(call_expr2) => call_expr2.decl_id == decl_id, - _ => false, - }, - _ => false, - }) - } - _ => false, - }) - }) -} - pub fn parse_alias( working_set: &mut StateWorkingSet, lite_command: &LiteCommand, diff --git a/crates/nu-protocol/src/ast/block.rs b/crates/nu-protocol/src/ast/block.rs index 21b3e77fa7..9e1f60ea80 100644 --- a/crates/nu-protocol/src/ast/block.rs +++ b/crates/nu-protocol/src/ast/block.rs @@ -10,7 +10,6 @@ pub struct Block { pub captures: Vec, pub redirect_env: bool, pub span: Option, // None option encodes no span to avoid using test_span() - pub recursive: Option, // does the block call itself? } impl Block { @@ -51,7 +50,6 @@ impl Block { captures: vec![], redirect_env: false, span: None, - recursive: None, } } @@ -62,7 +60,6 @@ impl Block { captures: vec![], redirect_env: false, span: None, - recursive: None, } } @@ -97,7 +94,6 @@ where captures: vec![], redirect_env: false, span: None, - recursive: None, } } } diff --git a/src/tests/test_custom_commands.rs b/src/tests/test_custom_commands.rs index f42db73f99..ad31928331 100644 --- a/src/tests/test_custom_commands.rs +++ b/src/tests/test_custom_commands.rs @@ -214,6 +214,18 @@ fn infinite_recursion_does_not_panic() { assert!(actual.err.contains("Recursion limit (50) reached")); } +// This test is disabled on Windows because they cause a stack overflow in CI (but not locally!). +// For reasons we don't understand, the Windows CI runners are prone to stack overflow. +// TODO: investigate so we can enable on Windows +#[cfg(not(target_os = "windows"))] +#[test] +fn infinite_mutual_recursion_does_not_panic() { + let actual = nu!(r#" + def bang [] { def boom [] { bang }; boom }; bang + "#); + assert!(actual.err.contains("Recursion limit (50) reached")); +} + #[test] fn type_check_for_during_eval() -> TestResult { fail_test(