From c75d51ae880267f567cc09adb4d041094d0bdaf1 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Thu, 6 Jan 2022 22:10:16 +0800 Subject: [PATCH] Reduce unnecessary data size checking. --- src/engine.rs | 150 ++++++++++++++++-------------------- src/func/call.rs | 19 +++-- src/packages/array_basic.rs | 6 +- tests/data_size.rs | 32 +++++--- 4 files changed, 105 insertions(+), 102 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index e5211627..00dadfeb 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -361,6 +361,7 @@ impl<'a> Target<'a> { } } /// Get the source [`Dynamic`] of the [`Target`]. + #[allow(dead_code)] #[inline] #[must_use] pub fn source(&self) -> &Dynamic { @@ -1459,9 +1460,6 @@ impl Engine { } } - #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.as_ref(), root.1)?; - Ok(result) } // xxx[rhs] op= new_val @@ -1478,6 +1476,8 @@ impl Engine { global, state, lib, op_info, op_pos, obj_ptr, root, new_val, ) .map_err(|err| err.fill_position(new_pos))?; + #[cfg(not(feature = "unchecked"))] + self.check_data_size(obj_ptr, new_pos)?; None } // Can't index - try to call an index setter @@ -1501,9 +1501,6 @@ impl Engine { )?; } - #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.as_ref(), root.1)?; - Ok((Dynamic::UNIT, true)) } // xxx[rhs] @@ -1549,7 +1546,7 @@ impl Engine { .map_err(|err| err.fill_position(new_pos))?; } #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.as_ref(), root.1)?; + self.check_data_size(target.source(), new_pos)?; Ok((Dynamic::UNIT, true)) } // {xxx:map}.id @@ -1605,9 +1602,6 @@ impl Engine { ) .map_err(|err| err.fill_position(new_pos))?; - #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.as_ref(), root.1)?; - new_val = orig_val; } @@ -1799,9 +1793,6 @@ impl Engine { _ => Err(err), }, )?; - - #[cfg(not(feature = "unchecked"))] - self.check_data_size(target.as_ref(), root.1)?; } Ok((result, may_be_changed)) @@ -1867,7 +1858,7 @@ impl Engine { let is_assignment = new_val.is_some(); - let result = match lhs { + match lhs { // id.??? or id[???] Expr::Variable(_, var_pos, x) => { #[cfg(not(feature = "unchecked"))] @@ -1900,12 +1891,6 @@ impl Engine { .map(|(v, _)| if is_assignment { Dynamic::UNIT } else { v }) .map_err(|err| err.fill_position(op_pos)) } - }; - - if is_assignment { - result.map(|_| Dynamic::UNIT) - } else { - self.check_return_value(result, expr.position()) } } @@ -2355,7 +2340,7 @@ impl Engine { .. } = expr; - let result = if let Some(namespace) = namespace.as_ref() { + if let Some(namespace) = namespace.as_ref() { // Qualified function call let hash = hashes.native; @@ -2369,9 +2354,7 @@ impl Engine { scope, global, state, lib, this_ptr, name, args, constants, *hashes, pos, *capture, level, ) - }; - - self.check_return_value(result, pos) + } } /// Evaluate an expression. @@ -2695,49 +2678,45 @@ impl Engine { op, }) = op_info { - { - let mut lock_guard; - let lhs_ptr_inner; + let mut lock_guard; + let lhs_ptr_inner; - #[cfg(not(feature = "no_closure"))] - let target_is_shared = target.is_shared(); - #[cfg(feature = "no_closure")] - let target_is_shared = false; + #[cfg(not(feature = "no_closure"))] + let target_is_shared = target.is_shared(); + #[cfg(feature = "no_closure")] + let target_is_shared = false; - if target_is_shared { - lock_guard = target.write_lock::().expect("`Dynamic`"); - lhs_ptr_inner = &mut *lock_guard; - } else { - lhs_ptr_inner = &mut *target; - } - - let hash = hash_op_assign; - let args = &mut [lhs_ptr_inner, &mut new_val]; - - match self.call_native_fn(global, state, lib, op, hash, args, true, true, op_pos) { - Err(err) if matches!(*err, ERR::ErrorFunctionNotFound(ref f, _) if f.starts_with(op)) => - { - // Expand to `var = var op rhs` - let op = &op[..op.len() - 1]; // extract operator without = - - // Run function - let (value, _) = self.call_native_fn( - global, state, lib, op, hash_op, args, true, false, op_pos, - )?; - - *args[0] = value.flatten(); - } - err => return err.map(|_| ()), + if target_is_shared { + lock_guard = target.write_lock::().expect("`Dynamic`"); + lhs_ptr_inner = &mut *lock_guard; + } else { + lhs_ptr_inner = &mut *target; + } + + let hash = hash_op_assign; + let args = &mut [lhs_ptr_inner, &mut new_val]; + + match self.call_native_fn(global, state, lib, op, hash, args, true, true, op_pos) { + Err(err) if matches!(*err, ERR::ErrorFunctionNotFound(ref f, _) if f.starts_with(op)) => + { + // Expand to `var = var op rhs` + let op = &op[..op.len() - 1]; // extract operator without = + + // Run function + let (value, _) = self.call_native_fn( + global, state, lib, op, hash_op, args, true, false, op_pos, + )?; + + *args[0] = value.flatten(); } + err => return err.map(|_| ()), } } else { // Normal assignment *target.as_mut() = new_val; } - target.propagate_changed_value()?; - - self.check_data_size(target.source(), Position::NONE) + target.propagate_changed_value() } /// Evaluate a statement. @@ -2768,20 +2747,14 @@ impl Engine { return self.eval_fn_call_expr(scope, global, state, lib, this_ptr, x, *pos, level); } - #[cfg(not(feature = "unchecked"))] - self.inc_operations(&mut global.num_operations, stmt.position())?; + // Then assignments. + // We shouldn't do this for too many variants because, soon or later, the added comparisons + // will cost more than the mis-predicted `match` branch. + if let Stmt::Assignment(x, op_pos) = stmt { + #[cfg(not(feature = "unchecked"))] + self.inc_operations(&mut global.num_operations, stmt.position())?; - match stmt { - // No-op - Stmt::Noop(_) => Ok(Dynamic::UNIT), - - // Expression as statement - Stmt::Expr(expr) => Ok(self - .eval_expr(scope, global, state, lib, this_ptr, expr, level)? - .flatten()), - - // var op= rhs - Stmt::Assignment(x, op_pos) if x.0.is_variable_access(false) => { + return if x.0.is_variable_access(false) { let (lhs_expr, op_info, rhs_expr) = x.as_ref(); let rhs_val = self .eval_expr(scope, global, state, lib, this_ptr, rhs_expr, level)? @@ -2810,16 +2783,8 @@ impl Engine { ) .map_err(|err| err.fill_position(rhs_expr.position()))?; - #[cfg(not(feature = "unchecked"))] - if op_info.is_some() { - self.check_data_size(lhs_ptr.as_ref(), lhs_expr.position())?; - } - Ok(Dynamic::UNIT) - } - - // lhs op= rhs - Stmt::Assignment(x, op_pos) => { + } else { let (lhs_expr, op_info, rhs_expr) = x.as_ref(); let rhs_val = self .eval_expr(scope, global, state, lib, this_ptr, rhs_expr, level)? @@ -2850,7 +2815,20 @@ impl Engine { } _ => unreachable!("cannot assign to expression: {:?}", lhs_expr), } - } + }; + } + + #[cfg(not(feature = "unchecked"))] + self.inc_operations(&mut global.num_operations, stmt.position())?; + + match stmt { + // No-op + Stmt::Noop(_) => Ok(Dynamic::UNIT), + + // Expression as statement + Stmt::Expr(expr) => Ok(self + .eval_expr(scope, global, state, lib, this_ptr, expr, level)? + .flatten()), // Block scope Stmt::Block(statements, _) if statements.is_empty() => Ok(Dynamic::UNIT), @@ -3395,7 +3373,7 @@ impl Engine { } /// Check a result to ensure that the data size is within allowable limit. - fn check_return_value(&self, mut result: RhaiResult, pos: Position) -> RhaiResult { + pub(crate) fn check_return_value(&self, mut result: RhaiResult, pos: Position) -> RhaiResult { let _pos = pos; match result { @@ -3470,7 +3448,11 @@ impl Engine { } Union::Str(ref s, _, _) => (0, 0, s.len()), #[cfg(not(feature = "no_closure"))] - Union::Shared(_, _, _) if !top => { + Union::Shared(_, _, _) if top => { + Self::calc_data_sizes(&*value.read_lock::().unwrap(), true) + } + #[cfg(not(feature = "no_closure"))] + Union::Shared(_, _, _) => { unreachable!("shared values discovered within data: {}", value) } _ => (0, 0, 0), @@ -3536,7 +3518,7 @@ impl Engine { /// Check whether the size of a [`Dynamic`] is within limits. #[cfg(not(feature = "unchecked"))] - fn check_data_size(&self, value: &Dynamic, pos: Position) -> RhaiResultOf<()> { + pub(crate) fn check_data_size(&self, value: &Dynamic, pos: Position) -> RhaiResultOf<()> { // If no data size limits, just return if !self.has_data_size_limit() { return Ok(()); diff --git a/src/func/call.rs b/src/func/call.rs index e7c10cad..2af8dc00 100644 --- a/src/func/call.rs +++ b/src/func/call.rs @@ -388,7 +388,14 @@ impl Engine { bk.restore_first_arg(args) } - let result = result.map_err(|err| err.fill_position(pos))?; + // Check the return value (including data sizes) + let result = self.check_return_value(result, pos)?; + + // Check the data size of any `&mut` object, which may be changed. + #[cfg(not(feature = "unchecked"))] + if is_ref_mut && args.len() > 0 { + self.check_data_size(&args[0], pos)?; + } // See if the function match print/debug (which requires special processing) return Ok(match name { @@ -1285,17 +1292,19 @@ impl Engine { Some(f) if f.is_plugin_fn() => { let context = (self, fn_name, module.id(), &*global, lib, pos).into(); - f.get_plugin_fn() + let result = f + .get_plugin_fn() .expect("plugin function") .clone() - .call(context, &mut args) - .map_err(|err| err.fill_position(pos)) + .call(context, &mut args); + self.check_return_value(result, pos) } Some(f) if f.is_native() => { let func = f.get_native_fn().expect("native function"); let context = (self, fn_name, module.id(), &*global, lib, pos).into(); - func(context, &mut args).map_err(|err| err.fill_position(pos)) + let result = func(context, &mut args); + self.check_return_value(result, pos) } Some(f) => unreachable!("unknown function type: {:?}", f), diff --git a/src/packages/array_basic.rs b/src/packages/array_basic.rs index 72c40e87..2ad195ac 100644 --- a/src/packages/array_basic.rs +++ b/src/packages/array_basic.rs @@ -3,7 +3,6 @@ use crate::engine::OP_EQUALS; use crate::plugin::*; -use crate::types::dynamic::Union; use crate::{ def_package, Array, Dynamic, ExclusiveRange, FnPtr, InclusiveRange, NativeCallContext, Position, RhaiResultOf, ERR, INT, @@ -97,8 +96,9 @@ pub mod array_functions { #[cfg(not(feature = "unchecked"))] let check_sizes = match item.0 { - Union::Array(_, _, _) | Union::Str(_, _, _) => true, - Union::Map(_, _, _) => true, + crate::types::dynamic::Union::Array(_, _, _) + | crate::types::dynamic::Union::Str(_, _, _) => true, + crate::types::dynamic::Union::Map(_, _, _) => true, _ => false, }; #[cfg(feature = "unchecked")] diff --git a/tests/data_size.rs b/tests/data_size.rs index 303496f8..a4dc1818 100644 --- a/tests/data_size.rs +++ b/tests/data_size.rs @@ -30,7 +30,7 @@ fn test_max_string_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( r#" let x = "hello, "; let y = "world!"; @@ -44,7 +44,7 @@ fn test_max_string_size() -> Result<(), Box> { #[cfg(not(feature = "no_object"))] assert!(matches!( *engine - .eval::( + .run( r#" let x = "hello"; x.pad(100, '!'); @@ -90,7 +90,7 @@ fn test_max_array_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( " let x = [1,2,3,4,5,6]; let y = [7,8,9,10,11,12]; @@ -101,10 +101,22 @@ fn test_max_array_size() -> Result<(), Box> { EvalAltResult::ErrorDataTooLarge(_, _) )); + assert!(matches!( + *engine + .run( + " + let x = [ 42 ]; + loop { x[0] = x; } + " + ) + .expect_err("should error"), + EvalAltResult::ErrorDataTooLarge(_, _) + )); + #[cfg(not(feature = "no_object"))] assert!(matches!( *engine - .eval::( + .run( " let x = [1,2,3,4,5,6]; x.pad(100, 42); @@ -117,7 +129,7 @@ fn test_max_array_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( " let x = [1,2,3]; [x, x, x, x] @@ -130,7 +142,7 @@ fn test_max_array_size() -> Result<(), Box> { #[cfg(not(feature = "no_object"))] assert!(matches!( *engine - .eval::( + .run( " let x = #{a:1, b:2, c:3}; [x, x, x, x] @@ -142,7 +154,7 @@ fn test_max_array_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( " let x = [1]; let y = [x, x]; @@ -220,7 +232,7 @@ fn test_max_map_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( " let x = #{a:1,b:2,c:3,d:4,e:5,f:6}; let y = #{g:7,h:8,i:9,j:10,k:11,l:12}; @@ -233,7 +245,7 @@ fn test_max_map_size() -> Result<(), Box> { assert!(matches!( *engine - .eval::( + .run( " let x = #{a:1,b:2,c:3}; #{u:x, v:x, w:x, z:x} @@ -246,7 +258,7 @@ fn test_max_map_size() -> Result<(), Box> { #[cfg(not(feature = "no_index"))] assert!(matches!( *engine - .eval::( + .run( " let x = [1, 2, 3]; #{u:x, v:x, w:x, z:x}