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

[dennwc]

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.

[randall77]

@mdempsky

[cuonglm]

This works as intended. The rule 1 of unsafe.Pointer:

• B is not larger than A: unsafe.Sizeof(A{}) == unsafe.Sizeof(B{})
• B and A has the same memory layout, one pointer field.

If you change to:

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

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

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

Then the compiler will report correctly.

[dennwc]

@cuonglm Yes, I'm aware that it will work if the struct contains the field directly. It may not break any rules for the root type, but it does break the same rules for the pointer type field.

[cuonglm]

@cuonglm Yes, I'm aware that it will work if the struct contains the field directly. It may not break any rules for the root type, but it does break the same rules for the pointer type field.

What do you mean "but it does break the same rules for the pointer type field"? In your example, A and B has exactly the same memory layout, both contain one pointer field, so it does not break any unsafe.Pointer rule, so no report.

[dennwc]

@cuonglm Yes, both are pointers of the same size.

However, if I understood correctly, checkptr was intended specifically to find bugs with invalid pointer conversions. Although it's intended to only "fail if unsafe.Pointer rules are violated", I think there is a significant value in detecting cases similar to one I've described. If not for checkptr, then maybe for a different flag.

[cuonglm]

@cuonglm Yes, both are pointers of the same size.

However, if I understood correctly, checkptr was intended specifically to find bugs with invalid pointer conversions. Although it's intended to only "fail if unsafe.Pointer rules are violated", I think there is a significant value in detecting cases similar to one I've described. If not for checkptr, then maybe for a different flag.

Maybe you can read https://go-review.googlesource.com/c/go/+/162237

I quote the commit message here:

cmd/compile: add -d=checkptr to validate unsafe.Pointer rules

This CL adds -d=checkptr as a compile-time option for adding instrumentation to check that Go code is following unsafe.Pointer safety rules dynamically. In particular, it currently checks two things:

1. When converting unsafe.Pointer to *T, make sure the resulting pointer is aligned appropriately for T.

2. When performing pointer arithmetic, if the result points to a Go heap object, make sure we can find an unsafe.Pointer-typed operand that pointed into the same object.

[dennwc]

Sure, but again, it can be useful to check both rules for pointer fields as well (if the element types are not the same).

Just to clarify: I'm not claiming it's a bug in checkptr. It's rather a feature request to (optionally) add checks for struct fields which are pointers as well.

[cuonglm]

Sure, but again, it can be useful to check both rules for pointer fields as well (if the element types are not the same).

Just to clarify: I'm not claiming it's a bug in checkptr. It's rather a feature request to (optionally) add checks for struct fields which are pointers as well.

But those struct fields are the same in your example, both fields are pointer type. So I don't get the point that you think it's invalid.

[dennwc]

OK, sorry, let me explain it a bit more carefully.

Both examples execute the same operation (semantically): cast a pointer to a single uint32 value to a pointer to uint64, which is invalid according to the unsafe.Pointer rules.

The only difference is that the second example does the conversion by using a proxy struct type of the same size. It silently converts a *uint32 field to a *uint64 field, achieving the same invalid behavior as the first example, but without triggering a runtime failure.

So the point is not that *A -> *B conversion is invalid, but that the cast of underlying pointer field is invalid.

In fact, the same code can be made valid by switching the type of x to [2]uint32, for example.

[cuonglm]

OK, sorry, let me explain it a bit more carefully.

Both examples execute the same operation (semantically): cast a pointer to a single uint32 value to a pointer to uint64, which is invalid according to the unsafe.Pointer rules.

The only difference is that the second example does the conversion by using a proxy struct type of the same size. It silently converts a *uint32 field to a *uint64 field, achieving the same invalid behavior as the first example, but without triggering a runtime failure.

No, they're total different. One cast from uint32 to uint64, one cast from a struct which contains a pointer * to uint32 to a struct which contains a pointer * to uint64.

If you want to understand as using A and B as proxy, then you should set the field to uint32 and uint64 as in my example (Though I don't think it's the right way to think like that), to make them equivalent type. So now it becomes:

the second example does the conversion by using a proxy struct type of the same size.
It silently converts a `uint32` field to a `uint64` field 

So the point is not that *A -> *B conversion is invalid, but that the cast of underlying pointer field is invalid.

There's no such operation, the conversion is done on the struct pointer, not their fields.

[dennwc]

There's no such operation

You are right, there is no such explicit operation. However, there is implicit (or semantic, if you like) operation that happens during this *A -> *B conversion.

If you run both tests, it will be clear that both are invalid semantically. Does it matter that much if there is no field conversion operation defined explicitly in this case?

Both versions exhibit an invalid behavior as a matter of fact, so I'm trying to propose to introduce more (optional) runtime check(s) to detect those issues as well.

[mdempsky]

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.)

[cuonglm]

@mdempsky Should the unsafe.Pointer rules updated first?

[bcmills]

@mdempsky, why would we have to test for graph isomorphism?

Why wouldn't it suffice to check that all pointers reachable by tracing the data structure from a typed root match the size and pointer layout of the type through which they are reached?

[mdempsky]

@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).

[dennwc]

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?

[mdempsky]

@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@.