golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.29k stars 17.57k forks source link

cmd/cgo: a type alias of unsafe.Pointer is not treated as unsafe.Pointer #67333

Open dominikh opened 4 months ago

dominikh commented 4 months ago

Go version

go version devel go1.23-07fc59199b Sun May 12 05:36:29 2024 +0000 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/dominikh/.cache/go-build'
GOENV='/home/dominikh/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/dominikh/prj/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/dominikh/prj'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/dominikh/prj/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/dominikh/prj/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.23-07fc59199b Sun May 12 05:36:29 2024 +0000'
GODEBUG=''
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/dominikh/prj/src/example.com/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1241008282=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Run the following code:

package main

// void foo(void *arg) {}
import "C"
import "unsafe"

type T struct {
    self int
    x    *int
}

type up = unsafe.Pointer

func main() {
    x := T{x: new(int)}

    C.foo(unsafe.Pointer(&x))      // reports "cgo argument has Go pointer to unpinned Go pointer", as expected
    C.foo(unsafe.Pointer(&x.self)) // doesn't report anything, as desired
    C.foo(up(&x.self))             // reports "cgo argument has Go pointer to unpinned Go pointer", which is surprising
}

What did you see happen?

The first and third calls panic with runtime error: cgo argument has Go pointer to unpinned Go pointer. The second call doesn't panic.

What did you expect to see?

Only the first call to panic.

Arguably one might also expect all 3 calls to panic, since this also panics, regardless of the use of an alias:

y := unsafe.Pointer(&x.self)
C.foo(unsafe.Pointer(y))

Which makes the use of the 2nd form rather brittle.

cuonglm commented 4 months ago

This seems to me a cmd/cgo issue, not the runtime.

cgo does not recorgnize that up(&x.self) is a type conversion, so it emits the cgo check pointer call with up(&x.self) as argument, not &x.self in case of unsafe.Pointer(&x.self).

ianlancetaylor commented 4 months ago

I don't know how to fix this except to do full type checking on the cgo input file, which is difficult as we currently expect the type checker to run on cgo output, not cgo input. Fixing this correctly may require something like #16623, which is a lot of work.

cuonglm commented 4 months ago

@ianlancetaylor We could probably record type alias only, something like map[string]string{"up": "unsafe.Pointer"}, then the p.isType check can correctly recognize and stripe the type conversion.

ianlancetaylor commented 4 months ago

We can handle simple cases, but what if we import some other package that has type MyUnsafePointer = unsafe.Pointer and then refer to it as otherpkg.MyUnsafePointer(x) ? Is it better to have some type aliases work and some not, or is it better to not try to make type aliases work? It's not clear to me which approach is less confusing.

adonovan commented 4 months ago

A related previous issue was https://github.com/golang/go/issues/57926, in which users exploited the desugaring of cgo to declare methods on types that appear to belong to package C: func (C.T) method() {} The resolution was to use syntactic heuristics to try to reject it, so that we at least make clear that this is not allowed, even if we can't detect it in general (e.g. in the presence of aliasing) without type checking, which is infeasible for the reason @ianlancetaylor noted.

dmitshur commented 4 months ago

@ianlancetaylor Assigned it to you for now, please feel free to update.

ianlancetaylor commented 4 months ago

Thanks, I'm unassigning as I have no plans to work on this.