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: expand checkptr to find conversions of smaller types into bigger #34959

Closed
ainar-g opened this issue Oct 17, 2019 · 8 comments
Closed

Comments

@ainar-g
Copy link
Contributor

ainar-g commented Oct 17, 2019

Per discussion in golang-dev.

The checkptr dynamic analyser doesn't find bad type conversions between types where the destination type is greater than the source type. E. g.:

var i32sink int32

type t1 struct { a, b int32 }

type t2 struct { a, b, c, d int32 }

func main() {
	for i := 0; i < 1000; i++ {
		var badt2 = *(*t2)(unsafe.Pointer(&t1{}))
		i32sink = badt2.d
	}
}

Playground link: https://play.golang.org/p/32lWMkFPvm5.

According to @mdempsky:

Detecting that is not currently supported, but I think there may at least be some cases where we can detect it. (…)
@randall77
Copy link
Contributor

We do this sometimes in runtime and reflect. For example, see reflect/type.go:(*rtype).Elem. It does a switch on t.Kind() and then casts an *rtype to other types (e.g. *arrayType) for which the base type is larger.

So we'd need to check that the underlying object allocation is large enough. Just having the base type be larger is not in itself an error.

@mdempsky
Copy link
Member

@randall77 Yeah. I was thinking that when we convert p to *T, we just check that p and p+sizeof(T)-1 both point into the same heap object, kinda like how we do for pointer arithmetic checking

@mdempsky
Copy link
Member

It might also be worth recognizing (*[VeryBig]T)(ptr)[:n:m], since that's a common idiom for turning pointers into slices.

@gopherbot
Copy link

Change https://golang.org/cl/201778 mentions this issue: cmd/compile: detect unsafe conversions from smaller to larger types

@mdempsky
Copy link
Member

CL 201778 can now detect the invalid conversion here:

package main

import "unsafe"

var i32sink int32

type t1 struct{ a, b int32 }

type t2 struct{ a, b, c, d int32 }

//go:noinline
func alloc() *t1 {
	return &t1{}
}

func main() {
	for i := 0; i < 1000; i++ {
		var badt2 = *(*t2)(unsafe.Pointer(alloc()))
		i32sink = badt2.d
	}
}

It still doesn't detect the original example, but because in that one &t1{} is stack allocated, and the instrumentation currently can only validate pointers to heap objects.

One solution would be to make -d=checkptr change escape analysis to treat conversions to unsafe.Pointer as an escaping operation, which would force the objects onto the heap and allow better runtime validation.

gopherbot pushed a commit that referenced this issue Oct 17, 2019
This CL extends the runtime instrumentation for (*T)(ptr) to also
check that the first and last bytes of *(*T)(ptr) are part of the same
heap object.

Updates #22218.
Updates #34959.

Change-Id: I2c8063fe1b7fe6e6145e41c5654cb64dd1c9dd41
Reviewed-on: https://go-review.googlesource.com/c/go/+/201778
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/201781 mentions this issue: cmd/compile: escape unsafe.Pointer conversions when -d=checkptr

@ainar-g
Copy link
Contributor Author

ainar-g commented Oct 17, 2019

Holy Dennis Ritchie that was quick! Amazing, thanks!

@gopherbot
Copy link

Change https://golang.org/cl/201840 mentions this issue: cmd/compile: only escape unsafe.Pointer conversions when -d=checkptr=2

gopherbot pushed a commit that referenced this issue Oct 18, 2019
Escaping all unsafe.Pointer conversions for -d=checkptr seems like it
might be a little too aggressive to enable for -race/-msan mode, since
at least some tests are written to expect unsafe.Pointer conversions
to not affect escape analysis.

So instead only enable that functionality behind -d=checkptr=2.

Updates #22218.
Updates #34959.

Change-Id: I2f0a774ea5961dabec29bc5b8ebe387a1b90d27b
Reviewed-on: https://go-review.googlesource.com/c/go/+/201840
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants