From 7daae1c1e44174108041373f13985c7ea8f2cc23 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 28 Jul 2016 20:08:15 +0200 Subject: [PATCH 1/2] fix state unsafety with a mostly-guaranteed handle --- ethcore/src/state.rs | 54 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 11 deletions(-) diff --git a/ethcore/src/state.rs b/ethcore/src/state.rs index ca19101d854..c014aaf2fdb 100644 --- a/ethcore/src/state.rs +++ b/ethcore/src/state.rs @@ -48,6 +48,27 @@ pub struct State { const SEC_TRIE_DB_UNWRAP_STR: &'static str = "A state can only be created with valid root. Creating a SecTrieDB with a valid root will not fail. \ Therefore creating a SecTrieDB with this state's root will not fail."; +// handle to an account which is supposed to be cached. +struct Handle<'a> { + state: &'a State, + key: &'a Address +} + +impl<'a> Handle<'a> { + // Consume the handle with a closure which produces a value. + fn consume(self, f: F) -> U + where F: FnOnce(&Option) -> U + { + // if the cache has been tampered with since the handle was produced, + // fall back to None. + let cache = self.state.cache.borrow(); + match cache.get(self.key) { + Some(acc) => f(acc), + None => f(&None), + } + } +} + impl State { /// Creates new state with empty state root #[cfg(test)] @@ -167,22 +188,26 @@ impl State { /// Get the balance of account `a`. pub fn balance(&self, a: &Address) -> U256 { - self.get(a, false).as_ref().map_or(U256::zero(), |account| *account.balance()) + self.ensure_cached(a, false) + .consume(|a| a.as_ref().map_or(U256::zero(), |account| *account.balance())) } /// Get the nonce of account `a`. pub fn nonce(&self, a: &Address) -> U256 { - self.get(a, false).as_ref().map_or(U256::zero(), |account| *account.nonce()) + self.ensure_cached(a, false) + .consume(|a| a.as_ref().map_or(U256::zero(), |account| *account.nonce())) } /// Mutate storage of account `address` so that it is `value` for `key`. pub fn storage_at(&self, address: &Address, key: &H256) -> H256 { - self.get(address, false).as_ref().map_or(H256::new(), |a|a.storage_at(&AccountDB::new(self.db.as_hashdb(), address), key)) + self.ensure_cached(address, false) + .consume(|a| a.as_ref().map_or(H256::new(), |a|a.storage_at(&AccountDB::new(self.db.as_hashdb(), address), key))) } /// Mutate storage of account `a` so that it is `value` for `key`. pub fn code(&self, a: &Address) -> Option { - self.get(a, true).as_ref().map_or(None, |a|a.code().map(|x|x.to_vec())) + self.ensure_cached(a, true) + .consume(|a| a.as_ref().map_or(None, |a|a.code().map(|x|x.to_vec()))) } /// Add `incr` to the balance of account `a`. @@ -306,11 +331,14 @@ impl State { fn query_pod(&mut self, query: &PodState) { for (ref address, ref pod_account) in query.get() { - if self.get(address, true).is_some() { - for key in pod_account.storage.keys() { - self.storage_at(address, key); + let handle = self.ensure_cached(address, true); + handle.consume(|a| { + if a.is_some() { + for key in pod_account.storage.keys() { + self.storage_at(address, key); + } } - } + }); } } @@ -323,9 +351,9 @@ impl State { pod_state::diff_pod(&state_pre.to_pod(), &pod_state_post) } - /// Pull account `a` in our cache from the trie DB and return it. + /// Ensure account `a` is in our cache of the trie DB and return a handle for getting it. /// `require_code` requires that the code be cached, too. - fn get<'a>(&'a self, a: &Address, require_code: bool) -> &'a Option { + fn ensure_cached<'a>(&'a self, a: &'a Address, require_code: bool) -> Handle<'a> { let have_key = self.cache.borrow().contains_key(a); if !have_key { let db = self.trie_factory.readonly(self.db.as_hashdb(), &self.root).expect(SEC_TRIE_DB_UNWRAP_STR); @@ -336,7 +364,11 @@ impl State { account.cache_code(&AccountDB::new(self.db.as_hashdb(), a)); } } - unsafe { ::std::mem::transmute(self.cache.borrow().get(a).unwrap()) } + + Handle { + state: self, + key: a, + } } /// Pull account `a` in our cache from the trie DB. `require_code` requires that the code be cached, too. From 56f7232b4420e854f16589bbc431be4b8dc1b4e3 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Fri, 29 Jul 2016 14:35:37 +0200 Subject: [PATCH 2/2] ensure_cached takes a closure directly --- ethcore/src/state.rs | 48 +++++++++++--------------------------------- 1 file changed, 12 insertions(+), 36 deletions(-) diff --git a/ethcore/src/state.rs b/ethcore/src/state.rs index c014aaf2fdb..35f695313ff 100644 --- a/ethcore/src/state.rs +++ b/ethcore/src/state.rs @@ -48,27 +48,6 @@ pub struct State { const SEC_TRIE_DB_UNWRAP_STR: &'static str = "A state can only be created with valid root. Creating a SecTrieDB with a valid root will not fail. \ Therefore creating a SecTrieDB with this state's root will not fail."; -// handle to an account which is supposed to be cached. -struct Handle<'a> { - state: &'a State, - key: &'a Address -} - -impl<'a> Handle<'a> { - // Consume the handle with a closure which produces a value. - fn consume(self, f: F) -> U - where F: FnOnce(&Option) -> U - { - // if the cache has been tampered with since the handle was produced, - // fall back to None. - let cache = self.state.cache.borrow(); - match cache.get(self.key) { - Some(acc) => f(acc), - None => f(&None), - } - } -} - impl State { /// Creates new state with empty state root #[cfg(test)] @@ -188,26 +167,26 @@ impl State { /// Get the balance of account `a`. pub fn balance(&self, a: &Address) -> U256 { - self.ensure_cached(a, false) - .consume(|a| a.as_ref().map_or(U256::zero(), |account| *account.balance())) + self.ensure_cached(a, false, + |a| a.as_ref().map_or(U256::zero(), |account| *account.balance())) } /// Get the nonce of account `a`. pub fn nonce(&self, a: &Address) -> U256 { - self.ensure_cached(a, false) - .consume(|a| a.as_ref().map_or(U256::zero(), |account| *account.nonce())) + self.ensure_cached(a, false, + |a| a.as_ref().map_or(U256::zero(), |account| *account.nonce())) } /// Mutate storage of account `address` so that it is `value` for `key`. pub fn storage_at(&self, address: &Address, key: &H256) -> H256 { - self.ensure_cached(address, false) - .consume(|a| a.as_ref().map_or(H256::new(), |a|a.storage_at(&AccountDB::new(self.db.as_hashdb(), address), key))) + self.ensure_cached(address, false, + |a| a.as_ref().map_or(H256::new(), |a|a.storage_at(&AccountDB::new(self.db.as_hashdb(), address), key))) } /// Mutate storage of account `a` so that it is `value` for `key`. pub fn code(&self, a: &Address) -> Option { - self.ensure_cached(a, true) - .consume(|a| a.as_ref().map_or(None, |a|a.code().map(|x|x.to_vec()))) + self.ensure_cached(a, true, + |a| a.as_ref().map_or(None, |a|a.code().map(|x|x.to_vec()))) } /// Add `incr` to the balance of account `a`. @@ -331,8 +310,7 @@ impl State { fn query_pod(&mut self, query: &PodState) { for (ref address, ref pod_account) in query.get() { - let handle = self.ensure_cached(address, true); - handle.consume(|a| { + self.ensure_cached(address, true, |a| { if a.is_some() { for key in pod_account.storage.keys() { self.storage_at(address, key); @@ -353,7 +331,8 @@ impl State { /// Ensure account `a` is in our cache of the trie DB and return a handle for getting it. /// `require_code` requires that the code be cached, too. - fn ensure_cached<'a>(&'a self, a: &'a Address, require_code: bool) -> Handle<'a> { + fn ensure_cached<'a, F, U>(&'a self, a: &'a Address, require_code: bool, f: F) -> U + where F: FnOnce(&Option) -> U { let have_key = self.cache.borrow().contains_key(a); if !have_key { let db = self.trie_factory.readonly(self.db.as_hashdb(), &self.root).expect(SEC_TRIE_DB_UNWRAP_STR); @@ -365,10 +344,7 @@ impl State { } } - Handle { - state: self, - key: a, - } + f(self.cache.borrow().get(a).unwrap()) } /// Pull account `a` in our cache from the trie DB. `require_code` requires that the code be cached, too.