cmd/vet: "possible misuse of unsafe.Pointer" check false positive rate may be too high

[hajimehoshi]

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

$ go version
go version go1.15 darwin/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hajimehoshi/Library/Caches/go-build"
GOENV="/Users/hajimehoshi/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/hajimehoshi/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/hajimehoshi/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/hajimehoshi/ebiten/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/60/khbk2xqn1c5bml1byjn89dwc0000gn/T/go-build111042416=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

mkdir tmp
cd tmp
go mod init foo
GOOS=windows go vet golang.org/x/sys/windows

What did you expect to see?

No warnings

What did you see instead?

$ GOOS=windows go vet golang.org/x/sys/windows
go: finding module for package golang.org/x/sys/windows
go: found golang.org/x/sys/windows in golang.org/x/sys v0.0.0-20200831180312-196b9ba8737a
# golang.org/x/sys/windows
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/env_windows.go:42:39: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1412:19: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1547:18: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1553:32: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1809:27: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:1863:27: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3114:17: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3141:17: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3169:18: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3453:40: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3459:19: possible misuse of unsafe.Pointer
../go/pkg/mod/golang.org/x/sys@v0.0.0-20200831180312-196b9ba8737a/windows/zsyscall_windows.go:3465:27: possible misuse of unsafe.Pointer

[dmitshur]

/cc @alexbrainman @bradfitz per owners.

[rsc]

These are false positives.
(There is a reason this is not one of the vet checks that runs automatically during go test.)
The one in env_windows.go could be rewritten to avoid the check but is still OK as is.
The ones in zsyscall_windows.go cannot be rewritten.

I don't believe there's anything to do here.

[hajimehoshi]

go-vet's false positive happens not only on x/sys/windows but also on my library using Syscall. IIUC, as Syscall returns uintptr, it is impossible to use them as a pointer without violating the unsafe.Pointer rule (is that correct?).

Now I cannot run go-vet for my library on Windows CI due to this issue. I'd be happy if go-vet could avoid such false positives.

[dmitshur]

The check is "possible misuse of unsafe.Pointer", not "misuse of unsafe.Pointer". Based on Russ's comment, I understand there is no actual misuse of unsafe.Pointer in the golang.org/x/sys/windows package.

I'll retitle this to be about cmd/vet then, but I agree this may be working as intended. The presence of a "possible ..." check in vet is incompatible with running in CI systems such that any trigger fails the build.

[dmitshur]

vet's documentation and README suggest that false positives are allowed by design:

Vet uses heuristics that do not guarantee all reports are genuine problems, but it can find errors not caught by the compilers.

Most of vet's checks are heuristic and can generate both false positives (flagging
correct programs) and false negatives (not flagging incorrect ones). The rate of
both these failures must be very small.

This means you cannot rely on default behavior of go vet in CI, where a false positive (that isn't a bug) is generally not acceptable.

@hajimehoshi Do you think there's an issue, or do you agree this is working as intended?

[hajimehoshi]

I think there's still an issue. As we have already known these (uintptr usages of Syscall) are detected as false positives, the unsafe.Pointer rule and go-vet should be updated. Based on the vet's documentation, the rate of false positive (and negative) must be very small, right?

[toy80]

same question here. can you please relax the unsafe pointer check on unmanaged memory? thank you.

I have hundreds of lines of code like this:

case wmGETMINMAXINFO:
	pMMI := (*sMINMAXINFO)(unsafe.Pointer(lParam))
	pMMI.ptMinTrackSize.x = 100
	pMMI.ptMinTrackSize.y = 100
	return 0

and this:

func (fn PfnAllocationFunction) Call(pUserData unsafe.Pointer, size, alignment uintptr, allocationScope SystemAllocationScope) unsafe.Pointer {
	ret, _, _ := call(uintptr(fn), uintptr(pUserData), uintptr(size), uintptr(alignment), uintptr(allocationScope))
	return unsafe.Pointer(ret)
}

[rsc]

Note that the current very-high-confidence vet checks are already enabled during go test.
In the long term we are working toward enabling all of them, once they are high enough confidence.
This one in particular we may adjust or may just never enable.
But for CI purposes, I would suggest that you rely on "go test".