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 should disable inlining reflect.Value.{Pointer,UnsafeAddr} even when package reflect is compiled without it #35073

Closed
wI2L opened this issue Oct 22, 2019 · 18 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@wI2L
Copy link
Contributor

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

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

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

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

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

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

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

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

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

wI2L commented Oct 22, 2019

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

@mdempsky
Copy link
Member

@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 The path to resolution is known, but the work has not been done. label Oct 22, 2019
@cuonglm
Copy link
Member

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

@cuonglm What makes you say that?

@cuonglm
Copy link
Member

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

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

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 👍

@gopherbot
Copy link

Change https://golang.org/cl/222878 mentions this issue: cmd/compile: don't inline reflect.Value.UnsafeAddr/Pointer if enable checkptr

@bcmills
Copy link
Contributor

bcmills commented Mar 11, 2020

@mdempsky, could you update the title of this issue to reflect your current understanding of the root cause?

@mdempsky mdempsky changed the title cmd/compile: -d=checkptr doesn't seem to handle reflect.Value.UnsafeAddr cmd/compile: -d=checkptr should disable inlining reflect.Value.{Pointer,UnsafeAddr} even when package reflect is compiled without it Mar 11, 2020
@mdempsky
Copy link
Member

@bcmills Done.

@golang golang locked and limited conversation to collaborators Apr 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants