x/tools/go/analysis/passes/copylock: fails to report re-assignment of sync.WaitGroup variables

[merrickclay]

Proposal Details

Reassignment of variables of type sync.WaitGroup is likely to cause bugs due to the potential to overwrite a WaitGroup with a non-zero counter value with a WaitGroup that has a zero counter. I can't think of any scenarios whereby reassigning a variable of type sync.WaitGroup would be required or useful. The waitgroup analysis pass should be extended to report reassignments of sync.WaitGroup variables.

Example

package main

import (
	"sync"
)

type Object struct {
	wg sync.WaitGroup
}

var wg0 sync.WaitGroup

func main() {
	var wg1 sync.WaitGroup                    // ok
	wg2 := sync.WaitGroup{}                   // ok
	var wg3 sync.WaitGroup = sync.WaitGroup{} // ok

	obj := Object{
		wg: sync.WaitGroup{},  // ok
	}

	wg0 = sync.WaitGroup{} // want: variable of type sync.WaitGroup reassigned
	wg1 = sync.WaitGroup{} // want: variable of type sync.WaitGroup reassigned

	obj.wg = sync.WaitGroup{} // want: variable of type sync.WaitGroup reassigned

	wg0.Wait()
	wg1.Wait()
	wg2.Wait()
	wg3.Wait()
}

Should this proposal be approved, I'd like to contribute the aforementioned changes.

[adonovan]

Thanks for reporting this; it's a regression.

The existing copylock check in cmd/vet should report this problem already. It was historically intended to prevent accidental copying of sync.Mutexes, which it abstracted as "any value with a Lock and Unlock method", but really the crux of the problem is that Mutexes and WaitGroups both contain runtime semaphores, which are particularly disastrous to copy. So WaitGroup now contains a nonce field with these dummy methods to trigger the check. And the copylock check even has tests to ensure that it detects this kind of mistake, but somehow they are passing.

I will investigate.

[adonovan]

Should this proposal be approved, I'd like to contribute the aforementioned changes.
[...] I will investigate.

Actually: if you are interested to look into this, it would be much appreciated.

[merrickclay]

I wasn't aware the copylock pass was supposed to handle this. I'll take a look!

[merrickclay]

It appears copylock does not report any issues with the example above because it only checks for locks that are passed by value or copied, not lock variables that are reassigned.

copylock catches wg0 = wg1 because wg1 is being copied, but it does not catch wg0 = sync.WaitGroup{} because there is no lock being copied. I also don't see any reassignment without copying in the tests, which would explain why they are passing.

This isn't unique to WaitGroups. The following reassignment of a sync.Mutex does not get reported either.

var mx sync.Mutex

mx.Lock()
mx = sync.Mutex{} // is not reported
mx.Unlock()

I propose we report any time that a variable containing a lock gets assigned a value using the ASSIGN (=) token.

Considering there aren't any locks being "copied" in the above example, does it make sense to add checks for these scenarios in the copylock pass or a new pass?

[adonovan]

Thanks for investigating. I assume these cases were not implemented for a reason (as it must have required more effort than the general case); too bad it wasn't written down. By all means make those changes, then test the tool on a substantial body of code and sample some of the new errors to see what fraction are true positives (mistakes leading to actual bugs) versus opposed to false positives (which include code that is less than ideal but not actually wrong). If the signal is good, then I'll run it over the entire module mirror corpus.

[merrickclay]

Here are my thoughts so far:

There are some lock types for which reassignment is common and not necessarily an issue (e.g. Sync.Once). Therefore, we'd want to limit our reports to reassignments of variables of specific types (e.g. Mutex, RWMutex, WaitGroup) wherein reassignment in non-bug scenarios has no benefits (pre-assignment state == post-assignment state).

The naive approach would be to report all assignments to relevant lock-type variables where the ASSIGN token was used.

This approach would not catch reassignments that use the DEFINE token, as in the case of assigning multiple values at once, one or more of which has not been defined yet. In order to also catch these reassignments, we'd have to do some more complex analysis to check if the variable had already been assigned a value (and confirm that the assignment doesn't shadow an already defined variable of the same name).

