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

Decoding headers can fail #8570

Merged
merged 25 commits into from
May 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
70db4f5
rlp::decode returns Result
dvdplm May 1, 2018
3b6f5c9
Fix journaldb to handle rlp::decode Result
dvdplm May 1, 2018
f2f47a4
Fix ethcore to work with rlp::decode returning Result
dvdplm May 1, 2018
ae8483d
Light client handles rlp::decode returning Result
dvdplm May 2, 2018
af4970a
Fix tests in rlp_derive
dvdplm May 2, 2018
ffd7a65
Fix tests
dvdplm May 2, 2018
6eac3a2
Cleanup
dvdplm May 2, 2018
23f4597
cleanup
dvdplm May 2, 2018
40eed16
Allow panic rather than breaking out of iterator
dvdplm May 2, 2018
c8b5c16
Let decoding failures when reading from disk blow up
dvdplm May 3, 2018
ea4a6d2
syntax
dvdplm May 3, 2018
ea17bd7
Fix the trivial grumbles
dvdplm May 7, 2018
7b07b69
Fix failing tests
dvdplm May 7, 2018
0930b0a
Make Account::from_rlp return Result
dvdplm May 7, 2018
245ed85
Syntx, sigh
dvdplm May 7, 2018
9cdad30
Temp-fix for decoding failures
dvdplm May 7, 2018
c2afc3c
Header::decode returns Result
dvdplm May 8, 2018
b7d7d55
Do not continue reading from the DB when a value could not be read
dvdplm May 8, 2018
dc3269a
Merge branch 'fix/rlp-decode-returns-result-issue-8033' into fix/deco…
dvdplm May 8, 2018
9ca99dd
Fix tests
dvdplm May 8, 2018
9ecd8c9
Merge branch 'master' into fix/decoding-headers-can-fail-#8553
dvdplm May 8, 2018
b252fc1
Handle header decoding in light_sync
dvdplm May 8, 2018
25491ff
Handling header decoding errors
dvdplm May 8, 2018
97906c3
Let the DecodeError bubble up unchanged
dvdplm May 9, 2018
eaf6ed2
Remove redundant error conversion
dvdplm May 9, 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
10 changes: 6 additions & 4 deletions ethcore/light/src/client/header_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ impl HeaderChain {
batch.put(col, cht_key(cht_num as u64).as_bytes(), &::rlp::encode(cht_root));
}

let decoded_header = hardcoded_sync.header.decode();
let decoded_header = hardcoded_sync.header.decode()?;
let decoded_header_num = decoded_header.number();

// write the block in the DB.
Expand Down Expand Up @@ -585,7 +585,7 @@ impl HeaderChain {
bail!(ErrorKind::Database(msg.into()));
};

let decoded = header.decode();
let decoded = header.decode().expect("decoding db value failed");

