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 seem to handle reflect.Value.UnsafeAddr #35073

Open
wI2L opened this issue Oct 22, 2019 · 15 comments
Labels
Milestone

Comments

@wI2L
Copy link
Contributor

@wI2L wI2L commented Oct 22, 2019

goos/goarch: linux/amd64
version: go version devel +7f98c0e Tue Oct 22 08:54:50 2019 +0000 linux/amd64

This program should be valid according to unsafe rules number 5.

package main

import (
	"fmt"
	"reflect"
	"unsafe"
)

func main() {
	n := 10
	v := reflect.ValueOf(&n)
	p := unsafe.Pointer(v.Elem().UnsafeAddr()) // <- panic

	fmt.Println(p)
}

When runned with -d=checkptr, it panics on runtime.PtrArith.

godev run -gcflags=-d=checkptr test.go
panic: (runtime.ptrArith) (0x4a37c0,0xc00000c080)

goroutine 1 [running]:
main.main()
	/home/wpoussie/test.go:12 +0x11a
exit status 2

The rule number 5, unless I misunderstood something, specify that the uintptr-typed return of functions UnsafeAddr and Pointer MUST be converted immediately to unsafe.Pointer after the call, but nothing about that unsafe pointer being reinterpreted immediately to another pointer type.

This variant, where the pointer is reinterpreted and dereferenced at the same time panics as well.

package main

import (
	"fmt"
	"reflect"
	"unsafe"
)

func main() {
	n := 10
	v := reflect.ValueOf(&n)
	n1 := *(*int)(unsafe.Pointer(v.Elem().UnsafeAddr())) // <- panic

	fmt.Println(n1)
}

/cc @mdempsky @cuonglm

Edit: @mdempsky for reference, I found this because of this line in the project we mentionned in #35027.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

Yeah those programs are both valid.

Weird, I thought checkptr was handling this:

// TODO(mdempsky): Make stricter. We only need to exempt
// reflect.Value.Pointer and reflect.Value.UnsafeAddr.
switch n.Left.Op {
case OCALLFUNC, OCALLMETH, OCALLINTER:
return n
}

@wI2L

This comment has been minimized.

Copy link
Contributor Author

@wI2L wI2L commented Oct 22, 2019

I think the reproducers are representative of the issue, but nonetheless I'd like to add the stacktrace of the original panic from my wI2L/jettison project.

godev test -race -covermode=atomic -gcflags=-d=checkptr
--- FAIL: TestCompositeTypes (0.00s)
panic: (runtime.ptrArith) (0x81c900,0xc00000eb80) [recovered]
	panic: (runtime.ptrArith) (0x81c900,0xc00000eb80)

goroutine 12 [running]:
testing.tRunner.func1(0xc0000c2b00)
	/home/wpoussie/github/golang/go/src/testing/testing.go:874 +0x69f
panic(0x81c900, 0xc00000eb80)
	/home/wpoussie/github/golang/go/src/runtime/panic.go:679 +0x1b2
github.com/wI2L/jettison.(*Encoder).encode(0xc0000e8ab0, 0x8c6220, 0x804020, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, ...)
	/home/wpoussie/github/wI2L/jettison/encoder.go:231 +0x1cb
