x/tools/go/analysis/passes/unsafeptr: stricter reporting

The unsafe.Pointer safety rules have three separate rules allowing
conversion of uintptr to unsafe.Pointer:

(3) allows converting unsafe.Pointer to uintptr, performing
arithmetic, and converting back;

(5) allows converting the results from calling
reflect.Value.{Pointer,UnsafeAddr} to unsafe.Pointer; and

(6) allows converting the Data field of reflect.{Slice,String}Header
to unsafe.Pointer.

Notably, these are three separate rules, and they're not allowed to be
arbitrarily mixed. For example, this is not allowed:

    unsafe.Pointer(v.UnsafeAddr() + x) // BAD

It needs to instead be written as:

    unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr())) + x)

(For further explanation on why this is necessary, see
https://github.com/google/go-cmp/issues/167#issuecomment-546093202.)

This CL brings cmd/vet's unsafeptr check inline with the
unsafe.Pointer rules and cmd/compile's implementation thereof.

Change-Id: I1844e0f71dcc8fb7aafacc144b86cc80a2b83b42
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248191
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>

go/analysis/passes/unsafeptr/testdata/src/a/a.go

@@ -18,6 +18,7 @@ func f() {
 	// 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(((uintptr((x))) + 1))
 	x = unsafe.Pointer(1 + uintptr(x))          // want "possible misuse of unsafe.Pointer"
 	x = unsafe.Pointer(uintptr(x) + uintptr(x)) // want "possible misuse of unsafe.Pointer"
 	x = unsafe.Pointer(uintptr(x) - 1)
@@ -28,9 +29,12 @@ func f() {
 	// certain uses of reflect are okay
 	var v reflect.Value
 	x = unsafe.Pointer(v.Pointer())
+	x = unsafe.Pointer(v.Pointer() + 1) // want "possible misuse of unsafe.Pointer"
 	x = unsafe.Pointer(v.UnsafeAddr())
+	x = unsafe.Pointer((v.UnsafeAddr()))
 	var s1 *reflect.StringHeader
 	x = unsafe.Pointer(s1.Data)
+	x = unsafe.Pointer(s1.Data + 1) // want "possible misuse of unsafe.Pointer"
 	var s2 *reflect.SliceHeader
 	x = unsafe.Pointer(s2.Data)
 	var s3 reflect.StringHeader

go/analysis/passes/unsafeptr/unsafeptr.go

@@ -13,6 +13,7 @@ import (
 
 	"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"
 )
 
@@ -52,16 +53,15 @@ func run(pass *analysis.Pass) (interface{}, error) {
 }
 
 // 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.
+// is safe to convert to unsafe.Pointer.
 func isSafeUintptr(info *types.Info, x ast.Expr) bool {
-	switch x := x.(type) {
-	case *ast.ParenExpr:
-		return isSafeUintptr(info, x.X)
+	// Check unsafe.Pointer safety rules according to
+	// https://golang.org/pkg/unsafe/#Pointer.
 
+	switch x := analysisutil.Unparen(x).(type) {
 	case *ast.SelectorExpr:
+		// "(6) Conversion of a reflect.SliceHeader or
+		// reflect.StringHeader Data field to or from Pointer."
 		if x.Sel.Name != "Data" {
 			break
 		}
@@ -78,44 +78,56 @@ func isSafeUintptr(info *types.Info, x ast.Expr) bool {
 		// For now approximate by saying that *Header is okay
 		// but Header is not.
 		pt, ok := info.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
-				}
-			}
+		if ok && isReflectHeader(pt.Elem()) {
+			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 := info.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 hasBasicType(info, x.Fun, types.Uintptr) &&
-				hasBasicType(info, x.Args[0], types.UnsafePointer)
+		// "(5) Conversion of the result of reflect.Value.Pointer or
+		// reflect.Value.UnsafeAddr from uintptr to Pointer."
+		if len(x.Args) != 0 {
+			break
 		}
-
-	case *ast.BinaryExpr:
-		switch x.Op {
-		case token.ADD, token.SUB, token.AND_NOT:
-			return isSafeUintptr(info, x.X) && !isSafeUintptr(info, x.Y)
+		sel, ok := x.Fun.(*ast.SelectorExpr)
+		if !ok {
+			break
+		}
+		switch sel.Sel.Name {
+		case "Pointer", "UnsafeAddr":
+			t, ok := info.Types[sel.X].Type.(*types.Named)
+			if ok && t.Obj().Pkg().Path() == "reflect" && t.Obj().Name() == "Value" {
+				return true
+			}
 		}
 	}
+
+	// "(3) Conversion of a Pointer to a uintptr and back, with arithmetic."
+	return isSafeArith(info, x)
+}
+
+// isSafeArith reports whether x is a pointer arithmetic expression that is safe
+// to convert to unsafe.Pointer.
+func isSafeArith(info *types.Info, x ast.Expr) bool {
+	switch x := analysisutil.Unparen(x).(type) {
+	case *ast.CallExpr:
+		// Base case: initial conversion from unsafe.Pointer to uintptr.
+		return len(x.Args) == 1 &&
+			hasBasicType(info, x.Fun, types.Uintptr) &&
+			hasBasicType(info, x.Args[0], types.UnsafePointer)
+
+	case *ast.BinaryExpr:
+		// "It is valid both to add and to subtract offsets from a
+		// pointer in this way. It is also valid to use &^ to round
+		// pointers, usually for alignment."
+		switch x.Op {
+		case token.ADD, token.SUB, token.AND_NOT:
+			// TODO(mdempsky): Match compiler
+			// semantics. ADD allows a pointer on either
+			// side; SUB and AND_NOT don't care about RHS.
+			return isSafeArith(info, x.X) && !isSafeArith(info, x.Y)
+		}
+	}
+
 	return false
 }
 
@@ -128,3 +140,16 @@ func hasBasicType(info *types.Info, x ast.Expr, kind types.BasicKind) bool {
 	b, ok := t.(*types.Basic)
 	return ok && b.Kind() == kind
 }
+
+// isReflectHeader reports whether t is reflect.SliceHeader or reflect.StringHeader.
+func isReflectHeader(t types.Type) bool {
+	if named, ok := t.(*types.Named); ok {
+		if obj := named.Obj(); obj.Pkg().Path() == "reflect" {
+			switch obj.Name() {
+			case "SliceHeader", "StringHeader":
+				return true
+			}
+		}
+	}
+	return false
+}