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

util Host: fix a double Read Lock bug in fn Host::session_readable() #11175

Merged
merged 1 commit into from
Oct 17, 2019
Merged

util Host: fix a double Read Lock bug in fn Host::session_readable() #11175

merged 1 commit into from
Oct 17, 2019

Conversation

BurtonQin
Copy link
Contributor

Description
This PR fixes a double Read lock bug in util network-devp2p Host.
The lock self.handlers is a parking_lot::RwLock.
The first Read locks on self.handlers is on L836 in the function Host::session_readable()
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/util/network-devp2p/src/host.rs#L836-L845

Then it calls fn Host::kill_connection() and the second Read lock on self.handlers is on L943
https://github.com/paritytech/parity-ethereum/blob/6b57429d724c954ad5d64c1b1d42c746f8a4d08e/util/network-devp2p/src/host.rs#L931-L943

This bug is similar to #11172.

According to parking_lot::RwLock:
“readers trying to acquire the lock will block even if the lock is unlocked when there are writers waiting to acquire the lock.”
“attempts to recursively acquire a read lock within a single thread may result in a deadlock.”

Therefore, once a function (e.g. Host::message() on L1106) requires a write lock in between the execution of the two read locks, a dead lock may happen.

How I fix
Drop the handlers before calling kill_connection().
The fix method is the same as L382 of fn set_non_reserved_mode(),
dropping the first lock before calling functions that use the second lock.

@parity-cla-bot
Copy link

It looks like @BurtonQin signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Oct 16, 2019
niklasad1
niklasad1 approved these changes Oct 16, 2019
View reviewed changes
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Thanks again, nice catch

