Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Don't panic in import_block if invalid rlp #8522

Merged
merged 3 commits into from
May 3, 2018
Merged

Don't panic in import_block if invalid rlp #8522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0414a62
Don't panic in import_block if invalid rlp
ascjones May 1, 2018
a21ecb2
Remove redundant type annotation
ascjones May 2, 2018
22b2c3a
Replace RLP header view usage with safe decoding
ascjones May 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
let block = view!(BlockView, &block_bytes);
let header = block.header();
fn import_old_block(&self, header: &Header, block_bytes: Bytes, receipts_bytes: Bytes, db: &KeyValueDB, chain: &BlockChain) -> Result<H256, ::error::Error> {
let receipts = ::rlp::decode_list(&receipts_bytes);
let hash = header.hash();
let _import_lock = self.import_lock.lock();
Expand Down Expand Up @@ -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()) {
Expand All @@ -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)
}
}

Expand Down
2 changes: 2 additions & 0 deletions ethcore/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ error_chain! {

foreign_links {
Block(BlockError) #[doc = "Block error"];
Decoder(::rlp::DecoderError) #[doc = "Rlp decoding error"];
}

errors {
Expand All @@ -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(),
}
}
Expand Down
20 changes: 20 additions & 0 deletions ethcore/src/tests/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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();
Expand Down
9 changes: 4 additions & 5 deletions ethcore/src/verification/queue/kind.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,13 @@ pub mod blocks {

impl Unverified {
/// Create an `Unverified` from raw bytes.
pub fn new(bytes: Bytes) -> Self {
use views::BlockView;
pub fn from_rlp(bytes: Bytes) -> Result<Self, ::rlp::DecoderError> {

let header = view!(BlockView, &bytes).header();
Unverified {
let header = ::rlp::Rlp::new(&bytes).val_at(0)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind fixing similar issue for ancient block (block_with_receipt) import?
AFAIR we also have a BlockView there.

Copy link
Contributor

@andresilva andresilva May 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also collect_blocks (https://github.com/paritytech/parity/blob/master/ethcore/sync/src/block_sync.rs#L472). It's the only place that calls import_block_with_receipts but it creates a header view before doing so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I think what comes from BlockCollection is valid (invalid blocks/headers are silently ignored when inserting) so collect_blocks is safe, not sure if we should update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, invalid header/block body RLP is rejected further upstream in block_sync, which results in the offending peer being deactivated.

So blocks pulled from BlockCollection should all be valid RLP so safe to use the views inside collect_blocks.

Seems to me that we could remove plenty of 'unsafe' view usages in this path since we could pass down the pre-decoded headers/block bodies, alongside/instead of the raw bytes. But that would be a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomusdrw see 22b2c3a

Ok(Unverified {
header: header,
bytes: bytes,
}
})
}
}

Expand Down
23 changes: 14 additions & 9 deletions ethcore/src/verification/queue/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -757,19 +762,19 @@ 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);
}
}

#[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 {
Expand All @@ -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();
Expand All @@ -802,22 +807,22 @@ 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);
}
}

#[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);
Expand All @@ -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());
}
Expand Down Expand Up @@ -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.
Expand Down