From 01664ef7ee59a123a99f60dc106397942b27bf6f Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Thu, 4 Mar 2021 23:47:52 +0800 Subject: [PATCH] Fix range overflow panics. --- CHANGELOG.md | 2 + Cargo.toml | 6 +- README.md | 2 +- src/packages/iter_basic.rs | 203 +++++++++++++++++++++++++++++-------- src/packages/logic.rs | 3 - src/result.rs | 4 +- tests/for.rs | 77 +++++++++++++- 7 files changed, 241 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26ccf297..dcd5265f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Bug fixes * Errors in native Rust functions now contain the correct function call positions. * Fixed error types in `EvalAltResult::ErrorMismatchDataType` which were swapped. * Some expressions involving shared variables now work properly, for example `x in shared_value`, `return shared_value`, `obj.field = shared_value` etc. Previously, the resultant value is still shared which is counter-intuitive. +* Potential overflow panics in `range(from, to, step)` is fixed. Breaking changes ---------------- @@ -23,6 +24,7 @@ Breaking changes * Function keywords (e.g. `type_of`, `eval`, `Fn`) can no longer be overloaded. It is more trouble than worth. To disable these keywords, use `Engine::disable_symbol`. * `is_def_var` and `is_def_fn` are now reserved keywords. * `Engine::id` field is removed. +* `num-traits` is now a required dependency. Enhancements ------------ diff --git a/Cargo.toml b/Cargo.toml index a334d795..9ce632c5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,6 +26,7 @@ categories = [ "no-std", "embedded", "wasm", "parser-implementations" ] [dependencies] smallvec = { version = "1.6", default-features = false, features = ["union"] } ahash = { version = "0.6", default-features = false } +num-traits = { version = "0.2", default_features = false } rhai_codegen = { version = "0.3.3", path = "codegen" } [features] @@ -65,11 +66,6 @@ version = "0.2" default_features = false optional = true -[dependencies.num-traits] -version = "0.2" -default-features = false -optional = true - [dependencies.core-error] version = "0.0" default_features = false diff --git a/README.md b/README.md index 1bfd1806..8d2b90e6 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ Standard features * Built-in support for most common [data types](https://rhai.rs/book/language/values-and-types.html) including booleans, integers, floating-point numbers (including [`Decimal`](https://crates.io/crates/rust_decimal)), strings, Unicode characters, arrays and maps. * Easily [call a script-defined function](https://rhai.rs/book/engine/call-fn.html) from Rust. * Relatively little `unsafe` code (yes there are some for performance reasons). -* Few dependencies (currently only [`smallvec`](https://crates.io/crates/smallvec) and [`ahash`](https://crates.io/crates/ahash)). +* Few dependencies (currently only [`smallvec`](https://crates.io/crates/smallvec), [`num-traits`](https://crates.io/crates/num-traits) and [`ahash`](https://crates.io/crates/ahash)). * Re-entrant scripting engine can be made `Send + Sync` (via the `sync` feature). * Compile once to [AST](https://rhai.rs/book/engine/compile.html) form for repeated evaluations. * Scripts are [optimized](https://rhai.rs/book/engine/optimize.html) (useful for template-based machine-generated scripts). diff --git a/src/packages/iter_basic.rs b/src/packages/iter_basic.rs index e8ce703c..5a62821b 100644 --- a/src/packages/iter_basic.rs +++ b/src/packages/iter_basic.rs @@ -1,10 +1,18 @@ use crate::dynamic::Variant; -use crate::stdlib::{ - boxed::Box, - ops::{Add, Range, Sub}, - string::ToString, -}; -use crate::{def_package, EvalAltResult, Position, INT}; +use crate::stdlib::{boxed::Box, ops::Range}; +use crate::{def_package, EvalAltResult, INT}; + +#[cfg(not(feature = "unchecked"))] +use crate::stdlib::string::ToString; + +#[cfg(feature = "decimal")] +use rust_decimal::Decimal; + +#[cfg(not(feature = "unchecked"))] +use num_traits::{CheckedAdd as Add, CheckedSub as Sub}; + +#[cfg(feature = "unchecked")] +use crate::stdlib::ops::{Add, Sub}; fn get_range(from: T, to: T) -> Result, Box> { Ok(from..to) @@ -14,30 +22,35 @@ fn get_range(from: T, to: T) -> Result, Box(T, T, T) where - for<'a> &'a T: Add<&'a T, Output = T>, - T: Variant + Clone + PartialOrd; + T: Variant + Copy + PartialOrd + Add + Sub; impl StepRange where - for<'a> &'a T: Add<&'a T, Output = T> + Sub<&'a T, Output = T>, - T: Variant + Clone + PartialOrd, + T: Variant + Copy + PartialOrd + Add + Sub, { pub fn new(from: T, to: T, step: T) -> Result> { - if &from + &step == from { - Err(Box::new(EvalAltResult::ErrorArithmetic( - "step value cannot be zero".to_string(), - Position::NONE, - ))) - } else { - Ok(Self(from, to, step)) + #[cfg(not(feature = "unchecked"))] + if let Some(r) = from.checked_add(&step) { + if r == from { + return Err(Box::new(EvalAltResult::ErrorInFunctionCall( + "range".to_string(), + "".to_string(), + Box::new(EvalAltResult::ErrorArithmetic( + "step value cannot be zero".to_string(), + crate::Position::NONE, + )), + crate::Position::NONE, + ))); + } } + + Ok(Self(from, to, step)) } } impl Iterator for StepRange where - for<'a> &'a T: Add<&'a T, Output = T> + Sub<&'a T, Output = T>, - T: Variant + Clone + PartialOrd, + T: Variant + Copy + PartialOrd + Add + Sub, { type Item = T; @@ -45,31 +58,75 @@ where if self.0 == self.1 { None } else if self.0 < self.1 { - let diff1 = &self.1 - &self.0; + #[cfg(not(feature = "unchecked"))] + let diff1 = if let Some(diff) = self.1.checked_sub(&self.0) { + diff + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let diff1 = self.1 - self.0; - let v = self.0.clone(); - let n = self.0.add(&self.2); + let v = self.0; - let diff2 = &self.1 - &n; + #[cfg(not(feature = "unchecked"))] + let n = if let Some(num) = self.0.checked_add(&self.2) { + num + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let n = self.0 + self.2; + + #[cfg(not(feature = "unchecked"))] + let diff2 = if let Some(diff) = self.1.checked_sub(&n) { + diff + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let diff2 = self.1 - n; if diff2 >= diff1 { None } else { - self.0 = if n >= self.1 { self.1.clone() } else { n }; + self.0 = if n >= self.1 { self.1 } else { n }; Some(v) } } else { - let diff1 = &self.0 - &self.1; + #[cfg(not(feature = "unchecked"))] + let diff1 = if let Some(diff) = self.0.checked_sub(&self.1) { + diff + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let diff1 = self.0 - self.1; - let v = self.0.clone(); - let n = self.0.add(&self.2); + let v = self.0; - let diff2 = &n - &self.1; + #[cfg(not(feature = "unchecked"))] + let n = if let Some(num) = self.0.checked_add(&self.2) { + num + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let n = self.0 + self.2; + + #[cfg(not(feature = "unchecked"))] + let diff2 = if let Some(diff) = n.checked_sub(&self.1) { + diff + } else { + return None; + }; + #[cfg(feature = "unchecked")] + let diff2 = n - self.1; if diff2 >= diff1 { None } else { - self.0 = if n <= self.1 { self.1.clone() } else { n }; + self.0 = if n <= self.1 { self.1 } else { n }; Some(v) } } @@ -78,14 +135,13 @@ where fn get_step_range(from: T, to: T, step: T) -> Result, Box> where - for<'a> &'a T: Add<&'a T, Output = T> + Sub<&'a T, Output = T>, - T: Variant + Clone + PartialOrd, + T: Variant + Copy + PartialOrd + Add + Sub, { StepRange::::new(from, to, step) } macro_rules! reg_range { - ($lib:expr, $x:expr, $( $y:ty ),*) => ( + ($lib:ident | $x:expr => $( $y:ty ),*) => { $( $lib.set_iterator::>(); let hash = $lib.set_fn_2($x, get_range::<$y>); @@ -95,11 +151,8 @@ macro_rules! reg_range { concat!("Iterator") ]); )* - ) -} - -macro_rules! reg_stepped_range { - ($lib:expr, $x:expr, $( $y:ty ),*) => ( + }; + ($lib:ident | step $x:expr => $( $y:ty ),*) => { $( $lib.set_iterator::>(); let hash = $lib.set_fn_3($x, get_step_range::<$y>); @@ -110,31 +163,93 @@ macro_rules! reg_stepped_range { concat!("Iterator") ]); )* - ) + }; } def_package!(crate:BasicIteratorPackage:"Basic range iterators.", lib, { - reg_range!(lib, "range", INT); + reg_range!(lib | "range" => INT); #[cfg(not(feature = "only_i32"))] #[cfg(not(feature = "only_i64"))] { - reg_range!(lib, "range", i8, u8, i16, u16, i32, u32, i64, u64); + reg_range!(lib | "range" => i8, u8, i16, u16, i32, u32, i64, u64); if cfg!(not(target_arch = "wasm32")) { - reg_range!(lib, "range", i128, u128); + reg_range!(lib | "range" => i128, u128); } } - reg_stepped_range!(lib, "range", INT); + reg_range!(lib | step "range" => INT); #[cfg(not(feature = "only_i32"))] #[cfg(not(feature = "only_i64"))] { - reg_stepped_range!(lib, "range", i8, u8, i16, u16, i32, u32, i64, u64); + reg_range!(lib | step "range" => i8, u8, i16, u16, i32, u32, i64, u64); if cfg!(not(target_arch = "wasm32")) { - reg_stepped_range!(lib, "range", i128, u128); + reg_range!(lib | step "range" => i128, u128); } } + + #[cfg(feature = "decimal")] + { + #[derive(Debug, Clone, Copy, Hash, Eq, PartialEq)] + struct StepDecimalRange(Decimal, Decimal, Decimal); + + impl StepDecimalRange { + pub fn new(from: Decimal, to: Decimal, step: Decimal) -> Result> { + #[cfg(not(feature = "unchecked"))] + if step.is_zero() { + use crate::stdlib::string::ToString; + + return Err(Box::new(EvalAltResult::ErrorInFunctionCall("range".to_string(), "".to_string(), + Box::new(EvalAltResult::ErrorArithmetic("step value cannot be zero".to_string(), crate::Position::NONE)), + crate::Position::NONE, + ))); + } + + Ok(Self(from, to, step)) + } + } + + impl Iterator for StepDecimalRange { + type Item = Decimal; + + fn next(&mut self) -> Option { + if self.0 == self.1 { + None + } else if self.0 < self.1 { + #[cfg(not(feature = "unchecked"))] + if self.2.is_sign_negative() { + return None; + } + + let v = self.0; + let n = self.0 + self.2; + + self.0 = if n >= self.1 { self.1 } else { n }; + Some(v) + } else { + #[cfg(not(feature = "unchecked"))] + if self.2.is_sign_positive() { + return None; + } + + let v = self.0; + let n = self.0 + self.2; + + self.0 = if n <= self.1 { self.1 } else { n }; + Some(v) + } + } + } + + lib.set_iterator::(); + + let hash = lib.set_fn_2("range", |from, to| StepDecimalRange::new(from, to, Decimal::one())); + lib.update_fn_metadata(hash, &["from: Decimal", "to: Decimal", "Iterator"]); + + let hash = lib.set_fn_3("range", |from, to, step| StepDecimalRange::new(from, to, step)); + lib.update_fn_metadata(hash, &["from: Decimal", "to: Decimal", "step: Decimal", "Iterator"]); + } }); diff --git a/src/packages/logic.rs b/src/packages/logic.rs index 3f0ff0da..6a2cbe68 100644 --- a/src/packages/logic.rs +++ b/src/packages/logic.rs @@ -3,9 +3,6 @@ use crate::def_package; use crate::plugin::*; -#[cfg(feature = "decimal")] -use rust_decimal::Decimal; - #[cfg(any( not(feature = "no_float"), all(not(feature = "only_i32"), not(feature = "only_i64")) diff --git a/src/result.rs b/src/result.rs index 1e6ec07b..1376dbcb 100644 --- a/src/result.rs +++ b/src/result.rs @@ -153,13 +153,13 @@ impl fmt::Display for EvalAltResult { #[cfg(not(feature = "no_function"))] Self::ErrorInFunctionCall(s, src, err, _) if crate::engine::is_anonymous_fn(s) => { - write!(f, "{}, in call to closure", err)?; + write!(f, "{} in call to closure", err)?; if !src.is_empty() { write!(f, " @ '{}'", src)?; } } Self::ErrorInFunctionCall(s, src, err, _) => { - write!(f, "{}, in call to function {}", err, s)?; + write!(f, "{} in call to function {}", err, s)?; if !src.is_empty() { write!(f, " @ '{}'", src)?; } diff --git a/tests/for.rs b/tests/for.rs index 03a40a89..8827c123 100644 --- a/tests/for.rs +++ b/tests/for.rs @@ -2,7 +2,7 @@ use rhai::{Engine, EvalAltResult, Module, INT}; #[cfg(not(feature = "no_index"))] #[test] -fn test_for_array() -> Result<(), Box> { +fn test_for() -> Result<(), Box> { let engine = Engine::new(); let script = r" @@ -27,6 +27,81 @@ fn test_for_array() -> Result<(), Box> { assert_eq!(engine.eval::(script)?, 35); + assert_eq!( + engine.eval::( + r" + let sum = 0; + for x in range(1, 10, 2) { sum += x; } + sum + " + )?, + 25 + ); + + assert_eq!( + engine.eval::( + r" + let sum = 0; + for x in range(10, 1, 2) { sum += x; } + sum + " + )?, + 0 + ); + + assert_eq!( + engine.eval::( + r" + let sum = 0; + for x in range(1, 10, -2) { sum += x; } + sum + " + )?, + 0 + ); + + assert_eq!( + engine.eval::( + r" + let sum = 0; + for x in range(10, 1, -2) { sum += x; } + sum + " + )?, + 30 + ); + + Ok(()) +} + +#[cfg(not(feature = "unchecked"))] +#[test] +fn test_for_overflow() -> Result<(), Box> { + let engine = Engine::new(); + + #[cfg(not(feature = "only_i32"))] + let script = r" + let sum = 0; + + for x in range(9223372036854775807, 0, 9223372036854775807) { + sum += 1; + } + + sum + "; + #[cfg(feature = "only_i32")] + let script = r" + let sum = 0; + + for x in range(2147483647 , 0, 2147483647 ) { + sum += 1; + } + + sum + "; + + assert_eq!(engine.eval::(script)?, 0); + Ok(()) }