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

ethcore/client: fix deadlock caused by double-read lock and conflicting lock order #11766

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ethcore/client: fix deadlock caused by double-read lock and conflicting lock order #11766

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a380494
ethcore/client: fix deadlock caused by double read lock
BurtonQin Jun 5, 2020
e616bda
ethcore/client: fix deadlock caused by conflicting lock order
BurtonQin Jun 5, 2020
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
59 changes: 32 additions & 27 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use ethereum_types::{Address, H256, H264, U256};
use hash::keccak;
use hash_db::EMPTY_PREFIX;
use kvdb::{DBTransaction, DBValue, KeyValueDB};
use parking_lot::{Mutex, RwLock};
use parking_lot::{Mutex, MutexGuard, RwLock, RwLockReadGuard};
use rand::rngs::OsRng;
use rlp::PayloadInfo;
use rustc_hex::FromHex;
Expand Down Expand Up @@ -424,7 +424,7 @@ impl Importer {
};

// Enact Verified Block
let last_hashes = client.build_last_hashes(*header.parent_hash());
let last_hashes = client.build_last_hashes(*header.parent_hash(), &chain);
let db = client.state_db.read().boxed_clone_canon(header.parent_hash());

let is_epoch_begin = chain.epoch_transition(parent.number(), *header.parent_hash()).is_some();
Expand Down Expand Up @@ -465,6 +465,7 @@ impl Importer {
return Err(e);
}

drop(chain);
let pending = self.check_epoch_end_signal(
&header,
&locked_block.receipts,
Expand All @@ -479,9 +480,8 @@ 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, unverified: Unverified, receipts_bytes: &[u8], db: &dyn KeyValueDB, chain: &BlockChain) -> EthcoreResult<()> {
fn import_old_block(&self, unverified: Unverified, receipts_bytes: &[u8], db: &dyn KeyValueDB, chain: &BlockChain, _import_lock: MutexGuard<()>) -> EthcoreResult<()> {
let receipts = ::rlp::decode_list(receipts_bytes);
let _import_lock = self.import_lock.lock();

{
trace_time!("import_old_block");
Expand Down Expand Up @@ -633,7 +633,7 @@ impl Importer {
author: *header.author(),
timestamp: header.timestamp(),
difficulty: *header.difficulty(),
last_hashes: client.build_last_hashes(*header.parent_hash()),
last_hashes: client.build_last_hashes(*header.parent_hash(), &client.chain.read()),
gas_used: U256::default(),
gas_limit: u64::max_value().into(),
};
Expand Down Expand Up @@ -911,14 +911,14 @@ impl Client {
author: header.author(),
timestamp: header.timestamp(),
difficulty: header.difficulty(),
last_hashes: self.build_last_hashes(header.parent_hash()),
last_hashes: self.build_last_hashes(header.parent_hash(), &self.chain.read()),
gas_used: U256::default(),
gas_limit: header.gas_limit(),
}
})
}

