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/vet: copylock does not warn when "lock_type_var = *func_return_pointer_to_locktype()" #32550

Open
cracker-ww opened this issue Jun 11, 2019 · 7 comments

Comments

@cracker-ww
Copy link

@cracker-ww cracker-ww commented Jun 11, 2019

I am using Go 1.12.1.

cmd/vet does not report any warning for example code below:

package main

type noCopy struct{}

func (*noCopy) Lock()              {}
func (*noCopy) Unlock()            {}
func (*noCopy) MuteUnusedWarning() {}

func returnNCP() *noCopy {
	var nc noCopy
	return &nc
}

func main() {
	var nc noCopy
	nc = *returnNCP()
	nc.MuteUnusedWarning()
}

I read code of the analyzer copylock.go:

func lockPathRhs(pass *analysis.Pass, x ast.Expr) typePath {
   224     if _, ok := x.(*ast.CompositeLit); ok {
   225         return nil
   226     }
   227     if _, ok := x.(*ast.CallExpr); ok {
   228         // A call may return a zero value.
   229         return nil
   230     }
   231     if star, ok := x.(*ast.StarExpr); ok {
   232         if _, ok := star.X.(*ast.CallExpr); ok {
   233             // A call may return a pointer to a zero value.
   234             return nil
   235     }

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?

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 14, 2019

Please provide Playground links instead of bare source files when possible, and format Code blocks using ```go Markdown blocks so that syntax highlighting works.

@bcmills bcmills changed the title copylock in cmd/vet do not warn when "lock_type_var = *func_return_pointer_to_locktype()" cmd/vet: copylock does not warn when "lock_type_var = *func_return_pointer_to_locktype()" Jun 14, 2019
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 14, 2019

@bcmills bcmills added this to the Unplanned milestone Jun 14, 2019
@benjaminjkraft
Copy link

@benjaminjkraft benjaminjkraft commented Dec 14, 2019

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 new is replaced by an arbitrary function returning *sync.Mutex.

@benjaminjkraft
Copy link

@benjaminjkraft benjaminjkraft commented Dec 14, 2019

It looks like the issue is that copylock.go:234 explicitly rejects the case where the pointer was returned from a function, because it could still be a pointer to the zero mutex. But the rest of the code seems to object if you copy any mutex, even a zero mutex where it's technically okay. (In fact the tests never bother to use the mutex, they just copy it.) Given that, it seems like it would be acceptable to report an issue here, perhaps by simply removing copylock.go:231-236, and perhaps one or both of the preceding ifs?

@matloob
Copy link
Contributor

@matloob matloob commented Dec 17, 2019

This code is intentional. See here: https://go-review.googlesource.com/c/go/+/24711/

Maybe we should improve the comment?

@benjaminjkraft
Copy link

@benjaminjkraft benjaminjkraft commented Dec 17, 2019

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.

@matloob
Copy link
Contributor

@matloob matloob commented Dec 23, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.