let entry: Entry = {
let bytes = self.db.get(self.col, era_key(h_num).as_bytes())?
Expand Down Expand Up @@ -815,7 +815,9 @@ impl HeaderChain {

for hdr in self.ancestry_iter(BlockId::Hash(parent_hash)) {
if let Some(transition) = live_proofs.get(&hdr.hash()).cloned() {
return Some((hdr.decode(), transition.proof))
return hdr.decode().map(|decoded_hdr| {
(decoded_hdr, transition.proof)
}).ok();
}
}

Expand Down Expand Up @@ -1224,7 +1226,7 @@ mod tests {
let hardcoded_sync = chain.read_hardcoded_sync().expect("failed reading hardcoded sync").expect("failed unwrapping hardcoded sync");
assert_eq!(hardcoded_sync.chts.len(), 3);
assert_eq!(hardcoded_sync.total_difficulty, total_difficulty);
let decoded: Header = hardcoded_sync.header.decode();
let decoded: Header = hardcoded_sync.header.decode().expect("decoding failed");
assert_eq!(decoded.number(), h_num);
}
}
12 changes: 10 additions & 2 deletions ethcore/light/src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ impl<T: ChainDataFetcher> Client<T> {

let epoch_proof = self.engine.is_epoch_end(
&verified_header,
&|h| self.chain.block_header(BlockId::Hash(h)).map(|hdr| hdr.decode()),
&|h| self.chain.block_header(BlockId::Hash(h)).and_then(|hdr| hdr.decode().ok()),
&|h| self.chain.pending_transition(h),
);

Expand Down Expand Up @@ -426,7 +426,15 @@ impl<T: ChainDataFetcher> Client<T> {
};

// Verify Block Family
let verify_family_result = self.engine.verify_block_family(&verified_header, &parent_header.decode());

let verify_family_result = {
parent_header.decode()
.map_err(|dec_err| dec_err.into())
.and_then(|decoded| {
self.engine.verify_block_family(&verified_header, &decoded)
})

};
if let Err(e) = verify_family_result {
warn!(target: "client", "Stage 3 block verification failed for #{} ({})\nError: {:?}",
verified_header.number(), verified_header.hash(), e);
Expand Down
14 changes: 9 additions & 5 deletions ethcore/src/client/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1261,8 +1261,7 @@ impl Client {
=> Some(self.chain.read().best_block_header()),
BlockId::Number(number) if number == self.chain.read().best_block_number()
=> Some(self.chain.read().best_block_header()),
_
=> self.block_header(id).map(|h| h.decode()),
_ => self.block_header(id).and_then(|h| h.decode().ok())
}
}
}
Expand Down Expand Up @@ -1998,7 +1997,11 @@ impl BlockChainClient for Client {

fn uncle_extra_info(&self, id: UncleId) -> Option<BTreeMap<String, String>> {
self.uncle(id)
.map(|header| self.engine.extra_info(&header.decode()))
.and_then(|h| {
h.decode().map(|dh| {
self.engine.extra_info(&dh)
}).ok()
})
}

fn pruning_info(&self) -> PruningInfo {
Expand Down Expand Up @@ -2050,7 +2053,8 @@ impl ReopenBlock for Client {
for h in uncles {
if !block.uncles().iter().any(|header| header.hash() == h) {
let uncle = chain.block_header_data(&h).expect("find_uncle_hashes only returns hashes for existing headers; qed");
block.push_uncle(uncle.decode()).expect("pushing up to maximum_uncle_count;
let uncle = uncle.decode().expect("decoding failure");
block.push_uncle(uncle).expect("pushing up to maximum_uncle_count;
push_uncle is not ok only if more than maximum_uncle_count is pushed;
so all push_uncle are Ok;
qed");
Expand Down Expand Up @@ -2091,7 +2095,7 @@ impl PrepareOpenBlock for Client {
.into_iter()
.take(engine.maximum_uncle_count(open_block.header().number()))
.foreach(|h| {
open_block.push_uncle(h.decode()).expect("pushing maximum_uncle_count;
open_block.push_uncle(h.decode().expect("decoding failure")).expect("pushing maximum_uncle_count;
open_block was just created;
push_uncle is not ok only if more than maximum_uncle_count is pushed;
so all push_uncle are Ok;
Expand Down
5 changes: 3 additions & 2 deletions ethcore/src/client/test_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ impl TestBlockChainClient {
/// Make a bad block by setting invalid extra data.
pub fn corrupt_block(&self, n: BlockNumber) {
let hash = self.block_hash(BlockId::Number(n)).unwrap();
let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode();
let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode().expect("decoding failed");
header.set_extra_data(b"This extra data is way too long to be considered valid".to_vec());
let mut rlp = RlpStream::new_list(3);
rlp.append(&header);
Expand All @@ -301,7 +301,7 @@ impl TestBlockChainClient {
/// Make a bad block by setting invalid parent hash.
pub fn corrupt_block_parent(&self, n: BlockNumber) {
let hash = self.block_hash(BlockId::Number(n)).unwrap();
let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode();
let mut header: BlockHeader = self.block_header(BlockId::Number(n)).unwrap().decode().expect("decoding failed");
header.set_parent_hash(H256::from(42));
let mut rlp = RlpStream::new_list(3);
rlp.append(&header);
Expand Down Expand Up @@ -479,6 +479,7 @@ impl BlockInfo for TestBlockChainClient {
self.block_header(BlockId::Hash(self.chain_info().best_block_hash))
.expect("Best block always has header.")
.decode()
.expect("decoding failed")
}

fn block(&self, id: BlockId) -> Option<encoded::Block> {
Expand Down
6 changes: 4 additions & 2 deletions ethcore/src/encoded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use ethereum_types::{H256, Bloom, U256, Address};
use hash::keccak;
use header::{BlockNumber, Header as FullHeader};
use heapsize::HeapSizeOf;
use rlp::{Rlp, RlpStream};
use rlp::{self, Rlp, RlpStream};
use transaction::UnverifiedTransaction;
use views::{self, BlockView, HeaderView, BodyView};

Expand All @@ -47,7 +47,9 @@ impl Header {
pub fn new(encoded: Vec<u8>) -> Self { Header(encoded) }

/// Upgrade this encoded view to a fully owned `Header` object.
pub fn decode(&self) -> FullHeader { ::rlp::decode(&self.0).expect("decoding failure") }
pub fn decode(&self) -> Result<FullHeader, rlp::DecoderError> {
rlp::decode(&self.0)
}

/// Get a borrowed header view onto the data.
#[inline]
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/engines/authority_round/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ impl Engine<EthereumMachine> for AuthorityRound {

let parent = client.block_header(::client::BlockId::Hash(*block.header().parent_hash()))
.expect("hash is from parent; parent header must exist; qed")
.decode();
.decode()?;

let parent_step = header_step(&parent, self.empty_steps_transition)?;
let current_step = self.step.load();
Expand Down
10 changes: 8 additions & 2 deletions ethcore/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,12 @@ error_chain! {
description("Unknown engine name")
display("Unknown engine name ({})", name)
}

#[doc = "RLP decoding errors"]
Decoder(err: ::rlp::DecoderError) {
description("decoding value failed")
display("decoding value failed with error: {}", err)
}
}
}

Expand All @@ -310,11 +316,11 @@ impl From<AccountsError> for Error {
fn from(err: AccountsError) -> Error {
ErrorKind::AccountProvider(err).into()
}
}
}

impl From<::rlp::DecoderError> for Error {
fn from(err: ::rlp::DecoderError) -> Error {
UtilError::from(err).into()
ErrorKind::Decoder(err).into()
}
}

Expand Down
11 changes: 8 additions & 3 deletions ethcore/src/miner/miner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,8 @@ impl Miner {
}

/// Attempts to perform internal sealing (one that does not require work) and handles the result depending on the type of Seal.
fn seal_and_import_block_internally<C>(&self, chain: &C, block: ClosedBlock) -> bool where
C: BlockChain + SealedBlockImporter,
fn seal_and_import_block_internally<C>(&self, chain: &C, block: ClosedBlock) -> bool
where C: BlockChain + SealedBlockImporter,
{
{
let sealing = self.sealing.lock();
Expand All @@ -544,7 +544,12 @@ impl Miner {
trace!(target: "miner", "seal_block_internally: attempting internal seal.");

let parent_header = match chain.block_header(BlockId::Hash(*block.header().parent_hash())) {
Some(hdr) => hdr.decode(),
Some(h) => {
match h.decode() {
Ok(decoded_hdr) => decoded_hdr,
Err(_) => return false
}
}
None => return false,
};

Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/snapshot/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ pub fn verify_old_block(rng: &mut OsRng, header: &Header, engine: &EthEngine, ch
if always || rng.gen::<f32>() <= POW_VERIFY_RATE {
engine.verify_block_unordered(header)?;
match chain.block_header_data(header.parent_hash()) {
Some(parent) => engine.verify_block_family(header, &parent.decode()),
Some(parent) => engine.verify_block_family(header, &parent.decode()?),
None => Ok(()),
}
} else {
Expand Down
5 changes: 2 additions & 3 deletions ethcore/src/verification/verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ fn verify_uncles(header: &Header, bytes: &[u8], bc: &BlockProvider, engine: &Eth
return Err(From::from(BlockError::UncleParentNotInChain(uncle_parent.hash())));
}

let uncle_parent = uncle_parent.decode();
let uncle_parent = uncle_parent.decode()?;
verify_parent(&uncle, &uncle_parent, engine)?;
engine.verify_block_family(&uncle, &uncle_parent)?;
verified.insert(uncle.hash());
Expand Down Expand Up @@ -500,10 +500,9 @@ mod tests {
// no existing tests need access to test, so having this not function
// is fine.
let client = ::client::TestBlockChainClient::default();

let parent = bc.block_header_data(header.parent_hash())
.ok_or(BlockError::UnknownParent(header.parent_hash().clone()))?
.decode();
.decode()?;

let full_params = FullFamilyParams {
block_bytes: bytes,
Expand Down
32 changes: 17 additions & 15 deletions ethcore/sync/src/light_sync/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@

//! Helpers for decoding and verifying responses for headers.

use std::fmt;

use ethcore::encoded;
use ethcore::header::Header;
use ethcore::{self, encoded, header::Header};
use ethereum_types::H256;
use light::request::{HashOrNumber, CompleteHeadersRequest as HeadersRequest};
use rlp::DecoderError;
use ethereum_types::H256;
use std::fmt;

/// Errors found when decoding headers and verifying with basic constraints.
#[derive(Debug, PartialEq)]
Expand Down Expand Up @@ -74,19 +72,23 @@ pub trait Constraint {

/// Do basic verification of provided headers against a request.
pub fn verify(headers: &[encoded::Header], request: &HeadersRequest) -> Result<Vec<Header>, BasicError> {
let headers: Vec<_> = headers.iter().map(|h| h.decode()).collect();
let headers: Result<Vec<_>, _> = headers.iter().map(|h| h.decode() ).collect();
match headers {
Ok(headers) => {
let reverse = request.reverse;

Max(request.max as usize).verify(&headers, reverse)?;
match request.start {
HashOrNumber::Number(ref num) => StartsAtNumber(*num).verify(&headers, reverse)?,
HashOrNumber::Hash(ref hash) => StartsAtHash(*hash).verify(&headers, reverse)?,
}

let reverse = request.reverse;
SkipsBetween(request.skip).verify(&headers, reverse)?;

Max(request.max as usize).verify(&headers, reverse)?;
match request.start {
HashOrNumber::Number(ref num) => StartsAtNumber(*num).verify(&headers, reverse)?,
HashOrNumber::Hash(ref hash) => StartsAtHash(*hash).verify(&headers, reverse)?,
Ok(headers)
},
Err(e) => Err(e.into())
}

SkipsBetween(request.skip).verify(&headers, reverse)?;

Ok(headers)
}

struct StartsAtNumber(u64);
Expand Down
2 changes: 1 addition & 1 deletion ethcore/sync/src/light_sync/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ fn fork_post_cht() {
for id in (0..CHAIN_LENGTH).map(|x| x + 1).map(BlockId::Number) {
let (light_peer, full_peer) = (net.peer(0), net.peer(1));
let light_chain = light_peer.light_chain();
let header = full_peer.chain().block_header(id).unwrap().decode();
let header = full_peer.chain().block_header(id).unwrap().decode().expect("decoding failure");
let _ = light_chain.import_header(header);
light_chain.flush_queue();
light_chain.import_verified();
Expand Down
13 changes: 13 additions & 0 deletions rpc/src/v1/helpers/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,19 @@ pub fn transaction<T: Into<EthcoreError>>(error: T) -> Error {
}
}

pub fn decode<T: Into<EthcoreError>>(error: T) -> Error {
let error = error.into();
match *error.kind() {
ErrorKind::Decoder(ref dec_err) => rlp(dec_err.clone()),
_ => Error {
code: ErrorCode::InternalError,
message: "decoding error".into(),
data: None,
}

}
}

pub fn rlp(error: DecoderError) -> Error {
Error {
code: ErrorCode::InvalidParams,
Expand Down
13 changes: 8 additions & 5 deletions rpc/src/v1/impls/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,10 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> EthClient<C, SN, S
let uncle_id = UncleId { block: block_id, position };

let uncle = match client.uncle(uncle_id) {
Some(hdr) => hdr.decode(),
Some(hdr) => match hdr.decode() {
Ok(h) => h,
Err(e) => return Err(errors::decode(e))
},
None => { return Ok(None); }
};

Expand Down Expand Up @@ -851,9 +854,9 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
};

let state = try_bf!(self.client.state_at(id).ok_or(errors::state_pruned()));
let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()));
let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()).and_then(|h| h.decode().map_err(errors::decode)));

(state, header.decode())
(state, header)
};

let result = self.client.call(&signed, Default::default(), &mut state, &header);
Expand Down Expand Up @@ -890,9 +893,9 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
};

let state = try_bf!(self.client.state_at(id).ok_or(errors::state_pruned()));
let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()));
let header = try_bf!(self.client.block_header(id).ok_or(errors::state_pruned()).and_then(|h| h.decode().map_err(errors::decode)));

(state, header.decode())
(state, header)
};

Box::new(future::done(self.client.estimate_gas(&signed, &state, &header)
Expand Down
2 changes: 1 addition & 1 deletion rpc/src/v1/impls/light/eth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ impl<T: LightChainClient + 'static> Eth for EthClient<T> {
}

fn send_raw_transaction(&self, raw: Bytes) -> Result<RpcH256> {
let best_header = self.client.best_block_header().decode();
let best_header = self.client.best_block_header().decode().map_err(errors::decode)?;

Rlp::new(&raw.into_vec()).as_val()
.map_err(errors::rlp)
Expand Down
2 changes: 1 addition & 1 deletion rpc/src/v1/impls/light/parity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl Parity for ParityClient {

let engine = self.light_dispatch.client.engine().clone();
let from_encoded = move |encoded: encoded::Header| {
let header = encoded.decode();
let header = encoded.decode().expect("decoding error"); // REVIEW: not sure what to do here; what is a decent return value for the error case here?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it should be BoxFuture<Result<RichHeader>>.

if self.fetcher is fetching headers from the network, this may be a critical vulnerability in light clients

let extra_info = engine.extra_info(&header);
RichHeader {
inner: Header {
Expand Down
4 changes: 2 additions & 2 deletions rpc/src/v1/impls/parity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -487,9 +487,9 @@ impl<C, M, U, S> Parity for ParityClient<C, M, U> where
};

let state = self.client.state_at(id).ok_or(errors::state_pruned())?;
let header = self.client.block_header(id).ok_or(errors::state_pruned())?;
let header = self.client.block_header(id).ok_or(errors::state_pruned())?.decode().map_err(errors::decode)?;

(state, header.decode())
(state, header)
};

self.client.call_many(&requests, &mut state, &header)
Expand Down
Loading