From b86c87253bc0137a0d8e9a03799bc811e6710e03 Mon Sep 17 00:00:00 2001 From: Stephen Chung Date: Sun, 2 Aug 2020 13:33:51 +0800 Subject: [PATCH] Prevent data races. --- src/any.rs | 32 ++++++++++++++++++++++++++++++++ src/api.rs | 6 ++++++ src/engine.rs | 15 ++++++++++----- src/fn_call.rs | 30 ++++++++++++++++++++++++++++++ src/result.rs | 8 +++++++- tests/shared.rs | 39 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 124 insertions(+), 6 deletions(-) diff --git a/src/any.rs b/src/any.rs index 6280e426..e0281efe 100644 --- a/src/any.rs +++ b/src/any.rs @@ -282,6 +282,11 @@ impl Dynamic { } /// Get the TypeId of the value held by this `Dynamic`. + /// + /// # Panics and Deadlocks When Value is Shared + /// + /// Under the `sync` feature, this call may deadlock. + /// Otherwise, this call panics if the data is currently borrowed for write. pub fn type_id(&self) -> TypeId { match &self.0 { Union::Unit(_) => TypeId::of::<()>(), @@ -307,6 +312,11 @@ impl Dynamic { } /// Get the name of the type of the value held by this `Dynamic`. + /// + /// # Panics and Deadlocks When Value is Shared + /// + /// Under the `sync` feature, this call may deadlock. + /// Otherwise, this call panics if the data is currently borrowed for write. pub fn type_name(&self) -> &'static str { match &self.0 { Union::Unit(_) => "()", @@ -752,6 +762,28 @@ impl Dynamic { } } + /// Is the `Dynamic` a shared value that is locked? + /// + /// ## Note + /// + /// Under the `sync` feature, shared values use `RwLock` and they are never locked. + /// Access just waits until the `RwLock` is released. + /// So this method always returns `false` under `sync`. + #[inline(always)] + pub fn is_locked(&self) -> bool { + match self.0 { + #[cfg(not(feature = "no_shared"))] + Union::Shared(ref _cell) => { + #[cfg(not(feature = "sync"))] + return _cell.try_borrow().is_err(); + + #[cfg(feature = "sync")] + return false; + } + _ => false, + } + } + /// Get a reference of a specific type to the `Dynamic`. /// Casting to `Dynamic` just returns a reference to it. /// diff --git a/src/api.rs b/src/api.rs index e6e9ea7a..be69f53d 100644 --- a/src/api.rs +++ b/src/api.rs @@ -3,6 +3,7 @@ use crate::any::{Dynamic, Variant}; use crate::engine::{Engine, Imports, State}; use crate::error::ParseError; +use crate::fn_call::ensure_no_data_race; use crate::fn_native::{IteratorFn, SendSync}; use crate::module::{FuncReturn, Module}; use crate::optimize::{optimize_into_ast, OptimizationLevel}; @@ -1282,6 +1283,11 @@ impl Engine { let mut mods = Imports::new(); let args = args.as_mut(); + // Check for data race. + if cfg!(not(feature = "no_shared")) { + ensure_no_data_race(name, args, false)?; + } + self.call_script_fn( scope, &mut mods, &mut state, lib, this_ptr, name, fn_def, args, 0, ) diff --git a/src/engine.rs b/src/engine.rs index 24a1b20b..b0bd5e8b 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -41,14 +41,11 @@ use crate::stdlib::{ collections::{HashMap, HashSet}, fmt, format, iter::{empty, once}, + ops::DerefMut, string::{String, ToString}, vec::Vec, }; -#[cfg(not(feature = "no_shared"))] -#[cfg(not(feature = "no_object"))] -use crate::stdlib::ops::DerefMut; - #[cfg(not(feature = "no_index"))] use crate::stdlib::any::TypeId; @@ -561,7 +558,8 @@ pub fn search_imports_mut<'s>( }) } -/// Search for a variable within the scope and imports +/// Search for a variable within the scope or within imports, +/// depending on whether the variable name is qualified. pub fn search_namespace<'s, 'a>( scope: &'s mut Scope, mods: &'s mut Imports, @@ -631,6 +629,13 @@ pub fn search_scope_only<'s, 'a>( }; let (val, typ) = scope.get_mut(index); + + // Check for data race - probably not necessary because the only place it should conflict is in a method call + // when the object variable is also used as a parameter. + // if cfg!(not(feature = "no_shared")) && val.is_locked() { + // return Err(Box::new(EvalAltResult::ErrorDataRace(name.into(), *pos))); + // } + Ok((val, name, typ, *pos)) } diff --git a/src/fn_call.rs b/src/fn_call.rs index ace08128..2b4ffb65 100644 --- a/src/fn_call.rs +++ b/src/fn_call.rs @@ -160,6 +160,31 @@ fn add_captured_variables_into_scope<'s>( ); } +#[inline(always)] +pub fn ensure_no_data_race( + fn_name: &str, + args: &FnCallArgs, + is_ref: bool, +) -> Result<(), Box> { + if cfg!(not(feature = "no_shared")) { + let skip = if is_ref { 1 } else { 0 }; + + if let Some((n, _)) = args + .iter() + .skip(skip) + .enumerate() + .find(|(_, a)| a.is_locked()) + { + return Err(Box::new(EvalAltResult::ErrorDataRace( + format!("argument #{} of function '{}'", n + 1 + skip, fn_name), + Position::none(), + ))); + } + } + + Ok(()) +} + impl Engine { /// Call a native Rust function registered with the `Engine`. /// Position in `EvalAltResult` is `None` and must be set afterwards. @@ -430,6 +455,11 @@ impl Engine { def_val: Option, level: usize, ) -> Result<(Dynamic, bool), Box> { + // Check for data race. + if cfg!(not(feature = "no_shared")) { + ensure_no_data_race(fn_name, args, is_ref)?; + } + // Qualifiers (none) + function name + number of arguments + argument `TypeId`'s. let arg_types = args.iter().map(|a| a.type_id()); let hash_fn = calc_fn_hash(empty(), fn_name, args.len(), arg_types); diff --git a/src/result.rs b/src/result.rs index c0cc620d..cecd14bd 100644 --- a/src/result.rs +++ b/src/result.rs @@ -69,6 +69,8 @@ pub enum EvalAltResult { ErrorVariableNotFound(String, Position), /// Usage of an unknown module. Wrapped value is the name of the module. ErrorModuleNotFound(String, Position), + /// Data race detected when accessing a variable. Wrapped value is the name of the variable. + ErrorDataRace(String, Position), /// Assignment to an inappropriate LHS (left-hand-side) expression. ErrorAssignmentToUnknownLHS(Position), /// Assignment to a constant variable. @@ -136,7 +138,8 @@ impl EvalAltResult { Self::ErrorLogicGuard(_) => "Boolean value expected", Self::ErrorFor(_) => "For loop expects an array, object map, or range", Self::ErrorVariableNotFound(_, _) => "Variable not found", - Self::ErrorModuleNotFound(_, _) => "module not found", + Self::ErrorModuleNotFound(_, _) => "Module not found", + Self::ErrorDataRace(_, _) => "Data race detected when accessing variable", Self::ErrorAssignmentToUnknownLHS(_) => { "Assignment to an unsupported left-hand side expression" } @@ -180,6 +183,7 @@ impl fmt::Display for EvalAltResult { Self::ErrorFunctionNotFound(s, _) | Self::ErrorVariableNotFound(s, _) + | Self::ErrorDataRace(s, _) | Self::ErrorModuleNotFound(s, _) => write!(f, "{}: '{}'", desc, s)?, Self::ErrorDotExpr(s, _) if !s.is_empty() => write!(f, "{}", s)?, @@ -289,6 +293,7 @@ impl EvalAltResult { | Self::ErrorFor(pos) | Self::ErrorVariableNotFound(_, pos) | Self::ErrorModuleNotFound(_, pos) + | Self::ErrorDataRace(_, pos) | Self::ErrorAssignmentToUnknownLHS(pos) | Self::ErrorAssignmentToConstant(_, pos) | Self::ErrorMismatchOutputType(_, _, pos) @@ -329,6 +334,7 @@ impl EvalAltResult { | Self::ErrorFor(pos) | Self::ErrorVariableNotFound(_, pos) | Self::ErrorModuleNotFound(_, pos) + | Self::ErrorDataRace(_, pos) | Self::ErrorAssignmentToUnknownLHS(pos) | Self::ErrorAssignmentToConstant(_, pos) | Self::ErrorMismatchOutputType(_, _, pos) diff --git a/tests/shared.rs b/tests/shared.rs index 8223c876..b2238fbf 100644 --- a/tests/shared.rs +++ b/tests/shared.rs @@ -283,3 +283,42 @@ fn test_shared() -> Result<(), Box> { Ok(()) } + +#[test] +fn test_shared_data_race() -> Result<(), Box> { + let engine = Engine::new(); + + #[cfg(not(feature = "no_function"))] + #[cfg(not(feature = "no_object"))] + { + assert_eq!( + engine.eval::( + r#" + fn foo(x) { this += x }; + + let a = shared(41); + a.foo(1); + a + "# + )?, + 42 + ); + + assert!(matches!( + *engine + .eval::( + r#" + fn foo(x) { this += x }; + + let a = shared(42); + a.foo(a); + a + "# + ) + .expect_err("should error"), + EvalAltResult::ErrorDataRace(_, _) + )); + } + + Ok(()) +}