From 13f5cec29169dcd7a57ff6e433ec1bc1c307619c Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Tue, 29 Dec 2020 12:29:45 +0800 Subject: [PATCH] Fix call stack limits. --- RELEASES.md | 1 + src/engine.rs | 8 ++++-- src/engine_api.rs | 7 +++--- src/engine_settings.rs | 2 ++ src/fn_call.rs | 29 ++++++++++----------- src/module/mod.rs | 2 +- src/parser.rs | 57 ++++++++++++++++++++++-------------------- tests/stack.rs | 19 ++++++++------ 8 files changed, 70 insertions(+), 55 deletions(-) diff --git a/RELEASES.md b/RELEASES.md index 9530b5a7..ba43f472 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -16,6 +16,7 @@ Bug fixes * Fix bug when accessing properties in closures. * Fix bug when accessing a deep index with a function call. * Fix bug that sometimes allow assigning to an invalid l-value. +* Fix off-by-one error with `Engine::set_max_call_levels`. Breaking changes ---------------- diff --git a/src/engine.rs b/src/engine.rs index dae39f5f..bf36d028 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -170,6 +170,7 @@ impl FromIterator<(ImmutableString, Shared)> for Imports { #[cfg(not(feature = "unchecked"))] #[cfg(debug_assertions)] +#[cfg(not(feature = "no_function"))] pub const MAX_CALL_STACK_DEPTH: usize = 8; #[cfg(not(feature = "unchecked"))] #[cfg(debug_assertions)] @@ -181,6 +182,7 @@ pub const MAX_FUNCTION_EXPR_DEPTH: usize = 16; #[cfg(not(feature = "unchecked"))] #[cfg(not(debug_assertions))] +#[cfg(not(feature = "no_function"))] pub const MAX_CALL_STACK_DEPTH: usize = 128; #[cfg(not(feature = "unchecked"))] #[cfg(not(debug_assertions))] @@ -527,8 +529,8 @@ impl State { #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub struct Limits { /// Maximum levels of call-stack to prevent infinite recursion. - /// - /// Defaults to 16 for debug builds and 128 for non-debug builds. + /// Not available under `no_function`. + #[cfg(not(feature = "no_function"))] pub max_call_stack_depth: usize, /// Maximum depth of statements/expressions at global level (0 = unlimited). pub max_expr_depth: usize, @@ -809,6 +811,7 @@ impl Engine { #[cfg(not(feature = "unchecked"))] limits: Limits { + #[cfg(not(feature = "no_function"))] max_call_stack_depth: MAX_CALL_STACK_DEPTH, max_expr_depth: MAX_EXPR_DEPTH, #[cfg(not(feature = "no_function"))] @@ -864,6 +867,7 @@ impl Engine { #[cfg(not(feature = "unchecked"))] limits: Limits { + #[cfg(not(feature = "no_function"))] max_call_stack_depth: MAX_CALL_STACK_DEPTH, max_expr_depth: MAX_EXPR_DEPTH, #[cfg(not(feature = "no_function"))] diff --git a/src/engine_api.rs b/src/engine_api.rs index fff309b4..ff07e9e8 100644 --- a/src/engine_api.rs +++ b/src/engine_api.rs @@ -1453,7 +1453,7 @@ impl Engine { ) -> Result> { let mods = &mut (&self.global_sub_modules).into(); - let result = self.eval_ast_with_scope_raw(scope, mods, ast)?; + let result = self.eval_ast_with_scope_raw(scope, mods, ast, 0)?; let typ = self.map_type_name(result.type_name()); @@ -1473,12 +1473,13 @@ impl Engine { scope: &mut Scope, mods: &mut Imports, ast: &'a AST, + level: usize, ) -> Result> { let state = &mut State { source: ast.clone_source(), ..Default::default() }; - self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()]) + self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], level) } /// Evaluate a file, but throw away the result and only return error (if any). /// Useful for when you don't need the result, but still need to keep track of possible errors. @@ -1542,7 +1543,7 @@ impl Engine { source: ast.clone_source(), ..Default::default() }; - self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()])?; + self.eval_statements_raw(scope, mods, state, ast.statements(), &[ast.lib()], 0)?; Ok(()) } /// Call a script function defined in an [`AST`] with multiple arguments. diff --git a/src/engine_settings.rs b/src/engine_settings.rs index 1a291ccc..cb60a6d0 100644 --- a/src/engine_settings.rs +++ b/src/engine_settings.rs @@ -38,6 +38,7 @@ impl Engine { /// Set the maximum levels of function calls allowed for a script in order to avoid /// infinite recursion and stack overflows. #[cfg(not(feature = "unchecked"))] + #[cfg(not(feature = "no_function"))] #[inline(always)] pub fn set_max_call_levels(&mut self, levels: usize) -> &mut Self { self.limits.max_call_stack_depth = levels; @@ -45,6 +46,7 @@ impl Engine { } /// The maximum levels of function calls allowed for a script. #[cfg(not(feature = "unchecked"))] + #[cfg(not(feature = "no_function"))] #[inline(always)] pub fn max_call_levels(&self) -> usize { self.limits.max_call_stack_depth diff --git a/src/fn_call.rs b/src/fn_call.rs index c0329828..38bdeff6 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -405,11 +405,11 @@ impl Engine { mods.extend(fn_def.mods.iter_raw().map(|(n, m)| (n.clone(), m.clone()))); } - // Evaluate the function at one higher level of call depth + // Evaluate the function let stmt = &fn_def.body; let result = self - .eval_stmt(scope, mods, state, unified_lib, this_ptr, stmt, level + 1) + .eval_stmt(scope, mods, state, unified_lib, this_ptr, stmt, level) .or_else(|err| match *err { // Convert return statement to return value EvalAltResult::Return(x, _) => Ok(x), @@ -586,6 +586,8 @@ impl Engine { mem::swap(&mut state.source, &mut source); + let level = _level + 1; + let result = self.call_script_fn( scope, mods, @@ -595,7 +597,7 @@ impl Engine { func, rest, pos, - _level, + level, ); // Restore the original source @@ -610,8 +612,10 @@ impl Engine { mem::swap(&mut state.source, &mut source); + let level = _level + 1; + let result = self.call_script_fn( - scope, mods, state, lib, &mut None, func, args, pos, _level, + scope, mods, state, lib, &mut None, func, args, pos, level, ); // Restore the original source @@ -667,11 +671,12 @@ impl Engine { state: &mut State, statements: impl IntoIterator, lib: &[&Module], + level: usize, ) -> Result> { statements .into_iter() .try_fold(().into(), |_, stmt| { - self.eval_stmt(scope, mods, state, lib, &mut None, stmt, 0) + self.eval_stmt(scope, mods, state, lib, &mut None, stmt, level) }) .or_else(|err| match *err { EvalAltResult::Return(out, _) => Ok(out), @@ -691,7 +696,7 @@ impl Engine { lib: &[&Module], script: &str, pos: Position, - _level: usize, + level: usize, ) -> Result> { self.inc_operations(state, pos)?; @@ -700,13 +705,6 @@ impl Engine { return Ok(Dynamic::UNIT); } - // Check for stack overflow - #[cfg(not(feature = "no_function"))] - #[cfg(not(feature = "unchecked"))] - if _level > self.max_call_levels() { - return Err(Box::new(EvalAltResult::ErrorStackOverflow(pos))); - } - // Compile the script text // No optimizations because we only run it once let ast = self.compile_with_scope_and_optimization_level( @@ -727,7 +725,8 @@ impl Engine { ..Default::default() }; - let result = self.eval_statements_raw(scope, mods, &mut new_state, ast.statements(), lib); + let result = + self.eval_statements_raw(scope, mods, &mut new_state, ast.statements(), lib, level); state.operations = new_state.operations; result @@ -1208,6 +1207,8 @@ impl Engine { let mut source = module.id_raw().clone(); mem::swap(&mut state.source, &mut source); + let level = level + 1; + let result = self.call_script_fn( new_scope, mods, state, lib, &mut None, &fn_def, args, pos, level, ); diff --git a/src/module/mod.rs b/src/module/mod.rs index d1c023e5..c90775a2 100644 --- a/src/module/mod.rs +++ b/src/module/mod.rs @@ -1720,7 +1720,7 @@ impl Module { let orig_mods_len = mods.len(); // Run the script - engine.eval_ast_with_scope_raw(&mut scope, &mut mods, &ast)?; + engine.eval_ast_with_scope_raw(&mut scope, &mut mods, &ast, 0)?; // Create new module let mut module = Module::new(); diff --git a/src/parser.rs b/src/parser.rs index b1c39d36..ca546ac9 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -19,7 +19,7 @@ use crate::stdlib::{ vec::Vec, }; use crate::syntax::{CustomSyntax, MARKER_BLOCK, MARKER_EXPR, MARKER_IDENT}; -use crate::token::{is_doc_comment, is_keyword_function, is_valid_identifier, Token, TokenStream}; +use crate::token::{is_keyword_function, is_valid_identifier, Token, TokenStream}; use crate::utils::{get_hasher, StraightHasherBuilder}; use crate::{ calc_script_fn_hash, Dynamic, Engine, ImmutableString, LexError, ParseError, ParseErrorType, @@ -2534,35 +2534,38 @@ fn parse_stmt( ) -> Result { use AccessMode::{ReadOnly, ReadWrite}; - let mut comments: Vec = Default::default(); - let mut comments_pos = Position::NONE; + let mut _comments: Vec = Default::default(); - // Handle doc-comments. #[cfg(not(feature = "no_function"))] - while let (Token::Comment(ref comment), comment_pos) = input.peek().unwrap() { - if comments_pos.is_none() { - comments_pos = *comment_pos; - } + { + let mut comments_pos = Position::NONE; - if !is_doc_comment(comment) { - unreachable!("expecting doc-comment, but gets {:?}", comment); - } - - if !settings.is_global { - return Err(PERR::WrongDocComment.into_err(comments_pos)); - } - - match input.next().unwrap().0 { - Token::Comment(comment) => { - comments.push(comment); - - match input.peek().unwrap() { - (Token::Fn, _) | (Token::Private, _) => break, - (Token::Comment(_), _) => (), - _ => return Err(PERR::WrongDocComment.into_err(comments_pos)), - } + // Handle doc-comments. + while let (Token::Comment(ref comment), pos) = input.peek().unwrap() { + if comments_pos.is_none() { + comments_pos = *pos; + } + + if !crate::token::is_doc_comment(comment) { + unreachable!("expecting doc-comment, but gets {:?}", comment); + } + + if !settings.is_global { + return Err(PERR::WrongDocComment.into_err(comments_pos)); + } + + match input.next().unwrap().0 { + Token::Comment(comment) => { + _comments.push(comment); + + match input.peek().unwrap() { + (Token::Fn, _) | (Token::Private, _) => break, + (Token::Comment(_), _) => (), + _ => return Err(PERR::WrongDocComment.into_err(comments_pos)), + } + } + t => unreachable!("expecting Token::Comment, but gets {:?}", t), } - t => unreachable!("expecting Token::Comment, but gets {:?}", t), } } @@ -2618,7 +2621,7 @@ fn parse_stmt( pos: pos, }; - let func = parse_fn(input, &mut new_state, lib, access, settings, comments)?; + let func = parse_fn(input, &mut new_state, lib, access, settings, _comments)?; lib.insert( // Qualifiers (none) + function name + number of arguments. diff --git a/tests/stack.rs b/tests/stack.rs index 3319a55c..03f1c083 100644 --- a/tests/stack.rs +++ b/tests/stack.rs @@ -10,21 +10,24 @@ fn test_stack_overflow_fn_calls() -> Result<(), Box> { engine.eval::( r" fn foo(n) { if n <= 1 { 0 } else { n + foo(n-1) } } - foo(7) - ", + foo(6) + ", )?, - 27 + 20 ); + let max = engine.max_call_levels(); + #[cfg(not(feature = "unchecked"))] assert!(matches!( *engine - .eval::<()>( + .eval::<()>(&format!( r" - fn foo(n) { if n == 0 { 0 } else { n + foo(n-1) } } - foo(1000) - " - ) + fn foo(n) {{ if n == 0 {{ 0 }} else {{ n + foo(n-1) }} }} + foo({}) + ", + max + 1 + )) .expect_err("should error"), EvalAltResult::ErrorStackOverflow(_) ));