From b752e9ffdf67539a73dc836ce2ef786501f2217c Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 15 May 2014 15:32:51 -0400 Subject: [PATCH] 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 | 5 ++ cmd/vet/main.go | 2 + cmd/vet/testdata/unsafeptr.go | 61 ++++++++++++++++++++ cmd/vet/types.go | 10 ++++ cmd/vet/unsafeptr.go | 104 ++++++++++++++++++++++++++++++++++ 5 files changed, 182 insertions(+) create mode 100644 cmd/vet/testdata/unsafeptr.go create mode 100644 cmd/vet/unsafeptr.go diff --git a/cmd/vet/doc.go b/cmd/vet/doc.go index 72832da9..47e4eeea 100644 --- a/cmd/vet/doc.go +++ b/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 diff --git a/cmd/vet/main.go b/cmd/vet/main.go index b915da32..07a26ba9 100644 --- a/cmd/vet/main.go +++ b/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. diff --git a/cmd/vet/testdata/unsafeptr.go b/cmd/vet/testdata/unsafeptr.go new file mode 100644 index 00000000..8f64030b --- /dev/null +++ b/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 +} diff --git a/cmd/vet/types.go b/cmd/vet/types.go index 6afc6ab9..05af1037 100644 --- a/cmd/vet/types.go +++ b/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 { diff --git a/cmd/vet/unsafeptr.go b/cmd/vet/unsafeptr.go new file mode 100644 index 00000000..b64c98c5 --- /dev/null +++ b/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 +}