go/analysis/passes/cgocall: split out of vet
This checker needed some reworking because whereas vet sees
unprocessed cgo source files (with partial type information), the
analysis API sees cgo-processed files with complete type information.
However, that means the checker must effectively undo some of the
transformations done by cgo, making it more fragile during changes to
cgo.
Change-Id: I3a243260f59b16e2e546e8f3e4585b93d3731192
Reviewed-on: https://go-review.googlesource.com/c/141157
Reviewed-by: Michael Matloob <matloob@golang.org>
diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go
index 83e1a6a..25062c6 100644
--- a/go/analysis/analysistest/analysistest.go
+++ b/go/analysis/analysistest/analysistest.go
@@ -184,7 +184,6 @@
// 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 @@
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 @@
}
// 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 0000000..daf65b9
--- /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 0000000..db22818
--- /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 0000000..39ad897
--- /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 4f27116..a624afb 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 0b1518e..fd09516 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 67b5450..fc1d1e5 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/vet/testdata/cgo/cgo4.go b/go/analysis/passes/cgocall/testdata/src/b/b.go
similarity index 95%
copy from go/analysis/passes/vet/testdata/cgo/cgo4.go
copy to go/analysis/passes/cgocall/testdata/src/b/b.go
index 67b5450..342b14b 100644
--- a/go/analysis/passes/vet/testdata/cgo/cgo4.go
+++ b/go/analysis/passes/cgocall/testdata/src/b/b.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 b
import C "fmt"
diff --git a/go/analysis/passes/vet/cgo.go b/go/analysis/passes/vet/cgo.go
deleted file mode 100644
index 693dac9..0000000
--- 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 d0df7cf..0000000
--- 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"))
-}
