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

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Oct 17, 2019

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

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 17, 2019

@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

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 17, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 17, 2019

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

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 17, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 17, 2019

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

@gopherbot gopherbot closed this in b6b984f Oct 17, 2019
@ainar-g

This comment has been minimized.

Copy link
Contributor Author

@ainar-g ainar-g commented Oct 17, 2019

Holy Dennis Ritchie that was quick! Amazing, thanks!

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 18, 2019

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.