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>
diff --git a/go/analysis/passes/unsafeptr/testdata/src/a/a.go b/go/analysis/passes/unsafeptr/testdata/src/a/a.go
index f1f6103..361dfb9 100644
--- a/go/analysis/passes/unsafeptr/testdata/src/a/a.go
+++ b/go/analysis/passes/unsafeptr/testdata/src/a/a.go
@@ -18,6 +18,7 @@
// 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 @@
// 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
diff --git a/go/analysis/passes/unsafeptr/unsafeptr.go b/go/analysis/passes/unsafeptr/unsafeptr.go
index d5bafb8..5594149 100644
--- a/go/analysis/passes/unsafeptr/unsafeptr.go
+++ b/go/analysis/passes/unsafeptr/unsafeptr.go
@@ -13,6 +13,7 @@
"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 @@
}
// 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 @@
// 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 @@
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
+}
