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

ethcore client: fix a double Read Lock bug in fn Client::logs() #11172

Merged
merged 1 commit into from
Oct 16, 2019
Merged

ethcore client: fix a double Read Lock bug in fn Client::logs() #11172

merged 1 commit into from
Oct 16, 2019

Conversation

BurtonQin
Copy link
Contributor

Description
This PR fixes a double Read lock bug in ethcore client.
The two Read locks on self.chain reside in the same function Client::logs(), at the beginning and the end line of the function, respectively. The lock self.chain is a parking_lot::RwLock.

This bug is similar to #592

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. restore_db, etc) requires a write lock in between the execution of function logs(), a double lock may happen.

How I fix
Reuse the chain and remove the second lock.

@parity-cla-bot
Copy link

It looks like @BurtonQin hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

@BurtonQin
Copy link
Contributor Author

[clabot:check]

@parity-cla-bot
Copy link

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

Many thanks,

Parity Technologies CLA Bot

dvdplm
dvdplm approved these changes Oct 15, 2019
View reviewed changes
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

/cc @sorpaas

@dvdplm dvdplm requested a review from sorpaas October 15, 2019 20:36
@dvdplm dvdplm added A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 M4-core ⛓ Core client code / Rust. labels Oct 15, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented Oct 15, 2019

Thank you for spotting this. Just curious: did you observe the deadlock in the wild?

@niklasad1
Copy link
Collaborator

niklasad1 commented Oct 16, 2019 •
edited
Loading

@dvdplm it is not a deadlock because it is a RwLock where multiple readers are ok when there is no write acquired, which is the case here

@dvdplm
Copy link
Collaborator

dvdplm commented Oct 16, 2019

it is not a deadlock because it is a RwLock where multiple readers are ok when there is no write acquired, which is the case here

But the second read-lock taken here is unnecessary right? And if there is a write lock attempted in another thread, the second read-lock here could deadlock? (Seems like the only place we take the write lock is here, so it should be a rare problem I guess)

@niklasad1
Copy link
Collaborator

But the second read-lock taken here is unnecessary right?

Yes

And if there is a write lock attempted in another thread, the second read-lock here could deadlock? (Seems like the only place we take the write lock is here, so it should be a rare problem I guess)

Right, that it is correct. My bad it didn't read the PR description properly RwLock uses task-fair locking

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, nice catch

@sorpaas sorpaas merged commit 6b57429 into openethereum:master Oct 16, 2019
@ordian ordian added this to the 2.7 milestone Oct 16, 2019
@BurtonQin BurtonQin deleted the ClientDoubleLock branch October 16, 2019 18:59
This was referenced Oct 16, 2019
Merged
Closed
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
ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
6927d1d
dvdplm pushed a commit that referenced this pull request Nov 6, 2019
@dvdplm
ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172)
8b935ab
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

@sorpaas sorpaas sorpaas approved these changes

@dvdplm dvdplm dvdplm 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.
6 participants
@BurtonQin @parity-cla-bot @dvdplm @niklasad1 @sorpaas @ordian