From 68ea8c27fdf3d4b937bb43b269d6a29ce96bee5d Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sat, 12 Jun 2021 10:26:50 +0800 Subject: [PATCH] Fix unchecked. Do not duplicate data size checking. --- codegen/src/rhai_module.rs | 2 +- src/ast.rs | 10 +-- src/engine.rs | 140 ++++++++++++++++--------------------- src/optimize.rs | 2 +- src/token.rs | 2 +- tests/data_size.rs | 12 ++++ 6 files changed, 80 insertions(+), 88 deletions(-) diff --git a/codegen/src/rhai_module.rs b/codegen/src/rhai_module.rs index 77025db1..17a29fd6 100644 --- a/codegen/src/rhai_module.rs +++ b/codegen/src/rhai_module.rs @@ -130,7 +130,7 @@ pub fn generate_body( let mut namespace = FnNamespaceAccess::Internal; match function.params().special { - FnSpecialAccess::None => {} + FnSpecialAccess::None => (), FnSpecialAccess::Index(_) | FnSpecialAccess::Property(_) => { let reg_name = fn_literal.value(); if reg_name.starts_with(FN_GET) diff --git a/src/ast.rs b/src/ast.rs index 1834a965..0a75dd9d 100644 --- a/src/ast.rs +++ b/src/ast.rs @@ -952,7 +952,7 @@ pub enum Stmt { /// \[`export`\] `const` id `=` expr Const(Expr, Box, bool, Position), /// expr op`=` expr - Assignment(Box<(Expr, Option, Expr)>, Position), + Assignment(Box<(Expr, Option>, Expr)>, Position), /// func `(` expr `,` ... `)` /// /// Note - this is a duplicate of [`Expr::FnCall`] to cover the very common pattern of a single @@ -1384,14 +1384,14 @@ pub struct BinaryExpr { /// # Volatile Data Structure /// /// This type is volatile and may change. -#[derive(Debug, Clone, Eq, PartialEq, Hash)] -pub struct OpAssignment { +#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] +pub struct OpAssignment<'a> { pub hash_op_assign: u64, pub hash_op: u64, - pub op: &'static str, + pub op: &'a str, } -impl OpAssignment { +impl OpAssignment<'_> { /// Create a new [`OpAssignment`]. /// /// # Panics diff --git a/src/engine.rs b/src/engine.rs index fd508a75..905d7e5f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -437,34 +437,11 @@ impl<'a> Target<'a> { #[inline(always)] pub fn propagate_changed_value(&mut self) -> Result<(), Box> { match self { - Self::RefMut(_) | Self::TempValue(_) => Ok(()), + Self::RefMut(_) | Self::TempValue(_) => (), #[cfg(not(feature = "no_closure"))] - Self::LockGuard(_) => Ok(()), + Self::LockGuard(_) => (), #[cfg(not(feature = "no_index"))] - Self::BitField(_, _, value) => { - let new_val = value.clone(); - self.set_value(new_val) - } - #[cfg(not(feature = "no_index"))] - Self::StringChar(_, _, ch) => { - let new_val = ch.clone(); - self.set_value(new_val) - } - } - } - /// Update the value of the `Target`. - fn set_value(&mut self, new_val: Dynamic) -> Result<(), Box> { - match self { - Self::RefMut(r) => **r = new_val, - #[cfg(not(feature = "no_closure"))] - Self::LockGuard((r, _)) => **r = new_val, - Self::TempValue(_) => panic!("cannot update a value"), - #[cfg(not(feature = "no_index"))] - Self::BitField(value, index, _) => { - let value = &mut *value - .write_lock::() - .expect("never fails because `BitField` always holds an `INT`"); - + Self::BitField(value, index, new_val) => { // Replace the bit at the specified index position let new_bit = new_val.as_bool().map_err(|err| { Box::new(EvalAltResult::ErrorMismatchDataType( @@ -474,6 +451,10 @@ impl<'a> Target<'a> { )) })?; + let value = &mut *value + .write_lock::() + .expect("never fails because `BitField` always holds an `INT`"); + let index = *index; if index < std::mem::size_of_val(value) * 8 { @@ -483,14 +464,12 @@ impl<'a> Target<'a> { } else { *value &= !mask; } + } else { + unreachable!("bit-field index out of bounds: {}", index); } } #[cfg(not(feature = "no_index"))] - Self::StringChar(s, index, _) => { - let s = &mut *s - .write_lock::() - .expect("never fails because `StringChar` always holds an `ImmutableString`"); - + Self::StringChar(s, index, new_val) => { // Replace the character at the specified index position let new_ch = new_val.as_char().map_err(|err| { Box::new(EvalAltResult::ErrorMismatchDataType( @@ -500,6 +479,10 @@ impl<'a> Target<'a> { )) })?; + let s = &mut *s + .write_lock::() + .expect("never fails because `StringChar` always holds an `ImmutableString`"); + let index = *index; *s = s @@ -1236,8 +1219,6 @@ impl Engine { let ((new_val, new_pos), (op_info, op_pos)) = new_val.expect("never fails because `new_val` is `Some`"); let idx_val = idx_val.as_index_value(); - - #[cfg(not(feature = "no_index"))] let mut idx_val_for_setter = idx_val.clone(); let try_setter = match self.get_indexed_mut( @@ -1310,13 +1291,13 @@ impl Engine { // {xxx:map}.id op= ??? Expr::Property(x) if target.is::() && new_val.is_some() => { let (name, pos) = &x.2; + let ((new_val, new_pos), (op_info, op_pos)) = + new_val.expect("never fails because `new_val` is `Some`"); let index = name.into(); { let mut val = self.get_indexed_mut( mods, state, lib, target, index, *pos, true, false, level, )?; - let ((new_val, new_pos), (op_info, op_pos)) = - new_val.expect("never fails because `new_val` is `Some`"); self.eval_op_assignment( mods, state, lib, op_info, op_pos, &mut val, root, new_val, ) @@ -2355,7 +2336,7 @@ impl Engine { mods, state, lib, - op_info.clone(), + *op_info, *op_pos, &mut lhs_ptr, (var_name, pos), @@ -2363,8 +2344,10 @@ impl Engine { ) .map_err(|err| err.fill_position(rhs_expr.position()))?; - self.check_data_size(lhs_ptr.as_ref()) - .map_err(|err| err.fill_position(lhs_expr.position()))?; + if op_info.is_some() { + self.check_data_size(lhs_ptr.as_ref()) + .map_err(|err| err.fill_position(lhs_expr.position()))?; + } Ok(Dynamic::UNIT) } @@ -2960,55 +2943,52 @@ impl Engine { result.and_then(|r| self.check_data_size(&r).map(|_| r)) } + #[cfg(feature = "unchecked")] + #[inline(always)] + fn check_data_size(&self, _value: &Dynamic) -> Result<(), Box> { + Ok(()) + } + + #[cfg(not(feature = "unchecked"))] fn check_data_size(&self, value: &Dynamic) -> Result<(), Box> { // Recursively calculate the size of a value (especially `Array` and `Map`) fn calc_size(value: &Dynamic) -> (usize, usize, usize) { - match value { + match value.0 { #[cfg(not(feature = "no_index"))] - Dynamic(Union::Array(arr, _, _)) => { - let mut arrays = 0; - let mut maps = 0; - - arr.iter().for_each(|value| match value { - Dynamic(Union::Array(_, _, _)) => { - let (a, m, _) = calc_size(value); - arrays += a; - maps += m; - } - #[cfg(not(feature = "no_object"))] - Dynamic(Union::Map(_, _, _)) => { - let (a, m, _) = calc_size(value); - arrays += a; - maps += m; - } - _ => arrays += 1, - }); - - (arrays, maps, 0) + Union::Array(ref arr, _, _) => { + arr.iter() + .fold((0, 0, 0), |(arrays, maps, strings), value| match value.0 { + Union::Array(_, _, _) => { + let (a, m, s) = calc_size(value); + (arrays + a + 1, maps + m, strings + s) + } + #[cfg(not(feature = "no_object"))] + Union::Map(_, _, _) => { + let (a, m, s) = calc_size(value); + (arrays + a + 1, maps + m, strings + s) + } + Union::Str(ref s, _, _) => (arrays + 1, maps, strings + s.len()), + _ => (arrays + 1, maps, strings), + }) } #[cfg(not(feature = "no_object"))] - Dynamic(Union::Map(map, _, _)) => { - let mut arrays = 0; - let mut maps = 0; - - map.values().for_each(|value| match value { - #[cfg(not(feature = "no_index"))] - Dynamic(Union::Array(_, _, _)) => { - let (a, m, _) = calc_size(value); - arrays += a; - maps += m; - } - Dynamic(Union::Map(_, _, _)) => { - let (a, m, _) = calc_size(value); - arrays += a; - maps += m; - } - _ => maps += 1, - }); - - (arrays, maps, 0) + Union::Map(ref map, _, _) => { + map.values() + .fold((0, 0, 0), |(arrays, maps, strings), value| match value.0 { + #[cfg(not(feature = "no_index"))] + Union::Array(_, _, _) => { + let (a, m, s) = calc_size(value); + (arrays + a, maps + m + 1, strings + s) + } + Union::Map(_, _, _) => { + let (a, m, s) = calc_size(value); + (arrays + a, maps + m + 1, strings + s) + } + Union::Str(ref s, _, _) => (arrays, maps + 1, strings + s.len()), + _ => (arrays, maps + 1, strings), + }) } - Dynamic(Union::Str(s, _, _)) => (0, 0, s.len()), + Union::Str(ref s, _, _) => (0, 0, s.len()), _ => (0, 0, 0), } } diff --git a/src/optimize.rs b/src/optimize.rs index 7e1d6d40..da14b1d2 100644 --- a/src/optimize.rs +++ b/src/optimize.rs @@ -297,7 +297,7 @@ fn optimize_stmt_block( Stmt::Noop(*pos) }; } - [.., second_last_stmt, Stmt::Noop(_)] if second_last_stmt.returns_value() => {} + [.., second_last_stmt, Stmt::Noop(_)] if second_last_stmt.returns_value() => (), [.., second_last_stmt, last_stmt] if !last_stmt.returns_value() && is_pure(last_stmt) => { diff --git a/src/token.rs b/src/token.rs index 992f0601..ad72253c 100644 --- a/src/token.rs +++ b/src/token.rs @@ -1095,7 +1095,7 @@ pub fn parse_string_literal( match next_char { // \r - ignore if followed by \n - '\r' if stream.peek_next().map(|ch| ch == '\n').unwrap_or(false) => {} + '\r' if stream.peek_next().map(|ch| ch == '\n').unwrap_or(false) => (), // \... '\\' if !verbatim && escape.is_empty() => { escape.push('\\'); diff --git a/tests/data_size.rs b/tests/data_size.rs index f3ff3cd6..aa92c91b 100644 --- a/tests/data_size.rs +++ b/tests/data_size.rs @@ -206,6 +206,18 @@ fn test_max_map_size() -> Result<(), Box> { ) ); + assert!(matches!( + *engine + .consume( + " + let x = #{}; + loop { x.a = x; } + " + ) + .expect_err("should error"), + EvalAltResult::ErrorDataTooLarge(_, _) + )); + assert!(matches!( *engine .eval::(