les: A possible data race in setCapacity() due to inconsistent field protection

[BurtonQin]

System information

Geth version: 1.9.10
OS & Version: Windows/Linux/OSX
Commit hash : 8045504

Expected behaviour

A possible data race in func setCapacity() in les/clientpool.go.
f.capLimit is read/written 9 times; 7 out of 9 times it is protected by f.lock.Lock()
; 2 out of 9 times it is read without a Lock, which are in func setCapacity().

go-ethereum/les/clientpool.go

Lines 484 to 494 in 8045504

	if f.connectedCap > f.capLimit {
	var kickList []*clientInfo
	kick := true
	f.connectedQueue.MultiPop(func(data interface{}, priority int64) bool {
	client := data.(*clientInfo)
	kickList = append(kickList, client)
	f.connectedCap -= client.capacity
	if client == c {
	kick = false
	}
	return kick && (f.connectedCap > f.capLimit)

A data race may happen when setCapacity() and other func like setLimits are called in parallel.
I wonder if developers forgot to protect f.capLimit by f.lock.Lock() in func setCapacity() or there are some special calling rules on setCapacity() to guarantee the protection.

Actual behaviour

No, I found it through static analysis.

Steps to reproduce the behaviour

No.

Backtrace

No.

[BurtonQin]

Similarly, a possible data race is in func handle() in les/fetcher.go.
p.headInfo is not protected by p.lock.Lock()

go-ethereum/les/client_handler.go

Line 128 in 8045504

h.fetcher.announce(p, p.headInfo)

But it is protected by p.lock.Lock() or p.lock.Rlock() in all other places (1 place in fetcher.go, 10 places in peer.go)

go-ethereum/les/fetcher.go

Lines 348 to 351 in 8045504

	p.lock.Lock()
	p.headInfo = head
	fp.lastAnnounced = n
	p.lock.Unlock()

A data race may happen when func handle() and other func like announce() are called in parallel.
I wonder if developers forgot to protect p.headInfo in func handle() or there are some special calling rules on handle() to guarantee the protection.
A possible fix is to add a boolean parameter to f.announce(), indicating whether it is locked or not. If locked, we won't use p.lock.Lock() inside it.

[zsfelfoldi]

This is actually safe, though probably not the nicest piece of code because it is hard to see why it is safe. clientPool.setCapacity should only be called while clientPool.lock is locked and this is what really happens because it is called in api.go from a callback of clientPool.forClients which already locks the pool lock.
Btw a major refactoring of the client pool is happening right now because new features are being added and the existing code was hacky enough already. It is WIP but close to being finished, hopefully it will make things cleaner.
#21236