github.com/wI2L/jettison.(*Encoder).Encode(0xc0000e8ab0, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/home/wpoussie/github/wI2L/jettison/encoder.go:201 +0x1ba
github.com/wI2L/jettison.TestCompositeTypes(0xc0000c2b00)
	/home/wpoussie/github/wI2L/jettison/encoder_test.go:187 +0x82a
testing.tRunner(0xc0000c2b00, 0x866220)
	/home/wpoussie/github/golang/go/src/testing/testing.go:909 +0x19a
created by testing.(*T).Run
	/home/wpoussie/github/golang/go/src/testing/testing.go:960 +0x652
exit status 2
FAIL	github.com/wI2L/jettison	0.009s

I will check tonight when I get home if the issue reproduce on darwin/amd64, because I didn't see any issue yesterday evening after I fixed the remaining unsafe pointer misuses per your suggestions in wI2L/jettison@293c0b0.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

It's interesting that:

go run -gcflags=-d=checkptr main.go

panics, but:

go run -gcflags=all=-d=checkptr main.go

doesn't.

@wI2L

This comment has been minimized.

Copy link
Contributor Author

@wI2L wI2L commented Oct 22, 2019

@cuonglm Good catch. So I've reran the test of my package wI2L/jettison again with -gcflags=all=-d=checkptr and it doesn't panic this time.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

@cuonglm Oh right. Thanks for noticing that!

The UnsafeAddr and Pointer methods are marked as go:nocheckptr, which means they won't be inlined. This is necessary because otherwise walk.go can't recognize the function calls in their inlined forms.

But go:nocheckptr only disables inlining when that package is compiled with -d=checkptr. That's why all= makes a difference here.

I'll have to think about how best to handle this. Probably inl.go should be changed to disable inlining those calls based on whether the calling compilation unit is using checkptr, rather than whether package reflect was compiled with it.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@mdempsky This seems work:

        // If marked "go:nocheckptr", don't inline.
	if fn.Func.Pragma&NoCheckPtr != 0 {
		reason = "marked go:nocheckptr"
		return
	}

	// If -d checkptr compilation, don't inline.
	if Debug_checkptr != 0 {
		reason = "compile with -d=checkptr"
		return
	}
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@mdempsky This seems work:

        // If marked "go:nocheckptr", don't inline.
	if fn.Func.Pragma&NoCheckPtr != 0 {
		reason = "marked go:nocheckptr"
		return
	}

	// If -d checkptr compilation, don't inline.
	if Debug_checkptr != 0 {
		reason = "compile with -d=checkptr"
		return
	}

Ah no, it doesn't, as it will disable inline if compiled with -d=checkptr alone.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

I think the short-term solution here is just to declare that you have to enable -d=checkptr for all compilation units if you're going to use it (i.e., you have to use -gcflags=all=-d=checkptr, not just -gcflags=-d=checkptr).

The longer-term fix requires making inlining smarter about pragmas. This might be beneficial for the runtime too, since currently we disable inlining for a lot of runtime functions just because it's the easiest way to ensure write barriers are enabled/disabled correctly.

@wI2L

This comment has been minimized.

Copy link
Contributor Author

@wI2L wI2L commented Oct 22, 2019

@mdempsky Feel free to close this issue then, or keeping it for future references.

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

@wI2L Thanks, I'd like to keep it open for now. There's fundamentally no reason -d=checkptr can't work on a per-package basis, and I would like it to work; it just doesn't work today, and I don't think it's a priority at the moment.

@mdempsky mdempsky added this to the Unplanned milestone Oct 22, 2019
@mdempsky mdempsky added the NeedsFix label Oct 22, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@mdempsky Sorry to hjack here, but It sounds like -d=checkptr does not handle unsafe pointer rule 3rd at this moment, is it intended?

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

@cuonglm What makes you say that?

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@mdempsky This one should be invalid per rule 3rd, but it passes:

package main

import "unsafe"

func main() {
	n := 10
	b := make([]byte, n)
	end := unsafe.Pointer(uintptr(unsafe.Pointer(&b[0])) + uintptr(n))
	println(end)
}

I tried other invalid cases in rule 3rd, the only one that -d=checkptr reports correctly is:

u := unsafe.Pointer(nil)
p := unsafe.Pointer(uintptr(u) + offset)
@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

@cuonglm The detection is best effort. It's expected to have false negatives, but it should never have false positives.

In your example, make([]byte, 10) is actually allocated as a 16-byte GC object by the runtime. So &b[0] + 10 still points into this GC object, and we're not able to detect that it's unsafe.

If you change n := 10 to n := 16, then it's detected.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@cuonglm The detection is best effort. It's expected to have false negatives, but it should never have false positives.

In your example, make([]byte, 10) is actually allocated as a 16-byte GC object by the runtime. So &b[0] + 10 still points into this GC object, and we're not able to detect that it's unsafe.

If you change n := 10 to n := 16, then it's detected.

Thanks for details explanation 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.