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>

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
+				// It's tempting to compute the filename
-				processComment(filename, linenum, text)
+				// 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 {

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]
+}

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")
+}

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"))
+}

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 }

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
+package a
-// can't find "C".
-
-package testdata
 
 import "C"
 

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"
 

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})

go/analysis/passes/vet/cgo.go

@@ -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
-}

go/analysis/passes/vet/testdata/cgo/cgo.go

@@ -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"))
-}