From 565099707960de9891f27756327e2cdd34eefc5e Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 17 Jan 2018 18:12:06 +0100 Subject: [PATCH 1/3] fix unsafety hole in environmental using function --- environmental/src/lib.rs | 80 ++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/environmental/src/lib.rs b/environmental/src/lib.rs index 239a2fb90f07b..18dc10d10f77f 100644 --- a/environmental/src/lib.rs +++ b/environmental/src/lib.rs @@ -44,10 +44,12 @@ pub use std::cell::RefCell; use std::thread::LocalKey; +// invoking this function asserts that T and S are both instantiations +// of the same lifetime-parametric type. #[doc(hidden)] -pub fn using<'a, T: 'a, R, S, F: FnOnce() -> R>( +pub unsafe fn using R>( global: &'static LocalKey>, - protected: &'a mut T, + protected: &mut T, f: F ) -> R { // store the `protected` reference as a pointer so we can provide it to logic running within @@ -57,19 +59,22 @@ pub fn using<'a, T: 'a, R, S, F: FnOnce() -> R>( // - that no other thread will use it; and // - that we do not use the original mutating reference while the pointer. // exists. - let original = global.with(|r| { - let mut b = r.borrow_mut(); - let o = *b; - *b = protected as *mut T as *mut S; - o - }); - let r = f(); - global.with(|r| *r.borrow_mut() = original); - r + global.with(|r| { + let original = { + let mut global = r.borrow_mut(); + ::std::mem::replace(&mut *global, protected as *mut _ as *mut S) + }; + + let res = f(); + *r.borrow_mut() = original; + res + }) } +// invoking this function asserts that T and S are both instantiations +// of the same lifetime-parametric type. #[doc(hidden)] -pub fn with<'r, R, S, T: 'r, F: FnOnce(&'r mut T) -> R>( +pub unsafe fn with R>( global: &'static LocalKey>, mutator: F, ) -> Option { @@ -78,9 +83,7 @@ pub fn with<'r, R, S, T: 'r, F: FnOnce(&'r mut T) -> R>( if *br != 0 as *mut S { // safe because it's only non-zero when it's being called from using, which // is holding on to the underlying reference (and not using it itself) safely. - unsafe { - Some(mutator(&mut *(*br as *mut S as *mut T))) - } + Some(mutator(&mut *(*br as *mut S as *mut T))) } else { None } @@ -130,32 +133,24 @@ macro_rules! decl { /// ``` #[macro_export] macro_rules! declare_simple { - ($name:ident : $t:tt) => { + ($name:ident : $t:ty) => { mod $name { #[allow(unused_imports)] use super::*; decl!(GLOBAL: $t); - pub fn using<'a: 'b, 'b, R, F: FnOnce() -> R, T: 'a>( - protected: &'b mut T, + pub fn using R>( + protected: &mut $t, f: F ) -> R { - $crate::using(&GLOBAL, protected, f) + unsafe { $crate::using(&GLOBAL, protected, f) } } - pub fn with FnOnce(&'r mut $t) -> R>( + pub fn with R>( f: F ) -> Option { - let dummy = (); - with_closed(f, &dummy) - } - - fn with_closed<'d: 'r, 'r, R, F: FnOnce(&'r mut $t) -> R>( - f: F, - _dummy: &'d (), - ) -> Option { - $crate::with(&GLOBAL, f) + unsafe { $crate::with(&GLOBAL, f) } } } } @@ -230,20 +225,33 @@ macro_rules! declare_generic { #[cfg(test)] mod tests { - declare_simple!(counter: u32); - - fn stuff() { - // do some stuff, accessing the named reference as desired. - counter::with(|value| *value += 1); - } #[test] fn simple_works() { + declare_simple!(counter: u32); + + let stuff = || counter::with(|value| *value += 1); + // declare a stack variable of the same type as our global declaration. let mut local = 41u32; + // call stuff, setting up our `counter` environment as a refrence to our local counter var. counter::using(&mut local, stuff); - println!("The answer is {:?}", local); // will print 42! + assert_eq!(local, 42); + stuff(); // safe! doesn't do anything. } + + #[test] + fn overwrite_with_lesser_lifetime() { + declare_simple!(items: Vec); + + let mut local_items = vec![1, 2, 3]; + items::using(&mut local_items, || { + let dies_at_end = vec![4, 5, 6]; + items::with(|items| *items = dies_at_end); + }); + + assert_eq!(local_items, vec![4, 5, 6]); + } } From cdc1387e666a2bed9d83f596705921994e9d6e18 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 17 Jan 2018 19:20:38 +0100 Subject: [PATCH 2/3] fix unsafety hole around unwinding --- environmental/src/lib.rs | 239 +++++++++++++++++++++------------------ 1 file changed, 127 insertions(+), 112 deletions(-) diff --git a/environmental/src/lib.rs b/environmental/src/lib.rs index 18dc10d10f77f..ef5b5967bea9f 100644 --- a/environmental/src/lib.rs +++ b/environmental/src/lib.rs @@ -16,7 +16,7 @@ //! Safe global references to stack variables. //! -//! Set up a global reference with declare_simple! macro giving it a name and type. +//! Set up a global reference with environmental! macro giving it a name and type. //! Use the `using` function scoped under its name to name a reference and call a function that //! takes no parameters yet can access said reference through the similarly placed `with` function. //! @@ -25,7 +25,7 @@ //! ``` //! #[macro_use] extern crate environmental; //! // create a place for the global reference to exist. -//! declare_simple!(counter: u32); +//! environmental!(counter: u32); //! fn stuff() { //! // do some stuff, accessing the named reference as desired. //! counter::with(|i| *i += 1); @@ -40,15 +40,12 @@ //! } //! ``` -#[doc(hidden)] -pub use std::cell::RefCell; +use std::cell::RefCell; use std::thread::LocalKey; -// invoking this function asserts that T and S are both instantiations -// of the same lifetime-parametric type. #[doc(hidden)] -pub unsafe fn using R>( - global: &'static LocalKey>, +pub fn using R>( + global: &'static LocalKey>>, protected: &mut T, f: F ) -> R { @@ -62,60 +59,66 @@ pub unsafe fn using R>( global.with(|r| { let original = { let mut global = r.borrow_mut(); - ::std::mem::replace(&mut *global, protected as *mut _ as *mut S) + ::std::mem::replace(&mut *global, Some(protected as _)) }; - let res = f(); - *r.borrow_mut() = original; - res + // even if `f` panics the original will be replaced. + struct ReplaceOriginal<'a, T: 'a + ?Sized> { + original: Option<*mut T>, + global: &'a RefCell>, + } + + impl<'a, T: 'a + ?Sized> Drop for ReplaceOriginal<'a, T> { + fn drop(&mut self) { + *self.global.borrow_mut() = self.original.take(); + } + } + + let _guard = ReplaceOriginal { + original, + global: r, + }; + + f() }) } -// invoking this function asserts that T and S are both instantiations -// of the same lifetime-parametric type. #[doc(hidden)] -pub unsafe fn with R>( - global: &'static LocalKey>, +pub fn with R>( + global: &'static LocalKey>>, mutator: F, ) -> Option { - global.with(|r| { - let br = r.borrow_mut(); - if *br != 0 as *mut S { - // safe because it's only non-zero when it's being called from using, which - // is holding on to the underlying reference (and not using it itself) safely. - Some(mutator(&mut *(*br as *mut S as *mut T))) - } else { - None + global.with(|r| unsafe { + let ptr = r.borrow_mut(); + match *ptr { + Some(ptr) => { + // safe because it's only non-zero when it's being called from using, which + // is holding on to the underlying reference (and not using it itself) safely. + Some(mutator(&mut *ptr)) + } + None => None, } }) } -#[doc(hidden)] -#[macro_export] -macro_rules! decl { - ($name:ident : $t:ty) => { - thread_local! { - static $name: $crate::RefCell<*mut $t> = $crate::RefCell::new(0 as *mut $t); - } - } -} - /// Declare a new global reference module whose underlying value does not contain references. /// /// Will create a module of a given name that contains two functions: -/// * `pub fn using<'a: 'b, 'b, R, F: FnOnce() -> R, T: 'a>(protected: &'b mut T, f: F) -> R` +/// * `pub fn using R>(protected: &mut $t, f: F) -> R` /// This executes `f`, returning its value. During the call, the module's reference is set to /// be equal to `protected`. -/// * `pub fn with FnOnce(&'r mut $t<'t>) -> R>(f: F) -> Option` +/// * `pub fn with R>(f: F) -> Option` /// This executes `f`, returning `Some` of its value if called from code that is being executed /// as part of a `using` call. If not, it returns `None`. `f` is provided with one argument: the /// same reference as provided to the most recent `using` call. /// /// # Examples /// -/// ``` +/// Initializing the global context with a given value. +/// +/// ```rust /// #[macro_use] extern crate environmental; -/// declare_simple!(counter: u32); +/// environmental!(counter: u32); /// fn main() { /// let mut counter_value = 41u32; /// counter::using(&mut counter_value, || { @@ -131,96 +134,55 @@ macro_rules! decl { /// println!("The answer is {:?}", counter_value); // 42 /// } /// ``` -#[macro_export] -macro_rules! declare_simple { - ($name:ident : $t:ty) => { - mod $name { - #[allow(unused_imports)] - use super::*; - - decl!(GLOBAL: $t); - - pub fn using R>( - protected: &mut $t, - f: F - ) -> R { - unsafe { $crate::using(&GLOBAL, protected, f) } - } - - pub fn with R>( - f: F - ) -> Option { - unsafe { $crate::with(&GLOBAL, f) } - } - } - } -} - -/// Declare a new global reference module whose underlying value is generic over a reference. /// -/// Will create a module of a given name that contains two functions: +/// Roughly the same, but with a trait object: /// -/// * `pub fn using<'a: 'b, 'b, R, F: FnOnce() -> R, T: 'a>(protected: &'b mut T, f: F) -> R` -/// This executes `f`, returning its value. During the call, the module's reference is set to -/// be equal to `protected`. -/// * `pub fn with FnOnce(&'r mut $t<'t>) -> R>(f: F) -> Option` -/// This executes `f`, returning `Some` of its value if called from code that is being executed -/// as part of a `using` call. If not, it returns `None`. `f` is provided with one argument: the -/// same reference as provided to the most recent `using` call. +/// ```rust +/// #[macro_use] extern crate environmental; /// -/// # Examples +/// trait Increment { fn increment(&mut self); } /// -/// ``` -/// #[macro_use] extern crate environmental; -/// // a type that we want to reference from a temp global; it has a reference in it. -/// struct WithReference<'a> { answer_ref: &'a mut u32, } -/// // create a place for the global reference to exist. -/// declare_generic!(counter: WithReference); -/// fn stuff() { -/// // do some stuff, accessing the named reference as desired. -/// counter::with(|i| *i.answer_ref += 1); +/// impl Increment for i32 { +/// fn increment(&mut self) { *self += 1 } /// } +/// +/// environmental!(val: Increment + 'static); +/// /// fn main() { -/// // declare a stack variable of the same type as our global declaration. -/// let mut answer = 41u32; -/// { -/// let mut ref_struct = WithReference { answer_ref: &mut answer, }; -/// counter::using(&mut ref_struct, stuff); -/// } -/// println!("The answer is {:?}", answer); // will print 42! +/// let mut local = 0i32; +/// val::using(&mut local, || { +/// val::with(|v| for _ in 0..5 { v.increment() }); +/// }); +/// +/// assert_eq!(local, 5); /// } /// ``` #[macro_export] -macro_rules! declare_generic { - ($name:ident : $t:tt) => { - mod $name { - #[allow(unused_imports)] - use super::*; +macro_rules! environmental { + ($name:ident : $t:ty) => { + #[allow(non_camel_case_types)] + struct $name { __private_field: () } - decl!(GLOBAL: $t<'static> ); + thread_local!(static GLOBAL: ::std::cell::RefCell> + = ::std::cell::RefCell::new(None)); - pub fn using<'a: 'b, 'b, R, F: FnOnce() -> R, T: 'a>( - protected: &'b mut T, + impl $name { + #[allow(unused_imports)] + + pub fn using R>( + protected: &mut $t, f: F ) -> R { $crate::using(&GLOBAL, protected, f) } - pub fn with FnOnce(&'r mut $t<'t>) -> R>( + pub fn with R>( f: F ) -> Option { - let dummy = (); - with_closed(f, &dummy) - } - - fn with_closed<'d: 't, 't: 'r, 'r, R, F: FnOnce(&'r mut $t<'t>) -> R>( - f: F, - _dummy: &'d (), - ) -> Option { - $crate::with(&GLOBAL, f) + $crate::with(&GLOBAL, |x| f(x)) } } - } + }; } #[cfg(test)] @@ -228,9 +190,9 @@ mod tests { #[test] fn simple_works() { - declare_simple!(counter: u32); + environmental!(counter: u32); - let stuff = || counter::with(|value| *value += 1); + fn stuff() { counter::with(|value| *value += 1); }; // declare a stack variable of the same type as our global declaration. let mut local = 41u32; @@ -238,13 +200,12 @@ mod tests { // call stuff, setting up our `counter` environment as a refrence to our local counter var. counter::using(&mut local, stuff); assert_eq!(local, 42); - stuff(); // safe! doesn't do anything. } #[test] fn overwrite_with_lesser_lifetime() { - declare_simple!(items: Vec); + environmental!(items: Vec); let mut local_items = vec![1, 2, 3]; items::using(&mut local_items, || { @@ -254,4 +215,58 @@ mod tests { assert_eq!(local_items, vec![4, 5, 6]); } + + #[test] + fn declare_with_trait_object() { + trait Foo { + fn get(&self) -> i32; + fn set(&mut self, x: i32); + } + + impl Foo for i32 { + fn get(&self) -> i32 { *self } + fn set(&mut self, x: i32) { *self = x } + } + + environmental!(foo: Foo + 'static); + + fn stuff() { + foo::with(|value| { + let new_val = value.get() + 1; + value.set(new_val); + }); + } + + let mut local = 41i32; + foo::using(&mut local, stuff); + + assert_eq!(local, 42); + + stuff(); // doesn't do anything. + } + + #[test] + fn unwind_recursive() { + use std::panic; + + environmental!(items: Vec); + + let panicked = panic::catch_unwind(|| { + let mut local_outer = vec![1, 2, 3]; + + items::using(&mut local_outer, || { + let mut local_inner = vec![4, 5, 6]; + items::using(&mut local_inner, || { + panic!("are you unsafe?"); + }) + }); + }).is_err(); + + assert!(panicked); + + let mut was_cleared = true; + items::with(|_items| was_cleared = false); + + assert!(was_cleared); + } } From 66c93843ba3f59da6ad0b28473adae75ad8dc8e5 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Wed, 17 Jan 2018 19:27:38 +0100 Subject: [PATCH 3/3] use new environmental API --- native-runtime/support/src/lib.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/native-runtime/support/src/lib.rs b/native-runtime/support/src/lib.rs index a2fe13eb104a2..d929b699de6b0 100644 --- a/native-runtime/support/src/lib.rs +++ b/native-runtime/support/src/lib.rs @@ -21,21 +21,17 @@ impl fmt::Display for NoError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "") } } -pub struct ExternalitiesHolder<'a> { - ext: &'a mut Externalities, -} - -declare_generic!(ext : ExternalitiesHolder); +environmental!(ext : Externalities + 'static); pub fn storage(key: &[u8]) -> Vec { - ext::with(|holder| holder.ext.storage(key).ok().map(|s| s.to_vec())) + ext::with(|ext| ext.storage(key).ok().map(|s| s.to_vec())) .unwrap_or(None) .unwrap_or_else(|| vec![]) } pub fn read_storage(key: &[u8], value_out: &mut [u8]) -> usize { - ext::with(|holder| { - if let Ok(value) = holder.ext.storage(key) { + ext::with(|ext| { + if let Ok(value) = ext.storage(key) { let written = ::std::cmp::min(value.len(), value_out.len()); value_out[0..written].copy_from_slice(&value[0..written]); value.len() @@ -48,8 +44,8 @@ pub fn read_storage(key: &[u8], value_out: &mut [u8]) -> usize { pub fn storage_into(_key: &[u8]) -> Option { let size = size_of::(); - ext::with(|holder| { - if let Ok(value) = holder.ext.storage(_key) { + ext::with(|ext| { + if let Ok(value) = ext.storage(_key) { if value.len() == size { unsafe { let mut result: T = std::mem::uninitialized(); @@ -64,15 +60,15 @@ pub fn storage_into(_key: &[u8]) -> Option { } pub fn set_storage(_key: &[u8], _value: &[u8]) { - ext::with(|holder| - holder.ext.set_storage(_key.to_vec(), _value.to_vec()) + ext::with(|ext| + ext.set_storage(_key.to_vec(), _value.to_vec()) ); } /// The current relay chain identifier. pub fn chain_id() -> u64 { - ext::with(|holder| - holder.ext.chain_id() + ext::with(|ext| + ext.chain_id() ).unwrap_or(0) } @@ -81,9 +77,8 @@ pub use tiny_keccak::keccak256; /// Execute the given closure with global function available whose functionality routes into the /// externalities `ext`. Forwards the value that the closure returns. -pub fn with_externalities R>(ext: &mut Externalities, f: F) -> R { - let mut h = ExternalitiesHolder { ext }; - ext::using(&mut h, f) +pub fn with_externalities R>(ext: &mut (Externalities + 'static), f: F) -> R { + ext::using(ext, f) } #[macro_export]