cmd/vet: diagnose use of unsafe.Pointer to convert integer to pointer

LGTM=r
R=r
CC=golang-codereviews
https://golang.org/cl/100470044

cmd/vet/doc.go

@@ -133,6 +133,11 @@ Flag -shadow=false (experimental; must be set explicitly)
 
 Variables that may have been unintentionally shadowed.
 
+14. Misuse of unsafe.Pointer
+
+Flag -unsafeptr
+
+Likely incorrect uses of unsafe.Pointer to convert integers to pointers.
 
 Other flags
 

cmd/vet/main.go

@@ -51,6 +51,7 @@ var report = map[string]*triState{
 	"shadow":      triStateFlag("shadow", unset, "check for shadowed variables (experimental; must be set explicitly)"),
 	"structtags":  triStateFlag("structtags", unset, "check that struct field tags have canonical format"),
 	"unreachable": triStateFlag("unreachable", unset, "check for unreachable code"),
+	"unsafeptr":   triStateFlag("unsafeptr", unset, "check for misuse of unsafe.Pointer"),
 }
 
 // experimental records the flags enabling experimental features. These must be
@@ -478,6 +479,7 @@ func (f *File) walkCallExpr(call *ast.CallExpr) {
 	case *ast.SelectorExpr:
 		f.walkCall(call, x.Sel.Name)
 	}
+	f.checkUnsafePointer(call)
 }
 
 // walkCompositeLit walks a composite literal.

cmd/vet/testdata/unsafeptr.go

@@ -0,0 +1,61 @@
+// Copyright 2014 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 testdata
+
+import (
+	"reflect"
+	"unsafe"
+)
+
+func f() {
+	var x unsafe.Pointer
+	var y uintptr
+	x = unsafe.Pointer(y) // ERROR "possible misuse of unsafe.Pointer"
+	y = uintptr(x)
+
+	// only allowed pointer arithmetic is ptr +/- num.
+	// num+ptr is technically okay but still flagged: write ptr+num instead.
+	x = unsafe.Pointer(uintptr(x) + 1)
+	x = unsafe.Pointer(1 + uintptr(x))          // ERROR "possible misuse of unsafe.Pointer"
+	x = unsafe.Pointer(uintptr(x) + uintptr(x)) // ERROR "possible misuse of unsafe.Pointer"
+	x = unsafe.Pointer(uintptr(x) - 1)
+	x = unsafe.Pointer(1 - uintptr(x)) // ERROR "possible misuse of unsafe.Pointer"
+
+	// certain uses of reflect are okay
+	var v reflect.Value
+	x = unsafe.Pointer(v.Pointer())
+	x = unsafe.Pointer(v.UnsafeAddr())
+	var s1 *reflect.StringHeader
+	x = unsafe.Pointer(s1.Data)
+	var s2 *reflect.SliceHeader
+	x = unsafe.Pointer(s2.Data)
+	var s3 reflect.StringHeader
+	x = unsafe.Pointer(s3.Data) // ERROR "possible misuse of unsafe.Pointer"
+	var s4 reflect.SliceHeader
+	x = unsafe.Pointer(s4.Data) // ERROR "possible misuse of unsafe.Pointer"
+
+	// but only in reflect
+	var vv V
+	x = unsafe.Pointer(vv.Pointer())    // ERROR "possible misuse of unsafe.Pointer"
+	x = unsafe.Pointer(vv.UnsafeAddr()) // ERROR "possible misuse of unsafe.Pointer"
+	var ss1 *StringHeader
+	x = unsafe.Pointer(ss1.Data) // ERROR "possible misuse of unsafe.Pointer"
+	var ss2 *SliceHeader
+	x = unsafe.Pointer(ss2.Data) // ERROR "possible misuse of unsafe.Pointer"
+
+}
+
+type V interface {
+	Pointer() uintptr
+	UnsafeAddr() uintptr
+}
+
+type StringHeader struct {
+	Data uintptr
+}
+
+type SliceHeader struct {
+	Data uintptr
+}

cmd/vet/types.go

@@ -237,6 +237,16 @@ func (f *File) matchArgTypeInternal(t printfArgType, typ types.Type, arg ast.Exp
 	return false
 }
 
