From: Russ Cox Date: Sun, 16 Aug 2015 00:50:17 +0000 (-0400) Subject: cmd/vet: diagnose plain assignment in copylock detector X-Git-Tag: go1.6beta1~1119 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=d8384e9a8f35efff15c8f62187dc793f79b7a355;p=gostls13.git cmd/vet: diagnose plain assignment in copylock detector It went out of its way to look for implicit assignments but never checked explicit assignments. This detects the root bug for #12099. Change-Id: I6a6e774cc38749ea8be7cfd58ba6421247b67000 Reviewed-on: https://go-review.googlesource.com/13646 Reviewed-by: Josh Bleecher Snyder Reviewed-by: Rob Pike --- diff --git a/src/cmd/vet/copylock.go b/src/cmd/vet/copylock.go index 6c71061b3e..8d8399a851 100644 --- a/src/cmd/vet/copylock.go +++ b/src/cmd/vet/copylock.go @@ -18,7 +18,7 @@ func init() { register("copylocks", "check that locks are not passed by value", checkCopyLocks, - funcDecl, rangeStmt, funcLit) + funcDecl, rangeStmt, funcLit, assignStmt) } // checkCopyLocks checks whether node might @@ -31,6 +31,18 @@ func checkCopyLocks(f *File, node ast.Node) { checkCopyLocksFunc(f, node.Name.Name, node.Recv, node.Type) case *ast.FuncLit: checkCopyLocksFunc(f, "func", nil, node.Type) + case *ast.AssignStmt: + checkCopyLocksAssign(f, node) + } +} + +// checkCopyLocksAssign checks whether an assignment +// copies a lock. +func checkCopyLocksAssign(f *File, as *ast.AssignStmt) { + for _, x := range as.Lhs { + if path := lockPath(f.pkg.typesPkg, f.pkg.types[x].Type); path != nil { + f.Badf(x.Pos(), "assignment copies lock value to %v: %v", f.gofmt(x), path) + } } } @@ -42,7 +54,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func if recv != nil && len(recv.List) > 0 { expr := recv.List[0].Type if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil { - f.Badf(expr.Pos(), "%s passes Lock by value: %v", name, path) + f.Badf(expr.Pos(), "%s passes lock by value: %v", name, path) } } @@ -50,7 +62,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func for _, field := range typ.Params.List { expr := field.Type if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil { - f.Badf(expr.Pos(), "%s passes Lock by value: %v", name, path) + f.Badf(expr.Pos(), "%s passes lock by value: %v", name, path) } } } @@ -59,7 +71,7 @@ func checkCopyLocksFunc(f *File, name string, recv *ast.FieldList, typ *ast.Func for _, field := range typ.Results.List { expr := field.Type if path := lockPath(f.pkg.typesPkg, f.pkg.types[expr].Type); path != nil { - f.Badf(expr.Pos(), "%s returns Lock by value: %v", name, path) + f.Badf(expr.Pos(), "%s returns lock by value: %v", name, path) } } } @@ -100,7 +112,7 @@ func checkCopyLocksRangeVar(f *File, rtok token.Token, e ast.Expr) { return } if path := lockPath(f.pkg.typesPkg, typ); path != nil { - f.Badf(e.Pos(), "range var %s copies Lock: %v", f.gofmt(e), path) + f.Badf(e.Pos(), "range var %s copies lock: %v", f.gofmt(e), path) } } diff --git a/src/cmd/vet/testdata/copylock.go b/src/cmd/vet/testdata/copylock.go new file mode 100644 index 0000000000..03d0c33f36 --- /dev/null +++ b/src/cmd/vet/testdata/copylock.go @@ -0,0 +1,28 @@ +package testdata + +import "sync" + +func OkFunc() { + var x *sync.Mutex + p := x + var y sync.Mutex + p = &y +} + +type Tlock struct { + once sync.Once +} + +func BadFunc() { + var x *sync.Mutex + p := x + var y sync.Mutex + p = &y + *p = *x // ERROR "assignment copies lock value to \*p: sync.Mutex" + + var t Tlock + var tp *Tlock + tp = &t + *tp = t // ERROR "assignment copies lock value to \*tp: testdata.Tlock contains sync.Once contains sync.Mutex" + t = *tp // ERROR "assignment copies lock value to t: testdata.Tlock contains sync.Once contains sync.Mutex" +} diff --git a/src/cmd/vet/testdata/copylock_func.go b/src/cmd/vet/testdata/copylock_func.go index d83957fe18..62725d9fa1 100644 --- a/src/cmd/vet/testdata/copylock_func.go +++ b/src/cmd/vet/testdata/copylock_func.go @@ -10,13 +10,13 @@ package testdata import "sync" func OkFunc(*sync.Mutex) {} -func BadFunc(sync.Mutex) {} // ERROR "BadFunc passes Lock by value: sync.Mutex" +func BadFunc(sync.Mutex) {} // ERROR "BadFunc passes lock by value: sync.Mutex" func OkRet() *sync.Mutex {} -func BadRet() sync.Mutex {} // ERROR "BadRet returns Lock by value: sync.Mutex" +func BadRet() sync.Mutex {} // ERROR "BadRet returns lock by value: sync.Mutex" var ( OkClosure = func(*sync.Mutex) {} - BadClosure = func(sync.Mutex) {} // ERROR "func passes Lock by value: sync.Mutex" + BadClosure = func(sync.Mutex) {} // ERROR "func passes lock by value: sync.Mutex" ) type EmbeddedRWMutex struct { @@ -24,20 +24,20 @@ type EmbeddedRWMutex struct { } func (*EmbeddedRWMutex) OkMeth() {} -func (EmbeddedRWMutex) BadMeth() {} // ERROR "BadMeth passes Lock by value: testdata.EmbeddedRWMutex" +func (EmbeddedRWMutex) BadMeth() {} // ERROR "BadMeth passes lock by value: testdata.EmbeddedRWMutex" func OkFunc(e *EmbeddedRWMutex) {} -func BadFunc(EmbeddedRWMutex) {} // ERROR "BadFunc passes Lock by value: testdata.EmbeddedRWMutex" +func BadFunc(EmbeddedRWMutex) {} // ERROR "BadFunc passes lock by value: testdata.EmbeddedRWMutex" func OkRet() *EmbeddedRWMutex {} -func BadRet() EmbeddedRWMutex {} // ERROR "BadRet returns Lock by value: testdata.EmbeddedRWMutex" +func BadRet() EmbeddedRWMutex {} // ERROR "BadRet returns lock by value: testdata.EmbeddedRWMutex" type FieldMutex struct { s sync.Mutex } func (*FieldMutex) OkMeth() {} -func (FieldMutex) BadMeth() {} // ERROR "BadMeth passes Lock by value: testdata.FieldMutex contains sync.Mutex" +func (FieldMutex) BadMeth() {} // ERROR "BadMeth passes lock by value: testdata.FieldMutex contains sync.Mutex" func OkFunc(*FieldMutex) {} -func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes Lock by value: testdata.FieldMutex contains sync.Mutex" +func BadFunc(FieldMutex, int) {} // ERROR "BadFunc passes lock by value: testdata.FieldMutex contains sync.Mutex" type L0 struct { L1 @@ -52,7 +52,7 @@ type L2 struct { } func (*L0) Ok() {} -func (L0) Bad() {} // ERROR "Bad passes Lock by value: testdata.L0 contains testdata.L1 contains testdata.L2" +func (L0) Bad() {} // ERROR "Bad passes lock by value: testdata.L0 contains testdata.L1 contains testdata.L2" type EmbeddedMutexPointer struct { s *sync.Mutex // safe to copy this pointer @@ -76,7 +76,7 @@ func (*CustomLock) Lock() {} func (*CustomLock) Unlock() {} func Ok(*CustomLock) {} -func Bad(CustomLock) {} // ERROR "Bad passes Lock by value: testdata.CustomLock" +func Bad(CustomLock) {} // ERROR "Bad passes lock by value: testdata.CustomLock" // TODO: Unfortunate cases @@ -85,7 +85,7 @@ func Bad(CustomLock) {} // ERROR "Bad passes Lock by value: testdata.CustomLock" // sync.Mutex gets called out, but without any reference to the sync.Once. type LocalOnce sync.Once -func (LocalOnce) Bad() {} // ERROR "Bad passes Lock by value: testdata.LocalOnce contains sync.Mutex" +func (LocalOnce) Bad() {} // ERROR "Bad passes lock by value: testdata.LocalOnce contains sync.Mutex" // False negative: // LocalMutex doesn't have a Lock method. diff --git a/src/cmd/vet/testdata/copylock_range.go b/src/cmd/vet/testdata/copylock_range.go index f95b0252b6..f127381213 100644 --- a/src/cmd/vet/testdata/copylock_range.go +++ b/src/cmd/vet/testdata/copylock_range.go @@ -24,37 +24,37 @@ func rangeMutex() { } for i, _ := range s { } - for _, mu = range s { // ERROR "range var mu copies Lock: sync.Mutex" + for _, mu = range s { // ERROR "range var mu copies lock: sync.Mutex" } - for _, m := range s { // ERROR "range var m copies Lock: sync.Mutex" + for _, m := range s { // ERROR "range var m copies lock: sync.Mutex" } - for i, mu = range s { // ERROR "range var mu copies Lock: sync.Mutex" + for i, mu = range s { // ERROR "range var mu copies lock: sync.Mutex" } - for i, m := range s { // ERROR "range var m copies Lock: sync.Mutex" + for i, m := range s { // ERROR "range var m copies lock: sync.Mutex" } var a [3]sync.Mutex - for _, m := range a { // ERROR "range var m copies Lock: sync.Mutex" + for _, m := range a { // ERROR "range var m copies lock: sync.Mutex" } var m map[sync.Mutex]sync.Mutex - for k := range m { // ERROR "range var k copies Lock: sync.Mutex" + for k := range m { // ERROR "range var k copies lock: sync.Mutex" } - for mu, _ = range m { // ERROR "range var mu copies Lock: sync.Mutex" + for mu, _ = range m { // ERROR "range var mu copies lock: sync.Mutex" } - for k, _ := range m { // ERROR "range var k copies Lock: sync.Mutex" + for k, _ := range m { // ERROR "range var k copies lock: sync.Mutex" } - for _, mu = range m { // ERROR "range var mu copies Lock: sync.Mutex" + for _, mu = range m { // ERROR "range var mu copies lock: sync.Mutex" } - for _, v := range m { // ERROR "range var v copies Lock: sync.Mutex" + for _, v := range m { // ERROR "range var v copies lock: sync.Mutex" } var c chan sync.Mutex for range c { } - for mu = range c { // ERROR "range var mu copies Lock: sync.Mutex" + for mu = range c { // ERROR "range var mu copies lock: sync.Mutex" } - for v := range c { // ERROR "range var v copies Lock: sync.Mutex" + for v := range c { // ERROR "range var v copies lock: sync.Mutex" } // Test non-idents in range variables @@ -62,6 +62,6 @@ func rangeMutex() { i int mu sync.Mutex } - for t.i, t.mu = range s { // ERROR "range var t.mu copies Lock: sync.Mutex" + for t.i, t.mu = range s { // ERROR "range var t.mu copies lock: sync.Mutex" } }