Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dennwc opened this issue Dec 6, 2019 · 17 comments

Comments

@dennwc
Copy link
Contributor

@dennwc dennwc commented Dec 6, 2019

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
Copy link
Contributor

@randall77 randall77 commented Dec 6, 2019

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

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
Copy link
Contributor Author

@dennwc dennwc commented Dec 6, 2019

@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
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

@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
Copy link
Contributor Author

@dennwc dennwc commented Dec 6, 2019

@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
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

@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
Copy link
Contributor Author

@dennwc dennwc commented Dec 6, 2019

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
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

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
Copy link
Contributor Author

@dennwc dennwc commented Dec 6, 2019

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
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

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
Copy link
Contributor Author

@dennwc dennwc commented Dec 6, 2019

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
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2019

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
Copy link
Contributor

@cuonglm cuonglm commented Dec 6, 2019

@mdempsky Should the unsafe.Pointer rules updated first?

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 11, 2020

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

@bcmills bcmills changed the title cmd/compile: -d=checkptr doesn't handle pointer fields cmd/compile: -d=checkptr doesn't detect invalid pointer fields in converted pointers-to-structs Mar 11, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 11, 2020

@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
Copy link
Contributor Author

@dennwc dennwc commented 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?

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 2, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.