diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 83e1a6af..25062c68 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -184,7 +184,6 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis // Extract 'want' comments from Go files. for _, f := range pass.Files { - filename := sanitize(gopath, pass.Fset.File(f.Pos()).Name()) for _, cgroup := range f.Comments { for _, c := range cgroup.List { @@ -201,13 +200,19 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis text = text[i+len("// "):] } - linenum := pass.Fset.Position(c.Pos()).Line - processComment(filename, linenum, text) + // It's tempting to compute the filename + // once outside the loop, but it's + // incorrect because it can change due + // to //line directives. + posn := pass.Fset.Position(c.Pos()) + filename := sanitize(gopath, posn.Filename) + processComment(filename, posn.Line, text) } } } // Extract 'want' comments from non-Go files. + // TODO(adonovan): we may need to handle //line directives. for _, filename := range pass.OtherFiles { data, err := ioutil.ReadFile(filename) if err != nil { @@ -285,6 +290,12 @@ func check(t Testing, gopath string, pass *analysis.Pass, diagnostics []analysis } // Reject surplus expectations. + // + // Sometimes an Analyzer reports two similar diagnostics on a + // line with only one expectation. The reader may be confused by + // the error message. + // TODO(adonovan): print a better error: + // "got 2 diagnostics here; each one needs its own expectation". var surplus []string for key, expects := range want { for _, exp := range expects { diff --git a/go/analysis/passes/cgocall/cgocall.go b/go/analysis/passes/cgocall/cgocall.go new file mode 100644 index 00000000..daf65b97 --- /dev/null +++ b/go/analysis/passes/cgocall/cgocall.go @@ -0,0 +1,222 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package cgocall + +import ( + "fmt" + "go/ast" + "go/token" + "go/types" + "log" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "cgocall", + Doc: `detect some violations of the cgo pointer passing rules + +Check for invalid cgo pointer passing. +This looks for code that uses cgo to call C code passing values +whose types are almost always invalid according to the cgo pointer +sharing rules. +Specifically, it warns about attempts to pass a Go chan, map, func, +or slice to C, either directly, or via a pointer, array, or struct.`, + Requires: []*analysis.Analyzer{inspect.Analyzer}, + RunDespiteErrors: true, + Run: run, +} + +func run(pass *analysis.Pass) (interface{}, error) { + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return true + } + call, name := findCall(pass.Fset, stack) + if call == nil { + return true // not a call we need to check + } + + // A call to C.CBytes passes a pointer but is always safe. + if name == "CBytes" { + return true + } + + if false { + fmt.Printf("%s: inner call to C.%s\n", pass.Fset.Position(n.Pos()), name) + fmt.Printf("%s: outer call to C.%s\n", pass.Fset.Position(call.Lparen), name) + } + + for _, arg := range call.Args { + if !typeOKForCgoCall(cgoBaseType(pass.TypesInfo, arg), make(map[types.Type]bool)) { + pass.Reportf(arg.Pos(), "possibly passing Go type with embedded pointer to C") + break + } + + // Check for passing the address of a bad type. + if conv, ok := arg.(*ast.CallExpr); ok && len(conv.Args) == 1 && + isUnsafePointer(pass.TypesInfo, conv.Fun) { + arg = conv.Args[0] + } + if u, ok := arg.(*ast.UnaryExpr); ok && u.Op == token.AND { + if !typeOKForCgoCall(cgoBaseType(pass.TypesInfo, u.X), make(map[types.Type]bool)) { + pass.Reportf(arg.Pos(), "possibly passing Go type with embedded pointer to C") + break + } + } + } + return true + }) + return nil, nil +} + +// findCall returns the CallExpr that we need to check, which may not be +// the same as the one we're currently visiting, due to code generation. +// It also returns the name of the function, such as "f" for C.f(...). +// +// This checker was initially written in vet to inpect unprocessed cgo +// source files using partial type information. However, Analyzers in +// the new analysis API are presented with the type-checked, processed +// Go ASTs resulting from cgo processing files, so we must choose +// between: +// +// a) locating the cgo file (e.g. from //line directives) +// and working with that, or +// b) working with the file generated by cgo. +// +// We cannot use (a) because it does not provide type information, which +// the analyzer needs, and it is infeasible for the analyzer to run the +// type checker on this file. Thus we choose (b), which is fragile, +// because the checker may need to change each time the cgo processor +// changes. +// +// Consider a cgo source file containing this header: +// +// /* void f(void *x, *y); */ +// import "C" +// +// The cgo tool expands a call such as: +// +// C.f(x, y) +// +// to this: +// +// 1 func(param0, param1 unsafe.Pointer) { +// 2 ... various checks on params ... +// 3 (_Cfunc_f)(param0, param1) +// 4 }(x, y) +// +// We first locate the _Cfunc_f call on line 3, then +// walk up the stack of enclosing nodes until we find +// the call on line 4. +// +func findCall(fset *token.FileSet, stack []ast.Node) (*ast.CallExpr, string) { + last := len(stack) - 1 + call := stack[last].(*ast.CallExpr) + if id, ok := analysisutil.Unparen(call.Fun).(*ast.Ident); ok { + if name := strings.TrimPrefix(id.Name, "_Cfunc_"); name != id.Name { + // Find the outer call with the arguments (x, y) we want to check. + for i := last - 1; i >= 0; i-- { + if outer, ok := stack[i].(*ast.CallExpr); ok { + return outer, name + } + } + // This shouldn't happen. + // Perhaps the code generator has changed? + log.Printf("%s: can't find outer call for C.%s(...)", + fset.Position(call.Lparen), name) + } + } + return nil, "" +} + +// cgoBaseType tries to look through type conversions involving +// unsafe.Pointer to find the real type. It converts: +// unsafe.Pointer(x) => x +// *(*unsafe.Pointer)(unsafe.Pointer(&x)) => x +func cgoBaseType(info *types.Info, arg ast.Expr) types.Type { + switch arg := arg.(type) { + case *ast.CallExpr: + if len(arg.Args) == 1 && isUnsafePointer(info, arg.Fun) { + return cgoBaseType(info, arg.Args[0]) + } + case *ast.StarExpr: + call, ok := arg.X.(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + break + } + // Here arg is *f(v). + t := info.Types[call.Fun].Type + if t == nil { + break + } + ptr, ok := t.Underlying().(*types.Pointer) + if !ok { + break + } + // Here arg is *(*p)(v) + elem, ok := ptr.Elem().Underlying().(*types.Basic) + if !ok || elem.Kind() != types.UnsafePointer { + break + } + // Here arg is *(*unsafe.Pointer)(v) + call, ok = call.Args[0].(*ast.CallExpr) + if !ok || len(call.Args) != 1 { + break + } + // Here arg is *(*unsafe.Pointer)(f(v)) + if !isUnsafePointer(info, call.Fun) { + break + } + // Here arg is *(*unsafe.Pointer)(unsafe.Pointer(v)) + u, ok := call.Args[0].(*ast.UnaryExpr) + if !ok || u.Op != token.AND { + break + } + // Here arg is *(*unsafe.Pointer)(unsafe.Pointer(&v)) + return cgoBaseType(info, u.X) + } + + return info.Types[arg].Type +} + +// typeOKForCgoCall reports whether the type of arg is OK to pass to a +// C function using cgo. This is not true for Go types with embedded +// pointers. m is used to avoid infinite recursion on recursive types. +func typeOKForCgoCall(t types.Type, m map[types.Type]bool) bool { + if t == nil || m[t] { + return true + } + m[t] = true + switch t := t.Underlying().(type) { + case *types.Chan, *types.Map, *types.Signature, *types.Slice: + return false + case *types.Pointer: + return typeOKForCgoCall(t.Elem(), m) + case *types.Array: + return typeOKForCgoCall(t.Elem(), m) + case *types.Struct: + for i := 0; i < t.NumFields(); i++ { + if !typeOKForCgoCall(t.Field(i).Type(), m) { + return false + } + } + } + return true +} + +func isUnsafePointer(info *types.Info, e ast.Expr) bool { + t := info.Types[e].Type + return t != nil && t.Underlying() == types.Typ[types.UnsafePointer] +} diff --git a/go/analysis/passes/cgocall/cgocall_test.go b/go/analysis/passes/cgocall/cgocall_test.go new file mode 100644 index 00000000..db228187 --- /dev/null +++ b/go/analysis/passes/cgocall/cgocall_test.go @@ -0,0 +1,13 @@ +package cgocall_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/cgocall" +) + +func TestFromFileSystem(t *testing.T) { + testdata := analysistest.TestData() + analysistest.Run(t, testdata, cgocall.Analyzer, "a", "b") +} diff --git a/go/analysis/passes/cgocall/testdata/src/a/cgo.go b/go/analysis/passes/cgocall/testdata/src/a/cgo.go new file mode 100644 index 00000000..39ad897e --- /dev/null +++ b/go/analysis/passes/cgocall/testdata/src/a/cgo.go @@ -0,0 +1,59 @@ +// Copyright 2015 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains tests for the cgo checker. + +package a + +// void f(void *ptr) {} +import "C" + +import "unsafe" + +func CgoTests() { + var c chan bool + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&c))) // want "embedded pointer" + C.f(unsafe.Pointer(&c)) // want "embedded pointer" + + var m map[string]string + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&m))) // want "embedded pointer" + C.f(unsafe.Pointer(&m)) // want "embedded pointer" + + var f func() + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&f))) // want "embedded pointer" + C.f(unsafe.Pointer(&f)) // want "embedded pointer" + + var s []int + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&s))) // want "embedded pointer" + C.f(unsafe.Pointer(&s)) // want "embedded pointer" + + var a [1][]int + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&a))) // want "embedded pointer" + C.f(unsafe.Pointer(&a)) // want "embedded pointer" + + var st struct{ f []int } + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&st))) // want "embedded pointer" + C.f(unsafe.Pointer(&st)) // want "embedded pointer" + + // The following cases are OK. + var i int + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&i))) + C.f(unsafe.Pointer(&i)) + + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&s[0]))) + C.f(unsafe.Pointer(&s[0])) + + var a2 [1]int + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&a2))) + C.f(unsafe.Pointer(&a2)) + + var st2 struct{ i int } + C.f(*(*unsafe.Pointer)(unsafe.Pointer(&st2))) + C.f(unsafe.Pointer(&st2)) + + type cgoStruct struct{ p *cgoStruct } + C.f(unsafe.Pointer(&cgoStruct{})) + + C.CBytes([]byte("hello")) +} diff --git a/go/analysis/passes/vet/testdata/cgo/cgo2.go b/go/analysis/passes/cgocall/testdata/src/a/cgo2.go similarity index 71% rename from go/analysis/passes/vet/testdata/cgo/cgo2.go rename to go/analysis/passes/cgocall/testdata/src/a/cgo2.go index 4f271168..a624afb9 100644 --- a/go/analysis/passes/vet/testdata/cgo/cgo2.go +++ b/go/analysis/passes/cgocall/testdata/src/a/cgo2.go @@ -4,9 +4,11 @@ // Test the cgo checker on a file that doesn't use cgo. -package testdata +package a -var _ = C.f(*p(**p)) +import "unsafe" // Passing a pointer (via the slice), but C isn't cgo. -var _ = C.f([]int{3}) +var _ = C.f(unsafe.Pointer(new([]int))) + +var C struct{ f func(interface{}) int } diff --git a/go/analysis/passes/vet/testdata/cgo/cgo3.go b/go/analysis/passes/cgocall/testdata/src/a/cgo3.go similarity index 63% rename from go/analysis/passes/vet/testdata/cgo/cgo3.go rename to go/analysis/passes/cgocall/testdata/src/a/cgo3.go index 0b1518e1..fd095166 100644 --- a/go/analysis/passes/vet/testdata/cgo/cgo3.go +++ b/go/analysis/passes/cgocall/testdata/src/a/cgo3.go @@ -2,10 +2,7 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Used by TestVetVerbose to test that vet -v doesn't fail because it -// can't find "C". - -package testdata +package a import "C" diff --git a/go/analysis/passes/vet/testdata/cgo/cgo4.go b/go/analysis/passes/cgocall/testdata/src/a/cgo4.go similarity index 95% rename from go/analysis/passes/vet/testdata/cgo/cgo4.go rename to go/analysis/passes/cgocall/testdata/src/a/cgo4.go index 67b54506..fc1d1e5f 100644 --- a/go/analysis/passes/vet/testdata/cgo/cgo4.go +++ b/go/analysis/passes/cgocall/testdata/src/a/cgo4.go @@ -5,7 +5,7 @@ // Test the cgo checker on a file that doesn't use cgo, but has an // import named "C". -package testdata +package a import C "fmt" diff --git a/go/analysis/passes/cgocall/testdata/src/b/b.go b/go/analysis/passes/cgocall/testdata/src/b/b.go new file mode 100644 index 00000000..342b14be --- /dev/null +++ b/go/analysis/passes/cgocall/testdata/src/b/b.go @@ -0,0 +1,15 @@ +// Copyright 2017 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Test the cgo checker on a file that doesn't use cgo, but has an +// import named "C". + +package b + +import C "fmt" + +var _ = C.Println(*p(**p)) + +// Passing a pointer (via a slice), but C is fmt, not cgo. +var _ = C.Println([]int{3}) diff --git a/go/analysis/passes/vet/cgo.go b/go/analysis/passes/vet/cgo.go deleted file mode 100644 index 693dac96..00000000 --- a/go/analysis/passes/vet/cgo.go +++ /dev/null @@ -1,143 +0,0 @@ -// +build ignore - -// Copyright 2015 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// Check for invalid cgo pointer passing. -// This looks for code that uses cgo to call C code passing values -// whose types are almost always invalid according to the cgo pointer -// sharing rules. -// Specifically, it warns about attempts to pass a Go chan, map, func, -// or slice to C, either directly, or via a pointer, array, or struct. - -package main - -import ( - "go/ast" - "go/token" - "go/types" -) - -func init() { - register("cgocall", - "check for types that may not be passed to cgo calls", - checkCgoCall, - callExpr) -} - -func checkCgoCall(f *File, node ast.Node) { - x := node.(*ast.CallExpr) - - // We are only looking for calls to functions imported from - // the "C" package. - sel, ok := x.Fun.(*ast.SelectorExpr) - if !ok { - return - } - id, ok := sel.X.(*ast.Ident) - if !ok { - return - } - - pkgname, ok := f.pkg.uses[id].(*types.PkgName) - if !ok || pkgname.Imported().Path() != "C" { - return - } - - // A call to C.CBytes passes a pointer but is always safe. - if sel.Sel.Name == "CBytes" { - return - } - - for _, arg := range x.Args { - if !typeOKForCgoCall(cgoBaseType(f, arg), make(map[types.Type]bool)) { - f.Badf(arg.Pos(), "possibly passing Go type with embedded pointer to C") - } - - // Check for passing the address of a bad type. - if conv, ok := arg.(*ast.CallExpr); ok && len(conv.Args) == 1 && f.hasBasicType(conv.Fun, types.UnsafePointer) { - arg = conv.Args[0] - } - if u, ok := arg.(*ast.UnaryExpr); ok && u.Op == token.AND { - if !typeOKForCgoCall(cgoBaseType(f, u.X), make(map[types.Type]bool)) { - f.Badf(arg.Pos(), "possibly passing Go type with embedded pointer to C") - } - } - } -} - -// cgoBaseType tries to look through type conversions involving -// unsafe.Pointer to find the real type. It converts: -// unsafe.Pointer(x) => x -// *(*unsafe.Pointer)(unsafe.Pointer(&x)) => x -func cgoBaseType(f *File, arg ast.Expr) types.Type { - switch arg := arg.(type) { - case *ast.CallExpr: - if len(arg.Args) == 1 && f.hasBasicType(arg.Fun, types.UnsafePointer) { - return cgoBaseType(f, arg.Args[0]) - } - case *ast.StarExpr: - call, ok := arg.X.(*ast.CallExpr) - if !ok || len(call.Args) != 1 { - break - } - // Here arg is *f(v). - t := f.pkg.types[call.Fun].Type - if t == nil { - break - } - ptr, ok := t.Underlying().(*types.Pointer) - if !ok { - break - } - // Here arg is *(*p)(v) - elem, ok := ptr.Elem().Underlying().(*types.Basic) - if !ok || elem.Kind() != types.UnsafePointer { - break - } - // Here arg is *(*unsafe.Pointer)(v) - call, ok = call.Args[0].(*ast.CallExpr) - if !ok || len(call.Args) != 1 { - break - } - // Here arg is *(*unsafe.Pointer)(f(v)) - if !f.hasBasicType(call.Fun, types.UnsafePointer) { - break - } - // Here arg is *(*unsafe.Pointer)(unsafe.Pointer(v)) - u, ok := call.Args[0].(*ast.UnaryExpr) - if !ok || u.Op != token.AND { - break - } - // Here arg is *(*unsafe.Pointer)(unsafe.Pointer(&v)) - return cgoBaseType(f, u.X) - } - - return f.pkg.types[arg].Type -} - -// typeOKForCgoCall reports whether the type of arg is OK to pass to a -// C function using cgo. This is not true for Go types with embedded -// pointers. m is used to avoid infinite recursion on recursive types. -func typeOKForCgoCall(t types.Type, m map[types.Type]bool) bool { - if t == nil || m[t] { - return true - } - m[t] = true - switch t := t.Underlying().(type) { - case *types.Chan, *types.Map, *types.Signature, *types.Slice: - return false - case *types.Pointer: - return typeOKForCgoCall(t.Elem(), m) - case *types.Array: - return typeOKForCgoCall(t.Elem(), m) - case *types.Struct: - for i := 0; i < t.NumFields(); i++ { - if !typeOKForCgoCall(t.Field(i).Type(), m) { - return false - } - } - } - return true -} diff --git a/go/analysis/passes/vet/testdata/cgo/cgo.go b/go/analysis/passes/vet/testdata/cgo/cgo.go deleted file mode 100644 index d0df7cf6..00000000 --- a/go/analysis/passes/vet/testdata/cgo/cgo.go +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright 2015 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -// This file contains tests for the cgo checker. - -package testdata - -// void f(void *); -import "C" - -import "unsafe" - -func CgoTests() { - var c chan bool - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&c))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&c)) // ERROR "embedded pointer" - - var m map[string]string - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&m))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&m)) // ERROR "embedded pointer" - - var f func() - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&f))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&f)) // ERROR "embedded pointer" - - var s []int - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&s))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&s)) // ERROR "embedded pointer" - - var a [1][]int - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&a))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&a)) // ERROR "embedded pointer" - - var st struct{ f []int } - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&st))) // ERROR "embedded pointer" - C.f(unsafe.Pointer(&st)) // ERROR "embedded pointer" - - // The following cases are OK. - var i int - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&i))) - C.f(unsafe.Pointer(&i)) - - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&s[0]))) - C.f(unsafe.Pointer(&s[0])) - - var a2 [1]int - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&a2))) - C.f(unsafe.Pointer(&a2)) - - var st2 struct{ i int } - C.f(*(*unsafe.Pointer)(unsafe.Pointer(&st2))) - C.f(unsafe.Pointer(&st2)) - - type cgoStruct struct{ p *cgoStruct } - C.f(unsafe.Pointer(&cgoStruct{})) - - C.CBytes([]byte("hello")) -}