proposal: cmd/go: consider adding unsafeptr to go vet during test list

[lasiar]

What version of Go are you using (go version)?

$ go version
go version go1.19.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOOS="darwin"
GOPRIVATE="gitlab.corp.mail.ru/taxmonitor/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.19.5"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/49/y7d47mwn7rx69725q6y7ylx00000gp/T/go-build1390474754=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

compile with flag -race
https://go.dev/play/p/wNXEyU8p6XD?v=gotip

What did you expect to see?

Two same addresses, example:
0xc00012e280
0xc00012e280

What did you see instead?

fatal error: checkptr: pointer arithmetic result points to invalid allocation

I think the error cause incorrect compile on this place, because original value is null.

[ianlancetaylor]

The expression lp = unsafe.Pointer(sh.Data + ui*size) is not permitted by the rules at https://pkg.go.dev/unsafe#Pointer.

[bcmills]

Note that go vet explicitly flags this misuse:

$ go vet .
# example
./main.go:28:7: possible misuse of unsafe.Pointer

I am not sure why the unsafeptr check is not included in the default set run by go test. (It isn't mentioned in #18085.)

[rsc]

This looks like a shortcoming in the unsafe package docs that propagated into both the checkptr compiler code and the unsafeptr vet code. The compiler knows that when you read a SliceHeader or StructHeader Data field, it is as though you are writing uintptr(an unsafe.Pointer-typed field), but the docs and the checks don't apply this rule.

I don't think it's particularly defensible to say that

vp := unsafe.Pointer(sh.Data)
l1 = unsafe.Pointer(uintptr(vp) + ui*size)

is permitted but somehow the single-line form

lp = unsafe.Pointer(sh.Data + ui*size)

is not.

[mdempsky]

The compiler knows that when you read a SliceHeader or StructHeader Data field, it is as though you are writing uintptr(an unsafe.Pointer-typed field)

Caveat: The compiler knows this well enough to satisfy safety rule 6 in isolation, but I'm not certain it already satisfies it in combination with rule 3. That is, off hand, I can't think of any reason why unsafe.Pointer(sh.Data + E) (for arbitrary uintptr-typed expression E) wouldn't work today, but I've at least not had that in mind as an explicit goal when reviewing compiler code.

It's certainly not safe today to combine rules 3 and 5; e.g., unsafe.Pointer(v.Pointer() + E) can fail if E requires any function calls (including implicit runtime calls, possibly from instrumentation), because the v.Pointer() result will be spilled as a uintptr-typed temporary. We specially recognize the expression unsafe.Pointer(F(...)) (for arbitrary uintptr-returning F), and ensure that whole expression is spilled as an unsafe.Pointer-typed temporary instead.

One nice thing about that is it's safe to do regardless of what F actually is. But if we need to support unsafe.Pointer(v.Pointer() + E), then we're going to need to be stricter about recognizing just reflect.Value.Pointer and reflect.Value.UnsafeAddr calls, which then opens up the same questions as #34684 about whether indirect calls are allowed.

[rsc]

I think we can close this as a duplicate of #58625. In particular, the reports examined in #58625 (comment) make clear that the unsafeptr check is not accurate enough to turn on by default in 'go test' and instead probably should be removed from vet entirely. In any event, that conversation is happening on #58625 and need not be duplicated here.

[rsc]

This proposal is a duplicate of a previously discussed proposal, as noted above,
and there is no significant new information to justify reopening the discussion.
The issue has therefore been declined as a duplicate.
— rsc for the proposal review group