From 255fb2adf68116fd4918c3f0c735a4db2a5d08f5 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Wed, 12 Aug 2020 20:39:01 -0700 Subject: [PATCH] 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 gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Michael Matloob Trust: Matthew Dempsky --- .../passes/unsafeptr/testdata/src/a/a.go | 4 + go/analysis/passes/unsafeptr/unsafeptr.go | 103 +++++++++++------- 2 files changed, 68 insertions(+), 39 deletions(-) diff --git a/go/analysis/passes/unsafeptr/testdata/src/a/a.go b/go/analysis/passes/unsafeptr/testdata/src/a/a.go index f1f61035df..361dfb9365 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 @@ 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 diff --git a/go/analysis/passes/unsafeptr/unsafeptr.go b/go/analysis/passes/unsafeptr/unsafeptr.go index d5bafb859d..55941490fc 100644 --- a/go/analysis/passes/unsafeptr/unsafeptr.go +++ b/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 +}