@niklasad1 niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 16, 2019
@ordian ordian added this to the 2.7 milestone Oct 17, 2019
@ordian ordian added the M4-core ⛓ Core client code / Rust. label Oct 17, 2019
ordian
ordian approved these changes Oct 17, 2019
View reviewed changes
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks! Although I doubt it could be triggered in practice, the networking code and the locking in general (#7079) would definitely need some love.

@ordian ordian merged commit f99819d into openethereum:master Oct 17, 2019
@BurtonQin BurtonQin deleted the HostDoubleReadLock branch October 17, 2019 19:00
dvdplm added a commit that referenced this pull request Oct 24, 2019
@dvdplm
Merge branch 'master' into dp/fix/debug-snapshot-start
99d5b61 
* master:
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  Remove unused macro_use. (#11191)
  [dependencies]: jsonrpc `14.0.1` (#11183)
  [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
  [dependencies] bump rand 0.7 (#11022)
  [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
  TxPermissions ver 3: gas price & data (#11170)
  [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123)
  util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
  ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
  Aura: Report malice on sibling blocks from the same validator (#11160)
dvdplm added a commit that referenced this pull request Oct 24, 2019
@dvdplm
Merge branch 'master' into dp/chore/put-snapshot-progress-behind-rwlock
01cf083 
* master:
  Pause pruning while snapshotting (#11178)
  Type annotation for next_key() matching of json filter options (#11192)
  Crypto primitives removed from ethkey (#11174)
  Made ecrecover implementation trait public (#11188)
  Remove unused macro_use. (#11191)
  [dependencies]: jsonrpc `14.0.1` (#11183)
  [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
  [dependencies] bump rand 0.7 (#11022)
  [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
  TxPermissions ver 3: gas price & data (#11170)
  [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123)
  util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
  ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
  Aura: Report malice on sibling blocks from the same validator (#11160)
This was referenced Nov 5, 2019
Merged
Merged
niklasad1 pushed a commit that referenced this pull request Nov 5, 2019
@niklasad1
util Host: fix a double Read Lock bug in fn Host::session_readable() (#…
44dc779 
…11175)
dvdplm pushed a commit that referenced this pull request Nov 6, 2019
@dvdplm
util Host: fix a double Read Lock bug in fn Host::session_readable() (#…
d7f8a94 
…11175)
s3krit pushed a commit that referenced this pull request Nov 11, 2019
@soc1c @s3krit
v2.5.10 stable (#11239)
f3cdd7b 
* ropsten #6631425 foundation #8798209 (#11201)
* [stable] builtin, istanbul and mordor testnet backports (#11234)
  * ethcore-builtin (#10850)
  * [builtin]: support `multiple prices and activations` in chain spec (#11039)
  * [chain specs]: activate `Istanbul` on mainnet (#11228)
  * ethcore/res: add mordor testnet configuration (#11200)
* Update list of bootnodes for xDai chain (#11236)
* ethcore: remove `test-helper feat` from build (#11047)
* Secret store: fix Instant::now() related race in net_keep_alive (#11155) (#11159)
* [stable]: backport #10691 and #10683 (#11143)
  * Fix compiler warning (that will become an error) (#10683)
  * Refactor Clique stepping (#10691)
* Add Constantinople eips to the dev (instant_seal) config (#10809)
* Add cargo-remote dir to .gitignore (?)
* Insert explicit warning into the panic hook (#11225)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Add new line after writing block to hex file. (#10984)
* Type annotation for next_key() matching of json filter options (#11192) (but no `FilterOption` in 2.5 so…)
* Upgrade jsonrpc to latest (#11206)
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11086)
* Fix block detail updating (#11015)
* Switching sccache from local to Redis (#10971)
* Made ecrecover implementation trait public (#11188)
* [dependencies]: jsonrpc `14.0.1` (#11183)
* [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Cleanup stratum a bit (#11161)
* Upgrade to jsonrpc v14 (#11151)
* SecretStore: expose restore_key_public in HTTP API (#10241)
s3krit pushed a commit that referenced this pull request Nov 11, 2019
@soc1c @s3krit
v2.6.5 beta (#11240)
bc90ba2 
* [CI] check evmbin build (#11096)
* Correct EIP-712 encoding (#11092)
* [client]: Fix for incorrectly dropped consensus messages (#11082) (#11086)
* Update hardcoded headers (foundation, classic, kovan, xdai, ewc, ...) (#11053)
* Add cargo-remote dir to .gitignore (?)
* Update light client headers: ropsten 6631425 foundation 8798209 (#11201)
* Update list of bootnodes for xDai chain (#11236)
* ethcore/res: add mordor testnet configuration (#11200)
* [chain specs]: activate Istanbul on mainnet (#11228)
* [builtin]: support multiple prices and activations in chain spec (#11039)
* [receipt]: add sender & receiver to RichReceipts (#11179)
* [ethcore/builtin]: do not panic in blake2pricer on short input (#11180)
* Made ecrecover implementation trait public (#11188)
* Fix docker centos build (#11226)
* Update MIX bootnodes. (#11203)
* Insert explicit warning into the panic hook (#11225)
* Use provided usd-per-eth value if an endpoint is specified (#11209)
* Cleanup stratum a bit (#11161)
* Add Constantinople EIPs to the dev (instant_seal) config (#10809) (already backported)
* util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175)
* ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
* Type annotation for next_key() matching of json filter options (#11192)
* Upgrade jsonrpc to latest (#11206)
* [dependencies]: jsonrpc 14.0.1 (#11183)
* Upgrade to jsonrpc v14 (#11151)
* Switching sccache from local to Redis (#10971)
* Snapshot restoration overhaul (#11219)
* Add new line after writing block to hex file. (#10984)
* Pause pruning while snapshotting (#11178)
* Change how RPCs eth_call and eth_estimateGas handle "Pending" (#11127)
* Fix block detail updating (#11015)
* Make InstantSeal Instant again #11186
* Filter out some bad ropsten warp snapshots (#11247)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.
4 participants
@BurtonQin @parity-cla-bot @ordian @niklasad1