+// hasBasicType reports whether x's type is a types.Basic with the given kind.
+func (f *File) hasBasicType(x ast.Expr, kind types.BasicKind) bool {
+	t := f.pkg.types[x].Type
+	if t != nil {
+		t = t.Underlying()
+	}
+	b, ok := t.(*types.Basic)
+	return ok && b.Kind() == kind
+}
+
 // matchStructArgType reports whether all the elements of the struct match the expected
 // type. For instance, with "%d" all the elements must be printable with the "%d" format.
 func (f *File) matchStructArgType(t printfArgType, typ *types.Struct, arg ast.Expr, inProgress map[types.Type]bool) bool {

cmd/vet/unsafeptr.go

@@ -0,0 +1,104 @@
+// Copyright 2014 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 uintptr -> unsafe.Pointer conversions.
+//
+// A conversion from uintptr to unsafe.Pointer is invalid if it implies that
+// there is a uintptr-typed word in memory that holds a pointer value,
+// because that word will be invisible to stack copying and to the garbage
+// collector.
+//
+// Allow pointer arithmetic: unsafe.Pointer(uintptr(p) + delta).
+// Allow use of reflect:
+//	unsafe.Pointer(reflect.ValueOf(v).Pointer())
+// 	unsafe.Pointer(reflect.ValueOf(v).UnsafeAddr()).
+// 	unsafe.Pointer(h.Data) for var h *reflect.SliceHeader
+//	unsafe.Pointer(h.Data) for var h *reflect.StringHeader
+package main
+
+import (
+	"go/ast"
+	"go/token"
+
+	"code.google.com/p/go.tools/go/types"
+)
+
+func (f *File) checkUnsafePointer(x *ast.CallExpr) {
+	if !vet("unsafeptr") {
+		return
+	}
+	if len(x.Args) != 1 {
+		return
+	}
+	if f.hasBasicType(x.Fun, types.UnsafePointer) && f.hasBasicType(x.Args[0], types.Uintptr) && !f.isSafeUintptr(x.Args[0]) {
+		f.Badf(x.Pos(), "possible misuse of unsafe.Pointer")
+	}
+}
+
+// isSafeUintptr reports whether x - already known to be a uintptr -
+// is safe to convert to unsafe.Pointer. It is safe if x is itself derived
+// directly from an unsafe.Pointer via conversion and pointer arithmetic
+// or if x is the result of reflect.Value.Pointer or reflect.Value.UnsafeAddr
+// or obtained from the Data field of a *reflect.SliceHeader or *reflect.StringHeader.
+func (f *File) isSafeUintptr(x ast.Expr) bool {
+	switch x := x.(type) {
+	case *ast.ParenExpr:
+		return f.isSafeUintptr(x.X)
+
+	case *ast.SelectorExpr:
+		switch x.Sel.Name {
+		case "Data":
+			// reflect.SliceHeader and reflect.StringHeader are okay,
+			// but only if they are pointing at a real slice or string.
+			// It's not okay to do:
+			//	var x SliceHeader
+			//	x.Data = uintptr(unsafe.Pointer(...))
+			//	... use x ...
+			//	p := unsafe.Pointer(x.Data)
+			// because in the middle the garbage collector doesn't
+			// see x.Data as a pointer and so x.Data may be dangling
+			// by the time we get to the conversion at the end.
+			// For now approximate by saying that *Header is okay
+			// but Header is not.
+			pt, ok := f.pkg.types[x.X].Type.(*types.Pointer)
+			if ok {
+				t, ok := pt.Elem().(*types.Named)
+				if ok && t.Obj().Pkg().Path() == "reflect" {
+					switch t.Obj().Name() {
+					case "StringHeader", "SliceHeader":
+						return true
+					}
+				}
+			}
+		}
+
+	case *ast.CallExpr:
+		switch len(x.Args) {
+		case 0:
+			// maybe call to reflect.Value.Pointer or reflect.Value.UnsafeAddr.
+			sel, ok := x.Fun.(*ast.SelectorExpr)
+			if !ok {
+				break
+			}
+			switch sel.Sel.Name {
+			case "Pointer", "UnsafeAddr":
+				t, ok := f.pkg.types[sel.X].Type.(*types.Named)
+				if ok && t.Obj().Pkg().Path() == "reflect" && t.Obj().Name() == "Value" {
+					return true
+				}
+			}
+
+		case 1:
+			// maybe conversion of uintptr to unsafe.Pointer
+			return f.hasBasicType(x.Fun, types.Uintptr) && f.hasBasicType(x.Args[0], types.UnsafePointer)
+		}
+
+	case *ast.BinaryExpr:
+		switch x.Op {
+		case token.ADD, token.SUB:
+			return f.isSafeUintptr(x.X) && !f.isSafeUintptr(x.Y)
+		}
+	}
+	return false
+}