The obvious potential false positive would be reported in the case that an unlocked Mutex or WaitGroup with a "zero counter value" are reassigned. Although I wouldn't ever suggest reassignment of a Mutex or WaitGroup variable, it's not technically a bug in this case. It is, however, completely unnecessary, though, as the state of the lock post-reassignment is the same as pre-reassignment in this scenario.

I'd argue, however, that a reassignment of a Mutex, RWMutex should always be reported (bar two exceptions), even if the reassignment occurs immediately after an unlock. Mutexes are inherently designed to be used across multiple goroutines, meaning any reassignment, even after an unlock, could result in a race condition if another thread locks the mutex in between the unlock and reassignment.

The first exception is when a Mutex is reassigned immediately after it's been defined within a method or function. Though unnecessary, it's not a bug.

var mutex sync.Mutex
mutex = sync.Mutex{}

The second is when the Mutex is used by only one goroutine and the reassignment occurs after an unlock.

A similar argument that reassignment should never occur may exist for sync.WaitGroup and other lock types.

[merrickclay]

I'm interested to hear thoughts on whether the naive approach would provide value as part of cmd/vet, given the known false positive and false negative scenarios.

I know it can be hard to judge the value without writing a solution and running it against a lot of existing code, which I hope to get around to doing. I'll shamefully admit that I'm having an unreasonably difficult time building, installing, and running cmd/vet from source (mostly having trouble installing it).

[adonovan]

There are some lock types for which reassignment is common and not necessarily an issue (e.g. Sync.Once).

I'm not sure what you mean: a sync.Once is a synchronization primitive, so copying one is just as harmful as copying a Mutex or a WaitGroup. (Indeed, it is implemented using a Mutex.) So I don't see how it could belong to a different category.

Is your point that people commonly re-set values containing a sync.Once in ways that are in fact safe, despite the lock-copying involved?

Therefore, we'd want to limit our reports to reassignments of variables of specific types (e.g. Mutex, RWMutex, WaitGroup) wherein reassignment in non-bug scenarios has no benefits (pre-assignment state == post-assignment state).

Beware that even a self-assignment mu = mu in the presence of concurrency is a potential data race, even though there is no apparent change in the value, because the read and write occur at different times, and so it is possible that this causes some other write to the mutex to be lost.

The naive approach would be to report all assignments to relevant lock-type variables where the ASSIGN token was used.

This approach would not catch reassignments that use the DEFINE token, as in the case of assigning multiple values at once, one or more of which has not been defined yet. In order to also catch these reassignments, we'd have to do some more complex analysis to check if the variable had already been assigned a value (and confirm that the assignment doesn't shadow an already defined variable of the same name).

FWIW, our analyzers routinely consider a statement such as x, y := f(), g() to be an assignment to each left-side var that was already declared.

The obvious potential false positive would be reported in the case that an unlocked Mutex or WaitGroup with a "zero counter value" are reassigned. Although I wouldn't ever suggest reassignment of a Mutex or WaitGroup variable, it's not technically a bug in this case. It is, however, completely unnecessary, though, as the state of the lock post-reassignment is the same as pre-reassignment in this scenario.

I'd argue, however, that a reassignment of a Mutex, RWMutex should always be reported (bar two exceptions), even if the reassignment occurs immediately after an unlock. Mutexes are inherently designed to be used across multiple goroutines, meaning any reassignment, even after an unlock, could result in a race condition if another thread locks the mutex in between the unlock and reassignment.

The first exception is when a Mutex is reassigned immediately after it's been defined within a method or function. Though unnecessary, it's not a bug.

var mutex sync.Mutex
mutex = sync.Mutex{}
The second is when the Mutex is used by only one goroutine and the reassignment occurs after an unlock.

A similar argument that reassignment should never occur may exist for sync.WaitGroup and other lock types.
I'm interested to hear thoughts on whether the naive approach would provide value as part of cmd/vet, given the known false positive and false negative scenarios.
I know it can be hard to judge the value without writing a solution and running it against a lot of existing code, which I hope to get around to doing.

This seems like a reasonable heuristic, but "empirically" is really the only good way to evaluate these things, I'm afraid.

I'll shamefully admit that I'm having an unreasonably difficult time building, installing, and running cmd/vet from source (mostly having trouble installing it).

Tip: don't use cmd/vet as your testbed; build and run the analyzer's singlechecker executable (golang.org/x/tools/go/analysis/passes/copylock/main.go), or run the tests (adjacent).