x/tools: audit "unsafe"

This CL contains a number of small cleanups from auditing all
uses of the "unsafe" package (and the reflect.Value.Unsafe* methods)
in dependencies of gopls as potential causes of memory corruption.

None look actually problematic.

Change-Id: I443f29caf00c2d3734a31c5fa496e5296952930c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/725940
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

Author: adonovan

internal/event/core/export.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"sync/atomic"
 	"time"
-	"unsafe"
 
 	"golang.org/x/tools/internal/event/label"
 )
@@ -17,23 +16,21 @@ import (
 // It may return a modified context and event.
 type Exporter func(context.Context, Event, label.Map) context.Context
 
-var (
-	exporter unsafe.Pointer
-)
+var exporter atomic.Pointer[Exporter]
 
 // SetExporter sets the global exporter function that handles all events.
 // The exporter is called synchronously from the event call site, so it should
 // return quickly so as not to hold up user code.
 func SetExporter(e Exporter) {
-	p := unsafe.Pointer(&e)
 	if e == nil {
 		// &e is always valid, and so p is always valid, but for the early abort
 		// of ProcessEvent to be efficient it needs to make the nil check on the
 		// pointer without having to dereference it, so we make the nil function
 		// also a nil pointer
-		p = nil
+		exporter.Store(nil)
+	} else {
+		exporter.Store(&e)
 	}
-	atomic.StorePointer(&exporter, p)
 }
 
 // deliver is called to deliver an event to the supplied exporter.
@@ -48,7 +45,7 @@ func deliver(ctx context.Context, exporter Exporter, ev Event) context.Context {
 // Export is called to deliver an event to the global exporter if set.
 func Export(ctx context.Context, ev Event) context.Context {
 	// get the global exporter and abort early if there is not one
-	exporterPtr := (*Exporter)(atomic.LoadPointer(&exporter))
+	exporterPtr := exporter.Load()
 	if exporterPtr == nil {
 		return ctx
 	}
@@ -61,7 +58,7 @@ func Export(ctx context.Context, ev Event) context.Context {
 // It will fill in the time.
 func ExportPair(ctx context.Context, begin, end Event) (context.Context, func()) {
 	// get the global exporter and abort early if there is not one
-	exporterPtr := (*Exporter)(atomic.LoadPointer(&exporter))
+	exporterPtr := exporter.Load()
 	if exporterPtr == nil {
 		return ctx, func() {}
 	}

internal/typesinternal/classify_call.go

@@ -8,7 +8,7 @@ import (
 	"fmt"
 	"go/ast"
 	"go/types"
-	_ "unsafe"
+	_ "unsafe" // for go:linkname hack
 )
 
 // CallKind describes the function position of an [*ast.CallExpr].

internal/typesinternal/types.go

@@ -23,7 +23,6 @@ import (
 	"go/token"
 	"go/types"
 	"reflect"
-	"unsafe"
 
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/internal/aliases"
@@ -40,8 +39,7 @@ func SetUsesCgo(conf *types.Config) bool {
 		}
 	}
 
-	addr := unsafe.Pointer(f.UnsafeAddr())
-	*(*bool)(addr) = true
+	*(*bool)(f.Addr().UnsafePointer()) = true
 
 	return true
 }

refactor/satisfy/find.go

@@ -8,9 +8,7 @@
 // interface, and this fact is necessary for the package to be
 // well-typed.
 //
-// THIS PACKAGE IS EXPERIMENTAL AND MAY CHANGE AT ANY TIME.
-//
-// It is provided only for the gopls tool. It requires well-typed inputs.
+// It requires well-typed inputs.
 package satisfy // import "golang.org/x/tools/refactor/satisfy"
 
 // NOTES: