From 8b5fc053e7a42666195064d75d6e05668b17bbc0 Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 3 May 2018 09:17:52 +0200 Subject: [PATCH 1/4] Remove expect --- util/patricia_trie/src/lookup.rs | 43 +++++++++++++++++--------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/util/patricia_trie/src/lookup.rs b/util/patricia_trie/src/lookup.rs index 88d2bc66e02..e5b3451a24e 100644 --- a/util/patricia_trie/src/lookup.rs +++ b/util/patricia_trie/src/lookup.rs @@ -55,29 +55,32 @@ impl<'a, Q: Query> Lookup<'a, Q> { // without incrementing the depth. let mut node_data = &node_data[..]; loop { - match Node::decoded(node_data).expect("rlp read from db; qed") { - Node::Leaf(slice, value) => { - return Ok(match slice == key { - true => Some(self.query.decode(value)), - false => None, - }) - } - Node::Extension(slice, item) => { - if key.starts_with(&slice) { - node_data = item; - key = key.mid(slice.len()); - } else { - return Ok(None) + match Node::decoded(node_data) { + Ok(decoded_node) => match decoded_node { + Node::Leaf(slice, value) => { + return Ok(match slice == key { + true => Some(self.query.decode(value)), + false => None, + }) } - } - Node::Branch(children, value) => match key.is_empty() { - true => return Ok(value.map(move |val| self.query.decode(val))), - false => { - node_data = children[key.at(0) as usize]; - key = key.mid(1); + Node::Extension(slice, item) => { + if key.starts_with(&slice) { + node_data = item; + key = key.mid(slice.len()); + } else { + return Ok(None) + } } + Node::Branch(children, value) => match key.is_empty() { + true => return Ok(value.map(move |val| self.query.decode(val))), + false => { + node_data = children[key.at(0) as usize]; + key = key.mid(1); + } + }, + _ => return Ok(None), }, - _ => return Ok(None), + _ => return Ok(None) } // check if new node data is inline or hash. From 1d54fc973aa6db2a8586bf55f0536cc11696bb7a Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 3 May 2018 17:11:49 +0200 Subject: [PATCH 2/4] Propagate rlp::DecoderErrors as TrieErrors --- util/patricia_trie/src/lib.rs | 8 ++++++++ util/patricia_trie/src/lookup.rs | 2 +- util/rlp/src/error.rs | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/util/patricia_trie/src/lib.rs b/util/patricia_trie/src/lib.rs index 3a1d683c26c..b2aa56c02f7 100644 --- a/util/patricia_trie/src/lib.rs +++ b/util/patricia_trie/src/lib.rs @@ -67,6 +67,8 @@ pub enum TrieError { InvalidStateRoot(H256), /// Trie item not found in the database, IncompleteDatabase(H256), + /// Corrupt Trie item + DecoderError(rlp::DecoderError), } impl fmt::Display for TrieError { @@ -75,6 +77,7 @@ impl fmt::Display for TrieError { TrieError::InvalidStateRoot(ref root) => write!(f, "Invalid state root: {}", root), TrieError::IncompleteDatabase(ref missing) => write!(f, "Database missing expected key: {}", missing), + TrieError::DecoderError(ref err) => write!(f, "Decoding failed with {}", err), } } } @@ -84,10 +87,15 @@ impl error::Error for TrieError { match *self { TrieError::InvalidStateRoot(_) => "Invalid state root", TrieError::IncompleteDatabase(_) => "Incomplete database", + TrieError::DecoderError(ref e) => e.description(), } } } +impl From for TrieError { + fn from(e: rlp::DecoderError) -> Self { TrieError::DecoderError(e) } +} + /// Trie result type. Boxed to avoid copying around extra space for `H256`s on successful queries. pub type Result = ::std::result::Result>; diff --git a/util/patricia_trie/src/lookup.rs b/util/patricia_trie/src/lookup.rs index e5b3451a24e..62faaac8054 100644 --- a/util/patricia_trie/src/lookup.rs +++ b/util/patricia_trie/src/lookup.rs @@ -80,7 +80,7 @@ impl<'a, Q: Query> Lookup<'a, Q> { }, _ => return Ok(None), }, - _ => return Ok(None) + Err(e) => return Err(Box::new(e.into())) } // check if new node data is inline or hash. diff --git a/util/rlp/src/error.rs b/util/rlp/src/error.rs index 5113fdc17df..7aef6cfbf72 100644 --- a/util/rlp/src/error.rs +++ b/util/rlp/src/error.rs @@ -9,7 +9,7 @@ use std::fmt; use std::error::Error as StdError; -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Clone)] /// Error concerning the RLP decoder. pub enum DecoderError { /// Data has additional bytes at the end of the valid RLP fragment. From 1a7ced46ddae9bac2ca1c2279e57287fe657b7f1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 4 May 2018 07:00:32 +0200 Subject: [PATCH 3/4] Avoid nested match --- util/patricia_trie/src/lib.rs | 4 +-- util/patricia_trie/src/lookup.rs | 43 +++++++++++++++----------------- 2 files changed, 22 insertions(+), 25 deletions(-) diff --git a/util/patricia_trie/src/lib.rs b/util/patricia_trie/src/lib.rs index b2aa56c02f7..d1563becff0 100644 --- a/util/patricia_trie/src/lib.rs +++ b/util/patricia_trie/src/lib.rs @@ -92,8 +92,8 @@ impl error::Error for TrieError { } } -impl From for TrieError { - fn from(e: rlp::DecoderError) -> Self { TrieError::DecoderError(e) } +impl From for Box { + fn from(e: rlp::DecoderError) -> Self { Box::new(TrieError::DecoderError(e)) } } /// Trie result type. Boxed to avoid copying around extra space for `H256`s on successful queries. diff --git a/util/patricia_trie/src/lookup.rs b/util/patricia_trie/src/lookup.rs index 62faaac8054..2d63f7d00e1 100644 --- a/util/patricia_trie/src/lookup.rs +++ b/util/patricia_trie/src/lookup.rs @@ -55,32 +55,29 @@ impl<'a, Q: Query> Lookup<'a, Q> { // without incrementing the depth. let mut node_data = &node_data[..]; loop { - match Node::decoded(node_data) { - Ok(decoded_node) => match decoded_node { - Node::Leaf(slice, value) => { - return Ok(match slice == key { - true => Some(self.query.decode(value)), - false => None, - }) + match Node::decoded(node_data)? { + Node::Leaf(slice, value) => { + return Ok(match slice == key { + true => Some(self.query.decode(value)), + false => None, + }) + } + Node::Extension(slice, item) => { + if key.starts_with(&slice) { + node_data = item; + key = key.mid(slice.len()); + } else { + return Ok(None) } - Node::Extension(slice, item) => { - if key.starts_with(&slice) { - node_data = item; - key = key.mid(slice.len()); - } else { - return Ok(None) - } + } + Node::Branch(children, value) => match key.is_empty() { + true => return Ok(value.map(move |val| self.query.decode(val))), + false => { + node_data = children[key.at(0) as usize]; + key = key.mid(1); } - Node::Branch(children, value) => match key.is_empty() { - true => return Ok(value.map(move |val| self.query.decode(val))), - false => { - node_data = children[key.at(0) as usize]; - key = key.mid(1); - } - }, - _ => return Ok(None), }, - Err(e) => return Err(Box::new(e.into())) + _ => return Ok(None), } // check if new node data is inline or hash. From 133d057a91368020fcd162a3d09e0c5c2281b077 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 4 May 2018 08:33:54 +0200 Subject: [PATCH 4/4] Add test that will work once PR https://github.com/paritytech/parity/pull/8527 is merged --- util/patricia_trie/src/triedb.rs | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/util/patricia_trie/src/triedb.rs b/util/patricia_trie/src/triedb.rs index 682f12467d3..aed683117d3 100644 --- a/util/patricia_trie/src/triedb.rs +++ b/util/patricia_trie/src/triedb.rs @@ -493,3 +493,30 @@ fn get_len() { assert_eq!(t.get_with(b"B", |x: &[u8]| x.len()), Ok(Some(5))); assert_eq!(t.get_with(b"C", |x: &[u8]| x.len()), Ok(None)); } + +// Test will work once https://github.com/paritytech/parity/pull/8527 is merged and rlp::decode returns Result instead of panicking +//#[test] +//fn test_lookup_with_corrupt_data_returns_decoder_error() { +// use memorydb::*; +// use super::TrieMut; +// use super::triedbmut::*; +// use rlp; +// use ethereum_types::H512; +// +// let mut memdb = MemoryDB::new(); +// let mut root = H256::new(); +// { +// let mut t = TrieDBMut::new(&mut memdb, &mut root); +// t.insert(b"A", b"ABC").unwrap(); +// t.insert(b"B", b"ABCBA").unwrap(); +// } +// +// let t = TrieDB::new(&memdb, &root).unwrap(); +// +// // query for an invalid data type to trigger an error +// let q = rlp::decode::; +// let lookup = Lookup{ db: t.db, query: q, hash: root }; +// let query_result = lookup.look_up(NibbleSlice::new(b"A")); +// let expected = Box::new(TrieError::DecoderError(::rlp::DecoderError::RlpIsTooShort)); +// assert_eq!(query_result.unwrap_err(), expected); +//}