cmd/vet: `go vet` complains `unsafe.Slice` usage

[hajimehoshi]

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

$ go version
go version go1.19.2 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=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/hajimehoshi/Library/Caches/go-build"
GOENV="/Users/hajimehoshi/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
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"
GOVCS=""
GOVERSION="go1.19.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/hajimehoshi/test/vet/go.mod"
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/60/khbk2xqn1c5bml1byjn89dwc0000gn/T/go-build1170881183=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

package main

import (
        "unsafe"
)

func main() {
        var foo uintptr = 0x12345678
        _ = unsafe.Slice((*byte)(unsafe.Pointer(foo)), 0)
}

module foo

go 1.19

and run go vet

What did you expect to see?

No complaints

What did you see instead?

$ go vet 
# foo
./main.go:9:27: possible misuse of unsafe.Pointer

When reflect.SliceHeader is used, there is no such a problem since the Data field is uintptr and we could use a uintptr value without warnings. So this sounds like a kind of regression.

[D1CED]

When reading the documentation for unsafe.Pointer it states that

(2) Conversion of a Pointer to a uintptr (but not back to Pointer).
...
Conversion of a uintptr back to Pointer is not valid in general.
...
The remaining patterns enumerate the only valid conversions from uintptr to Pointer.

There is no pattern listed that allows what you did in the example.
Hence the given conversion of uintptr to unsafe.Pointer is invalid.
You are creating a string that points to no sensible content (from the pov of the Go runtime).

But lets look what you actually trying to do.

// The lifetime of the underlying C string is shorter than s [1].
// Copy the bytes to ensure that the returned string's lifetime is unrelated to s.
// [1] https://developer.apple.com/documentation/foundation/nsstring/1411189-utf8string?language=objc
src := unsafe.Slice((*byte)(unsafe.Pointer(s.Send(sel_UTF8String))), s.Send(sel_length))

As can be extracted from the comment you attempt to convert a "C string" to a Go string. Thankfully there is already a function that does this for you as part of cgo in C.GoString.

In the second case

func (b Buffer) CopyToContents(data unsafe.Pointer, lengthInBytes uintptr) {
    contents := b.buffer.Send(sel_contents)
    copy(unsafe.Slice((*byte)(unsafe.Pointer(contents)), lengthInBytes), unsafe.Slice((*byte)(data), lengthInBytes))
    if runtime.GOOS != "ios" {
        b.buffer.Send(sel_didModifyRange, 0, lengthInBytes)
    }
}

you copy the contents of a buffer from the Objective-C heap to Go. There is also a cgo function for that called C.GoBytes but that would return a newly allocated copy so instead I'd suggest you simply use C.memcpy instead of a Go copy here. This way you avoid having to create Go slices.

Hope this was helpful.
J.M.H.

[hajimehoshi]

As can be extracted from the comment you attempt to convert a "C string" to a Go string. Thankfully there is already a function that does this for you as part of cgo in C.GoString.

Unfortunately Cgo is not our option. We aim to avoid Cgo by using purego. So we want to treat a uintptr value as a data source. We could do it with reflect.SliceHeader without warnings, which is my point.

[D1CED]

I'm pretty sure the reflect.SliceHeader was also invalid as you also create a Go slice that points neither into the Go heap or static memory. Don't get me wrong both versions probably work just fine in the current implementation but it is like undefined behavior in C.

As an alternative you can copy each byte manually but this will likely perform worse because these loops will not be vectorized/unrolled.

So to get an optimized copy you have options

1. write your own copy function (best in assembly),
2. link to an internal Go runtime function (runtime.memmove, runtime.duffcopy) or
3. link to an (Objective-)C function (memcpy).

[hajimehoshi]

I'm pretty sure the reflect.SliceHeader was also invalid as you also create a Go slice that points neither into the Go heap or static memory. Don't get me wrong both versions probably work just fine in the current implementation but it is like undefined behavior in C.

In the above NSString case, the value points to a C heap, but the type is uintptr. A uintptr as a C address is very often used e.g. when low-level APIs like syscall.Syscall is used. It's OK that this can be like an undefined behavior in C, as this is an 'unsafe' package.

Using an alternative way to just avoid go-vet warnings doesn't solve the root problem. Actually if we just wanted to avoid warnings, we could use reflect.SliceHeader. The root problem here is that go-vet's warnings are false positives, and there was not a problem with the old API. We should be able to use unsafe.Slice without warnings.

[ianlancetaylor]

It's true that the unsafe package, and therefore vet, has an exception for reflect.SliceHeader. It might be appropriate for vet to not warn about passing a converted uintptr to unsafe.Slice, as that operation is clearly unsafe, and that is one of the uses of unsafe.Slice.

CC @adonovan

[bcmills]

It's true that the unsafe package, and therefore vet, has an exception for reflect.SliceHeader.

The exception for reflect.SliceHeader does not cover this use-case, though: the lack of a vet warning seems to be a false-negative. Per https://pkg.go.dev/unsafe#Slice, “SliceHeader and StringHeader are only valid when interpreting the content of an actual slice or string value. … In this usage hdr.Data is really an alternate way to refer to the underlying pointer in the string header, not a uintptr variable itself.”

Note that it is, in fact, possible to silence this vet warning when using unsafe.Slice, by adding another level of indirection: essentially, telling the compiler “you may think this variable is a uintptr but it's actually a *byte.”

	var foo uintptr = 0x12345678
	_ = unsafe.Slice(*(**byte)(unsafe.Pointer(&foo)), 0)

[ianlancetaylor]

I agree that the use is unsafe. That said, one of the goals of unsafe.Slice is to permit people to create slices out of things that aren't ordinary pointers. The vet warnings for unsafe.Pointer in general are useful because it's easy to misunderstand unsafe.Pointer. But passing a non-pointer to unsafe.Slice by converting it to unsafe.Pointer is not, in my opinion, one of the cases that is easy to misunderstand. If the call to unsafe.Slice works at all, then it's probably doing what the user expects. The vet warning doesn't help.

[bcmills]

The vet warning here is from converting a variable of type uintptr to a variable of type unsafe.Pointer. It is almost completely orthogonal to unsafe.Slice.

Moreover, silencing the unsafe.Pointer warning in general for arguments to unsafe.Slice would miss real misuses: https://go.dev/play/p/xprDEzQCM7G

Especially given how trivial the workaround is, I don't think it's worth trying to fine-tune the unsafe.Pointer definition (and the associated vet warnings) with more special cases for unsafe.Slice.