Don't panic in import_block if invalid rlp (#8522)

* Don't panic in import_block if invalid rlp

* Remove redundant type annotation

* Replace RLP header view usage with safe decoding

Using the view will panic with invalid RLP. Here we use Rlp decoding directly which will return a `Result<_, DecoderError>`. While this path currently should not have any invalid RLP - it makes it safer if ever called with invalid RLP from other code paths.

ethcore/src/client/client.rs

@@ -447,9 +447,7 @@ impl Importer {
 	///
 	/// The block is guaranteed to be the next best blocks in the
 	/// first block sequence. Does no sealing or transaction validation.
-	fn import_old_block(&self, block_bytes: Bytes, receipts_bytes: Bytes, db: &KeyValueDB, chain: &BlockChain) -> Result<H256, ::error::Error> {
+	fn import_old_block(&self, header: &Header, block_bytes: Bytes, receipts_bytes: Bytes, db: &KeyValueDB, chain: &BlockChain) -> Result<H256, ::error::Error> {
-		let block = view!(BlockView, &block_bytes);
-		let header = block.header();
 		let receipts = ::rlp::decode_list(&receipts_bytes);
 		let hash = header.hash();
 		let _import_lock = self.import_lock.lock();
@@ -1408,7 +1406,7 @@ impl ImportBlock for Client {
 		use verification::queue::kind::blocks::Unverified;
 
 		// create unverified block here so the `keccak` calculation can be cached.
-		let unverified = Unverified::new(bytes);
+		let unverified = Unverified::from_rlp(bytes)?;
 
 		{
 			if self.chain.read().is_known(&unverified.hash()) {
@@ -1423,19 +1421,19 @@ impl ImportBlock for Client {
 	}
 
 	fn import_block_with_receipts(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> Result<H256, BlockImportError> {
+		let header: Header = ::rlp::Rlp::new(&block_bytes).val_at(0)?; 
 		{
 			// check block order
-			let header = view!(BlockView, &block_bytes).header_view();
 			if self.chain.read().is_known(&header.hash()) {
 				bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
 			}
-			let status = self.block_status(BlockId::Hash(header.parent_hash()));
+			let status = self.block_status(BlockId::Hash(*header.parent_hash()));
 			if status == BlockStatus::Unknown || status == BlockStatus::Pending {
-				bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(header.parent_hash())));
+				bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(*header.parent_hash())));
 			}
 		}
 
-		self.importer.import_old_block(block_bytes, receipts_bytes, &**self.db.read(), &*self.chain.read()).map_err(Into::into)
+		self.importer.import_old_block(&header, block_bytes, receipts_bytes, &**self.db.read(), &*self.chain.read()).map_err(Into::into)
 	}
 }
 

ethcore/src/error.rs

@@ -190,6 +190,7 @@ error_chain! {
 
 	foreign_links {
 		Block(BlockError) #[doc = "Block error"];
+		Decoder(::rlp::DecoderError) #[doc = "Rlp decoding error"];
 	}
 
 	errors {
@@ -206,6 +207,7 @@ impl From<Error> for BlockImportError {
 		match e {
 			Error(ErrorKind::Block(block_error), _) => BlockImportErrorKind::Block(block_error).into(),
 			Error(ErrorKind::Import(import_error), _) => BlockImportErrorKind::Import(import_error.into()).into(),
+			Error(ErrorKind::Util(util_error::ErrorKind::Decoder(decoder_err)), _) => BlockImportErrorKind::Decoder(decoder_err).into(),
 			_ => BlockImportErrorKind::Other(format!("other block import error: {:?}", e)).into(),
 		}
 	}

ethcore/src/tests/client.rs

@@ -36,6 +36,7 @@ use views::BlockView;
 use ethkey::KeyPair;
 use transaction::{PendingTransaction, Transaction, Action, Condition};
 use miner::MinerService;
+use rlp::{RlpStream, EMPTY_LIST_RLP};
 use tempdir::TempDir;
 
 #[test]
@@ -111,6 +112,25 @@ fn imports_good_block() {
 	assert!(!block.into_inner().is_empty());
 }
 
+#[test]
+fn fails_to_import_block_with_invalid_rlp() {
+	use error::{BlockImportError, BlockImportErrorKind};
+
+	let client = generate_dummy_client(6);
+	let mut rlp = RlpStream::new_list(3);
+	rlp.append_raw(&EMPTY_LIST_RLP, 1); // empty header
+	rlp.append_raw(&EMPTY_LIST_RLP, 1);
+	rlp.append_raw(&EMPTY_LIST_RLP, 1);
+	let invalid_header_block = rlp.out();
+
+	match client.import_block(invalid_header_block) {
+		Err(BlockImportError(BlockImportErrorKind::Decoder(_), _)) => (), // all good
+		Err(_) => panic!("Should fail with a decoder error"),
+		Ok(_) => panic!("Should not import block with invalid header"),
+	}
+}
+
+
 #[test]
 fn query_none_block() {
 	let tempdir = TempDir::new("").unwrap();

ethcore/src/verification/queue/kind.rs

@@ -119,14 +119,13 @@ pub mod blocks {
 
 	impl Unverified {
 		/// Create an `Unverified` from raw bytes.
-		pub fn new(bytes: Bytes) -> Self {
+		pub fn from_rlp(bytes: Bytes) -> Result<Self, ::rlp::DecoderError> {
-			use views::BlockView;
 
-			let header = view!(BlockView, &bytes).header();
+			let header = ::rlp::Rlp::new(&bytes).val_at(0)?; 
-			Unverified {
+			Ok(Unverified {
 				header: header,
 				bytes: bytes,
-			}
+			})
 		}
 	}
 

ethcore/src/verification/queue/mod.rs

@@ -734,6 +734,7 @@ mod tests {
 	use test_helpers::{get_good_dummy_block_seq, get_good_dummy_block};
 	use error::*;
 	use views::BlockView;
+	use bytes::Bytes;
 
 	// create a test block queue.
 	// auto_scaling enables verifier adjustment.
@@ -746,6 +747,10 @@ mod tests {
 		BlockQueue::new(config, engine, IoChannel::disconnected(), true)
 	}
 
+	fn new_unverified(bytes: Bytes) -> Unverified {
+		Unverified::from_rlp(bytes).expect("Should be valid rlp")
+	}
+
 	#[test]
 	fn can_be_created() {
 		// TODO better test
@@ -757,7 +762,7 @@ mod tests {
 	#[test]
 	fn can_import_blocks() {
 		let queue = get_test_queue(false);
-		if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) {
+		if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) {
 			panic!("error importing block that is valid by definition({:?})", e);
 		}
 	}
@@ -765,11 +770,11 @@ mod tests {
 	#[test]
 	fn returns_error_for_duplicates() {
 		let queue = get_test_queue(false);
-		if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) {
+		if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) {
 			panic!("error importing block that is valid by definition({:?})", e);
 		}
 
-		let duplicate_import = queue.import(Unverified::new(get_good_dummy_block()));
+		let duplicate_import = queue.import(new_unverified(get_good_dummy_block()));
 		match duplicate_import {
 			Err(e) => {
 				match e {
@@ -786,7 +791,7 @@ mod tests {
 		let queue = get_test_queue(false);
 		let block = get_good_dummy_block();
 		let hash = view!(BlockView, &block).header().hash().clone();
-		if let Err(e) = queue.import(Unverified::new(block)) {
+		if let Err(e) = queue.import(new_unverified(block)) {
 			panic!("error importing block that is valid by definition({:?})", e);
 		}
 		queue.flush();
@@ -802,14 +807,14 @@ mod tests {
 		let queue = get_test_queue(false);
 		let block = get_good_dummy_block();
 		let hash = view!(BlockView, &block).header().hash().clone();
-		if let Err(e) = queue.import(Unverified::new(block)) {
+		if let Err(e) = queue.import(new_unverified(block)) {
 			panic!("error importing block that is valid by definition({:?})", e);
 		}
 		queue.flush();
 		queue.drain(10);
 		queue.mark_as_good(&[ hash ]);
 
-		if let Err(e) = queue.import(Unverified::new(get_good_dummy_block())) {
+		if let Err(e) = queue.import(new_unverified(get_good_dummy_block())) {
 			panic!("error importing block that has already been drained ({:?})", e);
 		}
 	}
@@ -817,7 +822,7 @@ mod tests {
 	#[test]
 	fn returns_empty_once_finished() {
 		let queue = get_test_queue(false);
-		queue.import(Unverified::new(get_good_dummy_block()))
+		queue.import(new_unverified(get_good_dummy_block()))
 			.expect("error importing block that is valid by definition");
 		queue.flush();
 		queue.drain(1);
@@ -835,7 +840,7 @@ mod tests {
 		assert!(!queue.queue_info().is_full());
 		let mut blocks = get_good_dummy_block_seq(50);
 		for b in blocks.drain(..) {
-			queue.import(Unverified::new(b)).unwrap();
+			queue.import(new_unverified(b)).unwrap();
 		}
 		assert!(queue.queue_info().is_full());
 	}
@@ -863,7 +868,7 @@ mod tests {
 		*queue.state.0.lock() = State::Work(0);
 
 		for block in get_good_dummy_block_seq(5000) {
-			queue.import(Unverified::new(block)).expect("Block good by definition; qed");
+			queue.import(new_unverified(block)).expect("Block good by definition; qed");
 		}
 
 		// almost all unverified == bump verifier count.