Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/faucet: protect f.reqs with Rlock to prevent data race #20669

Merged
merged 3 commits into from
Feb 15, 2020
Merged

cmd/faucet: protect f.reqs with Rlock to prevent data race #20669

merged 3 commits into from
Feb 15, 2020

Conversation

BurtonQin
Copy link
Contributor

In cmd/faucet/faucet.go,
f.reqs is protected by f.lock.Lock() or f.lock.Rlock() at five places.
The only place where it is not protected is on L367 in func apiHandler().
This may cause data race when apiHandler() and other func like refresh() where f.reqs is written to are called in parallel.
The fix is to use f.lock.Rlock() to protect f.reqs.

@karalabe
Copy link
Member

Thanks for the PR, however this will lock the faucet for the entire duration of the network transmission, which is not a good idea. If f.reqs needs to be protected, we need to make a locked copy and then transmit with the lock released.

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

This change does not do a deep copy of f.reqs, so it's not ultra safe, but we only ever append and slice it (never update an entry), so it should be fine.

@karalabe karalabe added this to the 1.9.11 milestone Feb 15, 2020
@karalabe karalabe merged commit 37531b1 into ethereum:master Feb 15, 2020
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
…20669)

* cmd/faucet: add Rlock to protect f.reqs in apiHandler

* cmd/faucet: make a locked copy of f.reqs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants