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, runtime, reflect: pointers to go:notinheap types must be stored indirectly in interfaces #42076

Closed
randall77 opened this issue Oct 19, 2020 · 15 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 19, 2020

(This bug is a consequence of the fixes for issue #40954.)

package main

import "reflect"

//go:notinheap
type T struct {
	x int
}

func main() {
	reflect.ValueOf((*T)(nil)).Pointer()
}

This program panics with:

panic: can't call pointer on a non-pointer Value

goroutine 1 [running]:
reflect.Value.pointer(...)
	/Users/khr/sandbox/ro/src/reflect/value.go:95
reflect.Value.Pointer(0x1081e40, 0x0, 0x16, 0x0)
	/Users/khr/sandbox/ro/src/reflect/value.go:1457 +0x1b6
main.main()
	/Users/khr/gowork/tmp1.go:11 +0x9b

reflect is confused by a type that claims to be a pointer, but has no pointer bits. It has no pointer bits because pointers to go:notinheap types are treated as uintptrs by the compiler.

More generally, we can't ever convert *T to unsafe.Pointer, as that's the equivalent of converting a uintptr, potentially containing a bad pointer value, to a pointer. Particularly, we can't store a *T in the data field of an interface, or in reflect.Value.ptr. We have to store them indirectly instead.

This issue originally came up with the a call to reflect.DeepEqual which ended up calling Pointer on one of these types.
Note that reflect.DeepEqual fundamentally doesn't work on incomplete types like this. They are represented as struct{}, so pointer equality doesn't really work. This program:

package main

import (
	"reflect"
	"unsafe"
)

//go:notinheap
type T struct {
}

var a [2]int

func main() {
	x := (*T)(unsafe.Pointer(&a[0]))
	y := (*T)(unsafe.Pointer(&a[1]))
	println(reflect.DeepEqual(x, y))
}

Will print true after this issue is fixed, although one would probably expect it to return false. Something about incomplete types and deepequal just don't play well together.

But it shouldn't panic.

@bartle-stripe
Copy link

@bartle-stripe bartle-stripe commented Oct 19, 2020

Would you expect this to also manifest as a segmentation fault? From running go test github.com/bsm/go-sparkey/...:

[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x47cfa51]

runtime stack:
runtime.throw(0x43f28b2, 0x2a)
	/Users/bartle/src/go/src/runtime/panic.go:1116 +0x72
runtime.sigpanic()
	/Users/bartle/src/go/src/runtime/signal_unix.go:704 +0x48c

goroutine 19 [syscall]:
runtime.cgocall(0x433a1f0, 0xc000187090, 0xc000105680)
	/Users/bartle/src/go/src/runtime/cgocall.go:133 +0x5b fp=0xc000187060 sp=0xc000187028 pc=0x400785b
github.com/bsm/go-sparkey._Cfunc_sparkey_logiter_fill_value(0x8204080, 0x0, 0x200, 0xc000328000, 0xc00026e4b0, 0x0)
	_cgo_gotypes.go:272 +0x4d fp=0xc000187090 sp=0xc000187060 pc=0x431d44d
github.com/bsm/go-sparkey.(*valueReader).Read.func1(0xc00011c3c8, 0x200, 0xc000328000, 0xc00026e4b0, 0x4369b40)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/iter.go:262 +0xb1 fp=0xc0001870e0 sp=0xc000187090 pc=0x4324091
github.com/bsm/go-sparkey.(*valueReader).Read(0xc00011c3c8, 0xc000328000, 0x200, 0x200, 0xc000328000, 0x0, 0x0)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/iter.go:262 +0x70 fp=0xc000187120 sp=0xc0001870e0 pc=0x431fe90
bytes.(*Buffer).ReadFrom(0xc0001871c8, 0x8039118, 0xc00011c3c8, 0x8039118, 0xc000000000, 0x4007f1b)
	/Users/bartle/src/go/src/bytes/buffer.go:204 +0xb1 fp=0xc000187190 sp=0xc000187120 pc=0x40e9431
io/ioutil.readAll(0x8039118, 0xc00011c3c8, 0x200, 0x0, 0x0, 0x0, 0x0, 0x0)
	/Users/bartle/src/go/src/io/ioutil/ioutil.go:36 +0xe5 fp=0xc000187210 sp=0xc000187190 pc=0x40ef445
io/ioutil.ReadAll(...)
	/Users/bartle/src/go/src/io/ioutil/ioutil.go:45
github.com/bsm/go-sparkey.(*LogIter).Value(0xc000242ba0, 0xc00026e401, 0x2, 0x2, 0x0, 0x0)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/iter.go:126 +0x90 fp=0xc000187260 sp=0xc000187210 pc=0x431f6b0
github.com/bsm/go-sparkey.(*HashIter).Get(0xc000219980, 0xc00026e4a8, 0x2, 0x2, 0xc000231940, 0x445dfa0, 0xc000187300, 0x40122b8, 0x2)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/iter.go:175 +0x93 fp=0xc0001872a0 sp=0xc000187260 pc=0x431f933
github.com/bsm/go-sparkey.(*HashReader).Get(0xc00021b770, 0xc00026e4a8, 0x2, 0x2, 0x0, 0x0, 0x0, 0x0, 0x0)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/hash.go:98 +0xe7 fp=0xc000187310 sp=0xc0001872a0 pc=0x431f007
github.com/bsm/go-sparkey.glob..func3.6()
	/Users/bartle/go/src/github.com/bsm/go-sparkey/hash_test.go:81 +0x4b2 fp=0xc0001873d8 sp=0xc000187310 pc=0x4326b32
github.com/onsi/ginkgo/internal/leafnodes.(*runner).runSync(0xc00011f0e0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113 +0xa3 fp=0xc000187438 sp=0xc0001873d8 pc=0x42c6da3
github.com/onsi/ginkgo/internal/leafnodes.(*runner).run(0xc00011f0e0, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/leafnodes/runner.go:64 +0xd7 fp=0xc0001875d8 sp=0xc000187438 pc=0x42c69b7
github.com/onsi/ginkgo/internal/leafnodes.(*ItNode).Run(0xc00012d160, 0x4457e20, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/leafnodes/it_node.go:26 +0x67 fp=0xc0001876f8 sp=0xc0001875d8 pc=0x42c6067
github.com/onsi/ginkgo/internal/spec.(*Spec).runSample(0xc00025ea50, 0x0, 0x4457e20, 0xc0001548c0)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/spec/spec.go:215 +0x691 fp=0xc0001879e8 sp=0xc0001876f8 pc=0x42c9811
github.com/onsi/ginkgo/internal/spec.(*Spec).Run(0xc00025ea50, 0x4457e20, 0xc0001548c0)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/spec/spec.go:138 +0xf2 fp=0xc000187a38 sp=0xc0001879e8 pc=0x42c8f52
github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpec(0xc00024c8c0, 0xc00025ea50, 0x0)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:200 +0x111 fp=0xc000187a90 sp=0xc000187a38 pc=0x42ea0f1
github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).runSpecs(0xc00024c8c0, 0x1)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:170 +0x127 fp=0xc000187b18 sp=0xc000187a90 pc=0x42e9c07
github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run(0xc00024c8c0, 0xc000121f68)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:66 +0x117 fp=0xc000187b58 sp=0xc000187b18 pc=0x42e9417
github.com/onsi/ginkgo/internal/suite.(*Suite).Run(0xc00017a070, 0x8017320, 0xc000106600, 0x43e0d08, 0x7, 0xc0001136c0, 0x1, 0x1, 0x445f9e0, 0xc0001548c0, ...)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/suite/suite.go:79 +0x586 fp=0xc000187db8 sp=0xc000187b58 pc=0x42ebb46
github.com/onsi/ginkgo.RunSpecsWithCustomReporters(0x44582e0, 0xc000106600, 0x43e0d08, 0x7, 0xc000187f20, 0x1, 0x1, 0x4607603)
	/Users/bartle/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:229 +0x238 fp=0xc000187ed0 sp=0xc000187db8 pc=0x42f9738
github.com/onsi/ginkgo.RunSpecs(0x44582e0, 0xc000106600, 0x43e0d08, 0x7, 0x0)
	/Users/bartle/go/src/github.com/onsi/ginkgo/ginkgo_dsl.go:210 +0x168 fp=0xc000187f40 sp=0xc000187ed0 pc=0x42f9388
github.com/bsm/go-sparkey.TestSuite(0xc000106600)
	/Users/bartle/go/src/github.com/bsm/go-sparkey/sparkey_test.go:54 +0xf6 fp=0xc000187f80 sp=0xc000187f40 pc=0x431c376
testing.tRunner(0xc000106600, 0x4401840)
	/Users/bartle/src/go/src/testing/testing.go:1123 +0xef fp=0xc000187fd0 sp=0xc000187f80 pc=0x40fd0af
runtime.goexit()
	/Users/bartle/src/go/src/runtime/asm_amd64.s:1374 +0x1 fp=0xc000187fd8 sp=0xc000187fd0 pc=0x4072f81
created by testing.(*T).Run
	/Users/bartle/src/go/src/testing/testing.go:1168 +0x2b3

goroutine 1 [chan receive]:
testing.(*T).Run(0xc000106600, 0x43e1e60, 0x9, 0x4401840, 0x4090926)
	/Users/bartle/src/go/src/testing/testing.go:1169 +0x2da
testing.runTests.func1(0xc000106480)
	/Users/bartle/src/go/src/testing/testing.go:1439 +0x78
testing.tRunner(0xc000106480, 0xc000185de0)
	/Users/bartle/src/go/src/testing/testing.go:1123 +0xef
testing.runTests(0xc00012ca00, 0x46d30c0, 0x1, 0x1, 0xbfdb9ed7f33c93c0, 0x8bb2ddb255, 0x46f5540, 0x4011950)
	/Users/bartle/src/go/src/testing/testing.go:1437 +0x2fe
testing.(*M).Run(0xc0001ac180, 0x0)
	/Users/bartle/src/go/src/testing/testing.go:1345 +0x1eb
main.main()
	_testmain.go:47 +0x138

goroutine 20 [chan receive]:
github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).registerForInterrupts(0xc00024c8c0, 0xc000102300)
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:223 +0xce
created by github.com/onsi/ginkgo/internal/specrunner.(*SpecRunner).Run
	/Users/bartle/go/src/github.com/onsi/ginkgo/internal/specrunner/spec_runner.go:60 +0x86

goroutine 22 [syscall]:
os/signal.signal_recv(0x0)
	/Users/bartle/src/go/src/runtime/sigqueue.go:144 +0x9d
os/signal.loop()
	/Users/bartle/src/go/src/os/signal/signal_unix.go:23 +0x25
created by os/signal.Notify.func1.1
	/Users/bartle/src/go/src/os/signal/signal.go:150 +0x45
FAIL	github.com/bsm/go-sparkey	0.274s
FAIL

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Oct 19, 2020

@bartle-stripe I don't think that would be related, no.

Loading

@bartle-stripe
Copy link

@bartle-stripe bartle-stripe commented Oct 19, 2020

Hm, so git bisect is implicating 9698372 and I can repro this pretty readily.

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Oct 20, 2020

@bartle-stripe Interesting. Can you open a new issue with all the details?

Loading

@cosnicolaou
Copy link
Contributor

@cosnicolaou cosnicolaou commented Oct 20, 2020

I know this is ugly, but would it make sense to add an annotation, eg, :opaque, to use for marking such opaque pointers and having the reflect package understand it - i.e. a pointer that's ignored by gc, but used by reflect for direct pointer comparison? This would allow the opaque pointer to be wrapped by a go type and a finalizer can be used to free the underlying type in C or whatever other language is being used. I guess it would require an extra field in reflect.Value which is an overhead.

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Oct 22, 2020

A complication in the runtime. pollDesc is marked as go:notinheap. It has a field, wt, which is a timer. That has a field arg which is an interface{}. Then at netpoll.go:290, we do (where pd is a *pollDesc):

pd.wt.arg = pd

That's writing a pointer to a go:notinheap type into an interface{}. Normally, that's fine, as we're just writing a pointer to the data field of an interface{}. But when I try to treat pointers to go:notinheap types as scalars, I can't store that scalar in the interface data field directly. I need to allocate some storage so I can store it indirectly. Allocation here is bad. The malloc call itself works, but then we're writing a pointer to that allocation to a location which isn't in the heap and won't get scanned.

Not sure why the prohibition on implicit allocation in the runtime didn't catch this.

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Oct 22, 2020

This kind of implicit allocation isn't caught by the runtime prohibition. I added a check to see what happens, and it seems to trigger quite a bit, all on conversions to the argument of panic. Maybe we should make those explicit somehow? Maybe later.

Loading

@josharian
Copy link
Contributor

@josharian josharian commented Oct 22, 2020

Assuming they're mostly a few common data types (ints, strings, etc), seems like with some helpers this would go pretty fast. And if it'd catch bugs, seems worthwhile. Alternatively, we could teach the compiler (and runtime backtracing) to use specialized panic handlers for common panic arg types, thereby avoiding the code weight associated with allocations in everyone's code.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 22, 2020

Change https://golang.org/cl/264480 mentions this issue: cmd/compile, runtime: store pointers to go:notinheap types indirectly

Loading

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Oct 23, 2020

@gopherbot Please open a backport issue for 1.15.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 23, 2020

Backport issue(s) opened: #42169 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 27, 2020

Change https://golang.org/cl/265757 mentions this issue: cmd/compile: print pointers to go:notinheap types without converting to unsafe.Pointer

Loading

@gopherbot gopherbot closed this in 009d714 Oct 27, 2020
gopherbot pushed a commit that referenced this issue Oct 27, 2020
…to unsafe.Pointer

Pretty minor concern, but after auditing the compiler/runtime for
conversions from pointers to go:notinheap types to unsafe.Pointer,
this is the only remaining one I found.

Update #42076

Change-Id: I81d5b893c9ada2fc19a51c2559262f2e9ff71c35
Reviewed-on: https://go-review.googlesource.com/c/go/+/265757
Trust: Keith Randall <khr@golang.org>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 9, 2021

Change https://golang.org/cl/308970 mentions this issue: reflect: make go:notinheap pointers indirect in New

Loading

gopherbot pushed a commit that referenced this issue Apr 9, 2021
For #42076
Fixes #45451

Change-Id: I69646226d3480d5403205412ddd13c0cfc2c8a53
Reviewed-on: https://go-review.googlesource.com/c/go/+/308970
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 9, 2021

Change https://golang.org/cl/340950 mentions this issue: test: add test case for CL 340609

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 10, 2021

Change https://golang.org/cl/340609 mentions this issue: compiler: store pointers to go:notinheap types indirectly

Loading

gopherbot pushed a commit to golang/gofrontend that referenced this issue Aug 12, 2021
This is the gofrontend version of https://golang.org/cl/264480.

For golang/go#42076

Change-Id: Ic8949fe5f052438a2858aae938eb9941dbd7c27c
Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/340609
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
gopherbot pushed a commit that referenced this issue Aug 13, 2021
The first version of CL 340609 for gofrontend passed all existing tests,
but not this one.

For #42076

Change-Id: I6491e2f186091bdae140b7f7befa511806a6478a
Reviewed-on: https://go-review.googlesource.com/c/go/+/340950
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
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
6 participants