go: cmd/compile: -d=checkptr doesn't detect invalid pointer fields in converted pointers-to-structs

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

$ go version
go version devel +0915a19a11 Fri Dec 6 05:12:15 2019 +0000 linux/amd64

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

go env Output

$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="~/.cache/go-build"
GOENV="~/.config/go/env"
GOEXE=""
GOFLAGS=" -mod="
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="~/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="~/projects/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="~/projects/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build392569888=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package tmp

import (
	"testing"
	"unsafe"
)

func TestUnsafeCast(t *testing.T) {
	var x uint32 = 0
	p := (*uint64)(unsafe.Pointer(&x))
	t.Log(*p)
}

func TestUnsafeStructCast(t *testing.T) {
	var x uint32 = 0

	type A struct {
		p *uint32
	}
	type B struct {
		p *uint64
	}

	v := &A{ptr:&x}
	p := (*B)(unsafe.Pointer(v))
	t.Log(*p.p)
}

What did you expect to see?

Both TestUnsafeCast and TestUnsafeStructCast fail with go test -gcflags=-d=checkptr, since both use invalid unsafe.Pointer casts.

What did you see instead?

Only the TestUnsafeCast fails. Pointer fields are not verified properly.

About this issue

Original URL
State: open
Created 5 years ago
Comments: 17 (17 by maintainers)

Most upvoted comments

@bcmills Yeah, I think graph isomorphism was too general of a problem to reference here. (In particular, graph isomorphism assumes edges are indistinguishable, but our edges are uniquely distinguishable by their memory offset within the origin node/variable.)

I think fully and recursively tracing out all pointers to make sure they point to the right type would be sufficient. It would take at least time proportional to the amount of memory reachable from the converted pointer though, which could be substantial in some cases.

That’s also just for handling conversions involving numbers, pointers, and structs. It becomes even more complex if we want to worry about Go language types (channels, maps, interfaces, functions).

mdempsky on Mar 11, 2020

I thought about doing this, but it would be very difficult in general. Because data structures can be cyclic, to be fully general, we’d have to test for graph isomorphism, which is NP complete.

Maybe there’s some sweet spot of partial testing that still helps but isn’t intractable.

(I thought I filed an issue for this, but I can’t immediately find it. Maybe I realized the difficulty before even filing the issue.)

mdempsky on Dec 6, 2019

@dennwc Thanks. Questions related to the issue would be best asked and answered here. General questions about compiler internals would be best on golang-dev@.

mdempsky on Jun 2, 2020

Since this is more or less C/syscall-related, limiting the scope to numbers, pointers and structs should be sufficient.

Also, the overhead is clear, but this would still be very useful for debugging unsafe code. I already found a few bugs in one complex codebase using checkptr and I have a few reasons to believe that there is a bug involving an implicit conversion hidden in it as well.

@mdempsky I could help with the implementation, but I might have a few questions along the way. Can I contact you in the Gophers Slack or somewhere else?

dennwc on Jun 2, 2020