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/vet: copylock does not warn when "lock_type_var = *func_return_pointer_to_locktype()" #32550
Comments
Please provide Playground links instead of bare source files when possible, and format Code blocks using |
Here's a diff to golang/tools that adds a test case to repro: diff --git go/analysis/passes/copylock/testdata/src/a/copylock.go go/analysis/passes/copylock/testdata/src/a/copylock.go
index 57d40765..217b6e4c 100644
--- go/analysis/passes/copylock/testdata/src/a/copylock.go
+++ go/analysis/passes/copylock/testdata/src/a/copylock.go
@@ -53,8 +53,9 @@ func BadFunc() {
*tp = t // want `assignment copies lock value to \*tp: a.Tlock contains sync.Once contains sync.Mutex`
t = *tp // want "assignment copies lock value to t: a.Tlock contains sync.Once contains sync.Mutex"
- y := *x // want "assignment copies lock value to y: sync.Mutex"
- var z = t // want "variable declaration copies lock value to z: a.Tlock contains sync.Once contains sync.Mutex"
+ y := *x // want "assignment copies lock value to y: sync.Mutex"
+ var z = t // want "variable declaration copies lock value to z: a.Tlock contains sync.Once contains sync.Mutex"
+ t = *new(sync.Mutex) // want "variable declaration copies lock value to t: a.Tlock contains sync.Once contains sync.Mutex"
w := struct{ L sync.Mutex }{
L: *x, // want `literal copies lock value from \*x: sync.Mutex` (I'm here because I ran into the same bug, after hunting down a real lock-copy deadlock.) The same applies if |
It looks like the issue is that |
This code is intentional. See here: https://go-review.googlesource.com/c/go/+/24711/ Maybe we should improve the comment? |
Alright, if this is intended feel free to close. The comment is fine, what was less clear to me -- based on its other behavior -- is that copylocks aims to be at all conservative about detecting only copies of nonzero locks. (So if there's anything to document further, it would be that.) In particular, it falsely reports several other ambiguous cases, as well as some that obviously only copy a zero lock: func newMutex() *sync.Mutex {
return new(sync.Mutex) // or it could lock the mutex first!
}
func caught() {
var x *m
_ = *x // [copylocks] assignment copies lock value to _: sync.Mutex
y := newMutex()
_ = *y // [copylocks] assignment copies lock value to _: sync.Mutex
}
func missed() {
_ = *newMutex()
} In general, I totally understand the impulse to be conservative about false positives. But here, a less conservative rule is plenty clear and in my experience not onerous: don't copy a lock unless you can prove it's zero. (Or, if you prefer, never copy a lock.) This has the advantage of being easy to implement consistently! But I admit after spending 2 hours hunting down a deadlock caused by a copied lock, I may be biased ;-) . Edit: one option is to do what I propose, but try to do it better: walk the call graph to determine which locks we know must be zero. I suspect this could handle all the cases from #16227, but it would definitely add complexity, so if that complexity is of unclear value to us (we're probably okay with a stricter rule and can do so in a fork) and the go team is okay with it as-is, maybe there's no point. |
I'd be okay with the option in your edit in theory, but I'm not convinced it's possible to get it right, and don't know if it's worth the effort. |
Note that this bug means that the check is not triggered for structs that embed mutexes, not just for functions that return mutexes. (This was not immediately clear to me, since the test case provided in the description has been reduced). I just ran into this in production code, where it was causing a bug because the constructor function spawned a goroutine, which then used a different mutex than the caller. The following Go program and playground link demonstrates the same bug with an embedded mutex: https://go.dev/play/p/x6zjlwQoa52 package main
import (
"fmt"
"sync"
)
type UseMutex struct {
mu sync.Mutex
foo int
}
func NewUseMutex() *UseMutex {
return &UseMutex{}
}
func (u *UseMutex) doSomething() {
fmt.Printf("UseMutex u=%p\n", u)
}
func main() {
// no vet warning printed
copyUM := *NewUseMutex()
copyUM.doSomething()
um := NewUseMutex()
// assignment copies lock value to copyUM: github.com/evanj/example.UseMutex contains sync.Mutex
copyUM = *um
copyUM.doSomething()
} |
I am using Go 1.12.1.
cmd/vet does not report any warning for example code below:
I read code of the analyzer copylock.go:
my confusion is why line 234 just return nil without check the return type of the function (BTW, in my example code, "return &nc" will pass the "checkCopyLocksReturnStmt" check, b/c the return type is a pointer). All this result to the mute of cmd/vet.
Could this case be caught by cmd/vet in future?
The text was updated successfully, but these errors were encountered: