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

les: protect field access with lock to avoid possible data race #20691

Closed
wants to merge 2 commits into from
Closed

les: protect field access with lock to avoid possible data race #20691

wants to merge 2 commits into from

Conversation

BurtonQin
Copy link
Contributor

@BurtonQin BurtonQin commented Feb 19, 2020

Fixed inconsistency and also potential data race in les/client_handler.go and les/clientpool.go:
e.g. In les/client_handler.go:
p.headInfo is read/written 11 times in les/peer.go and les/fetcher.go; 10 out of 11 times it is protected by q.lock.RLock()/Lock(); 1 out of 11 times it is read without a Lock, which is in func handle() on L128.
A data race may happen when handle() and other func like announce() are called in parallel.
The fix is to protect the access to p.headInfo and save the result to a local variable.
I am not sure if announce(p, headInfo) will have an atomicity violation after this fix.

A similar bug is an unprotected access to f.capLimit in setCapacity() in les/clientpool.go.
I wonder if this function has some special calling guarantee to avoid data race on f.capLimit, since all other functions protect the field with f.lock.Lock().

See #20651

@rjl493456442
Copy link
Member

I think like the entire setCapacity function should hold the lock before we access some sensitive data. It's changed in the new clientpool.

@fjl
Copy link
Contributor

fjl commented Mar 10, 2020

These issues will be fixed in the next protocol version. We don't want to add the changes now because they will cause a rebase conflict with the new code.

@fjl fjl closed this Mar 10, 2020
@fjl fjl removed the status:triage label Jul 1, 2020
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.

4 participants