fn build_last_hashes(&self, parent_hash: H256) -> Arc<LastHashes> {
fn build_last_hashes(&self, parent_hash: H256, chain: &RwLockReadGuard<Arc<BlockChain>>) -> Arc<LastHashes> {
{
let hashes = self.last_hashes.read();
if hashes.front().map_or(false, |h| h == &parent_hash) {
Expand All @@ -930,7 +930,6 @@ impl Client {
let mut last_hashes = LastHashes::new();
last_hashes.resize(256, H256::zero());
last_hashes[0] = parent_hash;
let chain = self.chain.read();
for i in 0..255 {
match chain.block_details(&last_hashes[i]) {
Some(details) => {
Expand Down Expand Up @@ -1030,7 +1029,7 @@ impl Client {

/// Access state from tests
#[cfg(any(test, feature = "test-helpers"))]
pub fn state_db(&self) -> ::parking_lot::RwLockReadGuard<StateDB> {
pub fn state_db(&self) -> RwLockReadGuard<StateDB> {
self.state_db.read()
}

Expand Down Expand Up @@ -1268,12 +1267,12 @@ impl Client {
}
}

fn block_number_ref(&self, id: &BlockId) -> Option<BlockNumber> {
fn block_number_ref(&self, id: &BlockId, chain: &RwLockReadGuard<Arc<BlockChain>>) -> Option<BlockNumber> {
match *id {
BlockId::Number(number) => Some(number),
BlockId::Hash(ref hash) => self.chain.read().block_number(hash),
BlockId::Hash(ref hash) => chain.block_number(hash),
BlockId::Earliest => Some(0),
BlockId::Latest => Some(self.chain.read().best_block_number()),
BlockId::Latest => Some(chain.best_block_number()),
}
}

Expand All @@ -1300,12 +1299,12 @@ impl DatabaseRestore for Client {
trace!(target: "snapshot", "Replacing client database with {:?}", new_db);

let _import_lock = self.importer.import_lock.lock();
let mut state_db = self.state_db.write();
let mut chain = self.chain.write();
let mut tracedb = self.tracedb.write();
self.importer.miner.clear();
let db = self.db.write();
db.restore(new_db)?;
let mut state_db = self.state_db.write();

let cache_size = state_db.cache_size();
*state_db = StateDB::new(journaldb::new(db.key_value().clone(), self.pruning, ::db::COL_STATE), cache_size);
Expand Down Expand Up @@ -1529,7 +1528,7 @@ impl Call for Client {
author: *header.author(),
timestamp: header.timestamp(),
difficulty: *header.difficulty(),
last_hashes: self.build_last_hashes(*header.parent_hash()),
last_hashes: self.build_last_hashes(*header.parent_hash(), &self.chain.read()),
gas_used: U256::default(),
gas_limit: U256::max_value(),
};
Expand All @@ -1544,7 +1543,7 @@ impl Call for Client {
author: *header.author(),
timestamp: header.timestamp(),
difficulty: *header.difficulty(),
last_hashes: self.build_last_hashes(*header.parent_hash()),
last_hashes: self.build_last_hashes(*header.parent_hash(), &self.chain.read()),
gas_used: U256::default(),
gas_limit: U256::max_value(),
};
Expand All @@ -1571,7 +1570,7 @@ impl Call for Client {
author: *header.author(),
timestamp: header.timestamp(),
difficulty: *header.difficulty(),
last_hashes: self.build_last_hashes(*header.parent_hash()),
last_hashes: self.build_last_hashes(*header.parent_hash(), &self.chain.read()),
gas_used: U256::default(),
gas_limit: max,
};
Expand Down Expand Up @@ -1746,7 +1745,7 @@ impl BlockChainClient for Client {
}

fn block_number(&self, id: BlockId) -> Option<BlockNumber> {
self.block_number_ref(&id)
self.block_number_ref(&id, &self.chain.read())
}

fn block_body(&self, id: BlockId) -> Option<encoded::Body> {
Expand Down Expand Up @@ -1997,11 +1996,11 @@ impl BlockChainClient for Client {
// If we are sure the block does not exist (where val > best_block_number), then return error. Note that we
// don't need to care about pending blocks here because RPC query sets pending back to latest (or handled
// pending logs themselves).
let from = match self.block_number_ref(&filter.from_block) {
let from = match self.block_number_ref(&filter.from_block, &chain) {
Some(val) if val <= chain.best_block_number() => val,
_ => return Err(filter.from_block),
};
let to = match self.block_number_ref(&filter.to_block) {
let to = match self.block_number_ref(&filter.to_block, &chain) {
Some(val) if val <= chain.best_block_number() => val,
_ => return Err(filter.to_block),
};
Expand Down Expand Up @@ -2130,7 +2129,8 @@ impl BlockChainClient for Client {
}

fn last_hashes(&self) -> LastHashes {
self.build_last_hashes(self.chain.read().best_block_hash()).to_vec()
let chain = self.chain.read();
self.build_last_hashes(chain.best_block_hash(), &chain).to_vec()
}

fn transactions_to_propagate(&self) -> Vec<Arc<VerifiedTransaction>> {
Expand Down Expand Up @@ -2271,12 +2271,17 @@ impl IoClient for Client {
let first = queued.write().1.pop_front();
if let Some((unverified, receipts_bytes)) = first {
let hash = unverified.hash();
let result = client.importer.import_old_block(
unverified,
&receipts_bytes,
&**client.db.read().key_value(),
&*client.chain.read(),
);
let result = {
let import_lock = client.importer.import_lock.lock();
let chain = client.chain.read();
client.importer.import_old_block(
unverified,
&receipts_bytes,
&**client.db.read().key_value(),
&*chain,
import_lock,
)
};
if let Err(e) = result {
error!(target: "client", "Error importing ancient block: {}", e);

Expand Down Expand Up @@ -2365,7 +2370,7 @@ impl PrepareOpenBlock for Client {
self.tracedb.read().tracing_enabled(),
self.state_db.read().boxed_clone_canon(&h),
&best_header,
self.build_last_hashes(h),
self.build_last_hashes(h, &chain),
author,
gas_range_target,
extra_data,
Expand Down