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/vet: warn about variables/values of type reflect.{Slice,String}Header #40701

Closed
mdempsky opened this issue Aug 11, 2020 · 21 comments
Closed

cmd/vet: warn about variables/values of type reflect.{Slice,String}Header #40701

mdempsky opened this issue Aug 11, 2020 · 21 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 11, 2020

Here are three ways I commonly see developers misuse reflect.SliceHeader / reflect.StringHeader:

package p

import (
	"reflect"
	"unsafe"
)

// Explicitly allocating a variable of type reflect.SliceHeader.
func a(p *byte, n int) []byte {
	var sh reflect.SliceHeader
	sh.Data = uintptr(unsafe.Pointer(p))
	sh.Len = n
	sh.Cap = n
	return *(*[]byte)(unsafe.Pointer(&sh))
}

// Implicitly allocating a variable of type reflect.SliceHeader.
func b(p *byte, n int) []byte {
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
		Data: uintptr(unsafe.Pointer(p)),
		Len: n,
		Cap: n,
	}))
}

// Use reflect.SliceHeader as a composite literal value.
func c(p *byte, n int) []byte {
	var res []byte
	*(*reflect.SliceHeader)(unsafe.Pointer(&res)) = reflect.SliceHeader{
		Data: uintptr(unsafe.Pointer(p)),
		Len: n,
		Cap: n,
	}
	return res
}

All three of these can lead to memory corruption, as escape analysis analyzes these as "p does not escape" (rather than "p leaks to result"), but none of them are currently diagnosed by either cmd/vet or checkptr.

I propose cmd/vet should look for variables and expressions of type reflect.SliceHeader or reflect.StringHeader (as opposed to *reflect.SliceHeader or *reflect.StringHeader) and warn about them. There should be no false positives for this, and it's consistent with the unsafeptr check that cmd/vet already has.

--

An alternative approach would be to outright disallow reflect.SliceHeader and reflect.StringHeader to be used except as pointed-to types. (Notably, this would be similar to the //go:cgo_incomplete directive suggested in #40507 to prevent misuse of incomplete C struct types.)

I think this would be consistent with Go 1 compat. Go 1 compat is about guaranteeing that programs continue to build and run correctly in the future. I think if we're okay with these programs not running correctly today, we should be okay with them not building either, and I expect users would prefer compilation errors instead of silent memory corruption at runtime.

But perhaps a more friendly approach would be a cmd/vet warning in Go 1.N, and then make it into a compiler error in Go 1.N+1 (and maybe gated by Go language version specified in go.mod).

@gopherbot gopherbot added this to the Proposal milestone Aug 11, 2020
@mdempsky mdempsky added this to Incoming in Proposals Aug 11, 2020
@martisch
Copy link
Contributor

@martisch martisch commented Aug 12, 2020

Unfixed example in the wild: alecthomas/unsafeslice#4
Corresponding static check proposal: dominikh/go-tools#782

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 12, 2020

Oh yeah, it's super easy to find instances of these mistakes. I found these in under 5 minutes with GitHub search (which doesn't even support regexps), and only made it to page 5 of search results by time I stopped:

20 examples of `&reflect.SliceHeader{`
@martisch
Copy link
Contributor

@martisch martisch commented Aug 12, 2020

Making it a compiler error will be a hard stop to this but creates friction when others use packages as dependencies that are effected by this. On the other side these code instances will lead to subtle memory corruption bugs that need the right circumstances come together and may not be caught by tests just checking for correct output values of the functioncs involved. They can be hard to diagnose which could justify this.

@rsc
Copy link
Contributor

@rsc rsc commented Aug 12, 2020

@martisch, the compiler error gated on go version in the (dependency's) go.mod file solves the friction problem. If it's in your dependency and your dependency says it wants the old version of Go, the code still compiles.

Definitely sounds like we should flag it in cmd/vet. I'm not really sure we need to move on to the compiler error. It would be a very specific thing for a compiler to check. (We don't, for example, flag bad regexp arguments to regexp.MustCompile in the compiler, even though we could plausibly do that in vet.)

I suggest we do just the vet check and not plan on the compiler change.

@rsc rsc moved this from Incoming to Active in Proposals Aug 12, 2020
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 12, 2020

We don't, for example, flag bad regexp arguments to regexp.MustCompile in the compiler, even though we could plausibly do that in vet.

I think there's a difference that compilers already have to specially handle reflect.SliceHeader and reflect.StringHeader because of the unsafe.Pointer safety rules. E.g., escape analysis, walk, and SSA lowering currently have code specific to using these types.

There's also the difference that misusing regexp.MustCompile still has defined semantics: it panics in a very loud and obvious way, which is clearly user error.

However, misusing reflect.SliceHeader / reflect.StringHeader means silent memory corruption. Not only are these failures harder to track down, I wouldn't be surprised if they're responsible for some of the issues being reported against the Go runtime. E.g., #40397 was reported as a runtime.selectgo crash, which I spent a while looking into; but it's plausible (albeit not yet confirmed) it was actually due to misuse of reflect.SliceHeader in that package: https://github.com/ethereum/go-ethereum/pull/21372/files.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248192 mentions this issue: x/tools/go/analysis/passes/unsafeptr: report Header misuse

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 13, 2020

I tried running cmd/vet patched with CL 248192 on the 20 packages I identified above. I wasn't able to immediately compile 6 of them due to missing C dependencies, but vet identifies reflect.SliceHeader misuse in the other 14:

go vet warnings (annotated with source line)
# github.com/asukakenji/go-benchmarks/common/reinterpret
./slice.go:17:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:31:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:45:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:59:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:73:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:87:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:109:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:123:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./slice.go:137:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
# github.com/kapitan-k/goutilities/unsafe
./unsafe.go:10:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:19:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:38:37: possible misuse of reflect.SliceHeader
	return *(*[]uint64)(unsafe.Pointer(&reflect.SliceHeader{
	                                   ^
./unsafe.go:47:36: possible misuse of reflect.SliceHeader
	return *(*[]int64)(unsafe.Pointer(&reflect.SliceHeader{
	                                  ^
./unsafe.go:64:37: possible misuse of reflect.SliceHeader
	return *(*[]uint64)(unsafe.Pointer(&reflect.SliceHeader{
	                                   ^
./unsafe.go:73:36: possible misuse of reflect.SliceHeader
	return *(*[]int64)(unsafe.Pointer(&reflect.SliceHeader{
	                                  ^
./unsafe.go:83:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:93:37: possible misuse of reflect.SliceHeader
	return *(*[]uint64)(unsafe.Pointer(&reflect.SliceHeader{
	                                   ^
./unsafe.go:103:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:113:36: possible misuse of reflect.SliceHeader
	return *(*[]int64)(unsafe.Pointer(&reflect.SliceHeader{
	                                  ^
./unsafe.go:123:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:133:37: possible misuse of reflect.SliceHeader
	return *(*[]uint32)(unsafe.Pointer(&reflect.SliceHeader{
	                                   ^
# github.com/gopher-os/gopher-os/src/gopheros/kernel
./mem_util.go:18:38: possible misuse of reflect.SliceHeader
	target := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                    ^
./mem_util.go:37:40: possible misuse of reflect.SliceHeader
	srcSlice := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
./mem_util.go:42:40: possible misuse of reflect.SliceHeader
	dstSlice := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
# github.com/sriharikapu/goos-e/src/goose/kernal
./mem_util.go:18:38: possible misuse of reflect.SliceHeader
	target := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                    ^
./mem_util.go:37:40: possible misuse of reflect.SliceHeader
	srcSlice := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
./mem_util.go:42:40: possible misuse of reflect.SliceHeader
	dstSlice := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
# github.com/ShoichiroKitano/kagemusha
./kagemusha.go:30:40: possible misuse of reflect.SliceHeader
	original := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
./kagemusha.go:36:36: possible misuse of reflect.SliceHeader
	page := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                  ^
./kagemusha.go:62:40: possible misuse of reflect.SliceHeader
	original := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                      ^
./kagemusha.go:70:36: possible misuse of reflect.SliceHeader
	page := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                  ^
# github.com/lukechampine/few
./few_test.go:15:34: reflect.SliceHeader composite literal uses unkeyed fields
		_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(&x)), int(unsafe.Sizeof(x)), int(unsafe.Sizeof(x))}))
		                               ^
./few_test.go:24:34: reflect.SliceHeader composite literal uses unkeyed fields
		_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(&x)), int(unsafe.Sizeof(x)), int(unsafe.Sizeof(x))}))
		                               ^
./few_test.go:37:35: reflect.SliceHeader composite literal uses unkeyed fields
			_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(x[j])), int(unsafe.Sizeof(x[j])), int(unsafe.Sizeof(x[j]))}))
			                               ^
./few_test.go:15:33: possible misuse of reflect.SliceHeader
		_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(&x)), int(unsafe.Sizeof(x)), int(unsafe.Sizeof(x))}))
		                              ^
./few_test.go:24:33: possible misuse of reflect.SliceHeader
		_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(&x)), int(unsafe.Sizeof(x)), int(unsafe.Sizeof(x))}))
		                              ^
./few_test.go:37:34: possible misuse of reflect.SliceHeader
			_ = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{uintptr(unsafe.Pointer(x[j])), int(unsafe.Sizeof(x[j])), int(unsafe.Sizeof(x[j]))}))
			                              ^
# go.etcd.io/bbolt
./freelist.go:107:35: possible misuse of reflect.SliceHeader
	dst := *(*[]pgid)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./freelist.go:297:36: possible misuse of reflect.SliceHeader
		ids := *(*[]pgid)(unsafe.Pointer(&reflect.SliceHeader{
		                                 ^
./node.go:234:34: possible misuse of reflect.SliceHeader
		b := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
		                               ^
./page.go:68:46: possible misuse of reflect.SliceHeader
	return *(*[]leafPageElement)(unsafe.Pointer(&reflect.SliceHeader{
	                                            ^
./page.go:86:48: possible misuse of reflect.SliceHeader
	return *(*[]branchPageElement)(unsafe.Pointer(&reflect.SliceHeader{
	                                              ^
./page.go:95:35: possible misuse of reflect.SliceHeader
	buf := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./page.go:118:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./page.go:135:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./page.go:144:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./tx.go:540:37: possible misuse of reflect.SliceHeader
			buf := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
			                                 ^
./tx.go:579:36: possible misuse of reflect.SliceHeader
		buf := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
		                                 ^
# gioui.org/internal/unsafe
./unsafe.go:15:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:32:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&sh))
	                                 ^
# github.com/ldeng7/go-x/monkey
./monkey.go:31:40: reflect.SliceHeader composite literal uses unkeyed fields
	tarCode := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{patch.tar, l, l}))
	                                      ^
./monkey_unix.go:16:35: reflect.SliceHeader composite literal uses unkeyed fields
	sl := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{pb, pl, pl}))
	                                 ^
./monkey_unix.go:22:35: reflect.SliceHeader composite literal uses unkeyed fields
	sl := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{p, iLen, iLen}))
	                                 ^
./monkey.go:27:34: possible misuse of unsafe.Pointer
	code := getJumpCode(*(*uintptr)(unsafe.Pointer(rp)))
	                                ^
./monkey.go:31:39: possible misuse of reflect.SliceHeader
	tarCode := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{patch.tar, l, l}))
	                                     ^
./monkey_unix.go:16:34: possible misuse of reflect.SliceHeader
	sl := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{pb, pl, pl}))
	                                ^
./monkey_unix.go:22:34: possible misuse of reflect.SliceHeader
	sl := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{p, iLen, iLen}))
	                                ^
# github.com/PieterD/glimmer/internal/convc
./convert.go:41:35: possible misuse of reflect.SliceHeader
	return *(*string)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./convert.go:51:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./convert.go:95:42: possible misuse of reflect.SliceHeader
	pointers := *(*[]*uint8)(unsafe.Pointer(&reflect.SliceHeader{
	                                        ^
# gioui.org/internal/unsafe
./unsafe.go:15:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./unsafe.go:32:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&sh))
	                                 ^
# github.com/timob/sindex
./list_test.go:71:1: ExampleIterator refers to unknown identifier: Iterator
func ExampleIterator() {
^
./list_test.go:139:1: ExampleStack refers to unknown identifier: Stack
func ExampleStack() {
^
./unsafe.go:31:35: possible misuse of reflect.SliceHeader
	dst := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{Data: data, Len: lenBytes, Cap: lenBytes}))
	                                 ^
./unsafe.go:34:35: possible misuse of reflect.SliceHeader
	src := *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{Data: data, Len: lenBytes, Cap: lenBytes}))
	                                 ^
# github.com/mcronce/gitcrypt/pkg/gitcrypt
./util.go:20:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
./util.go:29:35: possible misuse of reflect.SliceHeader
	return *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{
	                                 ^
# github.com/lentus/wotscoin/lib/others/qdb
./membind.go:34:35: possible misuse of reflect.SliceHeader
		res = *(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{Data:uintptr(v.data), Len:int(v.datlen), Cap:int(v.datlen)}))
		                                ^
./membind.go:65:36: possible misuse of reflect.SliceHeader
		f.Read(*(*[]byte)(unsafe.Pointer(&reflect.SliceHeader{Data:uintptr(v.data), Len:int(v.datlen), Cap:int(v.datlen)})))
		                                 ^
@rsc
Copy link
Contributor

@rsc rsc commented Aug 26, 2020

@mdempsky, I see your point about silent memory corruption being the province of the compiler.
But looking at the examples that you found and assuming the compiler gets involved, I wonder if maybe the answer is for the compiler to treat the uintptr field of reflect.SliceHeader and reflect.StringHeader as a pointer as far as type bitmaps. Then this kind of allocation becomes safe and the code becomes correct, instead of forcing everyone to rewrite the code.

There's at least a few precedents for this in the way we handle "temporary" uintptrs as unsafe.Pointers in the unsafe.Add-equivalent code today, and also in the way we handle uintptrs in system calls.

We would also want to treat assignment of a plain uintptr to that field as a conversion to unsafe.Pointer in the current unsafe.Pointer vet check (the one that flags things like p := (*T)(unsafe.Pointer(uintptrVariable))).

If there's a way to make existing code correct instead of flagging it being wrong, that might be preferable.

What do you think?

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Aug 27, 2020

As I understand your counter-proposal, it's to change reflect.{Slice,String}Header such that (1) the Data fields actually denote pointer-holding memory (while remaining declared as type uintptr), and (2) loads/stores of these fields are effectively implicit conversions to/from uintptr (with semantics identical to an explicitly written conversion).

I agree that would better match typical user expectations and make valid a lot of currently invalid user code, which would be great. I have two minor concerns though: one about backwards compatibility, and another about corner cases of language semantics.

--

1. Backwards compatibility. This has the risk of breaking code like:

var h = reflect.SliceHeader{Data: 42}

This use of reflect.SliceHeader is silly and ill-advised, but currently allowed. However, under this proposal, it becomes invalid just like how

var p = unsafe.Pointer(uintptr(42))

has been invalid since Go 1.3.

Now, the documentation for reflect.{Slice,String}Header already warns: "It cannot be used safely or portably and its representation may change in a later release." Also, checkptr would be able to catch this new form of misuse, so the breakage would be obvious and actionable. So I would argue this is an acceptable risk, on par with Go 1.3 breaking p.

2. Language semantics. I'm concerned this muddies type identity for uintptr and implies consequences beyond those intended. E.g., &h.Data now semantically involves an implicit conversion from *unsafe.Pointer to *uintptr. Converting (*T)(&h) where T has the same underlying type as reflect.SliceHeader has similar consequences. There might be weird implications for reflection as well.

I don't immediately see any ways this is likely to bite real-world Go programs, but I'd want to think about this some more.

--

Counter-counter-proposal. If this counter-proposal (i.e., ratifying existing user practice) is a direction we're interested in going in, I'd suggest we consider a slight tweak to it: actually change Data's declared type from uintptr to unsafe.Pointer, and figure out what type-checking exceptions we need to make to continue accepting most (if not all) current Go programs.

As I pointed out above, we are allowed to change the types of these fields (even if it breaks currently valid programs). And if we're going to have to continue special casing loads/stores of the Data field anyway, I think it's preferable to do this once during type checking rather than scattered a bunch of places throughout the back end.

I wouldn't be surprised if simply allowing assignments of uintptr-typed values to Data and vice-versa (i.e., assignments of Data to uintptr-typed variables) was already enough for >99% of currently valid code. At least skimming through Google's internal Code Search for uses of reflect.{Slice,String}Header.Data, I don't see any code this would still break.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

@ianlancetaylor raised some discomfort with putting this knowledge in the compiler (as did @mdempsky).
Given that (1) these are used mainly for "interesting" conversions, and (2) we have active discussions on other issues about how to support those kinds of conversions directly, perhaps the right path forward is to finish figuring out what the conversions should look like and then do the vet check. The vet failures can be fixed by either writing the code to only use pointers to these structs or, in many cases, to use whatever the new conversions are and avoid the structs entirely.

We could still consider elevating the vet check to the compiler, under the rationale that the compiler can see that the program may cause memory corruption, but we could start with the vet check. There are of course plenty of other ways to cause memory corruption that the compiler does not reject.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Sep 2, 2020

perhaps the right path forward is to finish figuring out what the conversions should look like and then do the vet check.

If we commit to having some solution here for Go 1.16, I'm okay with waiting. I think it would be a disservice to Go users if we know they're routinely misusing reflect.SliceHeader and reflect.StringHeader, and that misuse is easily caught, but we don't help them with that because we're unsure on the best spelling.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 2, 2020

I think a vet check is fine, and that's what you asked for in this proposal. I support that.

I'm just not comfortable with a compiler change. It doesn't make sense to me. reflect.StringHeader is just a struct. I think that having the compiler reject a struct because it is routinely misused is confusing.

Separately, I think it's worth considering why people use reflect.SliceHeader and reflect.StringHeader, and then ensuring that there are alternate mechanisms for all of those uses. For example, using reflect.SliceHeader to change the type of a slice, or to convert a pointer to a slice, is, I believe, #19367. Are there any other uses of reflect.SliceHeader that can't be done in other ways? I can't think of any offhand.

The case of reflect.StringHeader is still open. As far as I know, given a string, there is no other way to get the pointer to the actual string data. And as far as I know, there is no way to turn a pointer and a length into a string. So it seems to me that we should have proposals for ways to do those actions without using reflect.StringHeader.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Sep 2, 2020

reflect.StringHeader is just a struct.

Generally, I agree with that. But it's a struct whose usage is very particular and specially enshrined in the unsafe.Pointer safety rules, which Go compilers have to implement. That makes it more than "just a struct" in my mind.

I think the "just a struct" view is what misleads many Go programmers to write code like I identified in this issue.

So it seems to me that we should have proposals for ways to do those actions without using reflect.StringHeader.

That's why my original proposal for #19367 included options for both constructing and destructing both strings and slices. But then based on push back, I narrowed that down to just constructing them; and then to only constructing slices.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 2, 2020

The special rules about StringHeader and SliceHeader are permission rules. You are permitted to do a special operation with those structs. They don't forbid other uses of those structs.

The use in the compiler is only for supporting -d=checkptr, which is a very useful checking operation but doesn't affect code generation.

I think I hear what you are saying, and I agree that there is a problem with these structs in practice, and I agree that a vet check is appropriate. I just don't see why prohibiting them in the compiler is right.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Sep 2, 2020

The use in the compiler is only for supporting -d=checkptr, which is a very useful checking operation but doesn't affect code generation.

No, it does affect code generation. Escape analysis has to know that pointers flow through assignments to Data and code generation has to use write barriers for it.

For example, compile this code with -m -S (i.e., without checkptr), and you'll see different escape analysis for the two functions and also very different assembly:

package p

import (
	"reflect"
	"unsafe"
)

func F(h *reflect.SliceHeader, p unsafe.Pointer) {
	h.Data = uintptr(p)
}

func G(h *mySliceHeader, p unsafe.Pointer) {
	h.Data = uintptr(p)
}

type mySliceHeader struct {
	Data uintptr
	Len, Cap int
}

// Assert that underlying struct types are identical.
var _ = reflect.SliceHeader(mySliceHeader{})
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 2, 2020

Ah, OK, thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 16, 2020

It sounds like there is consensus here that adding a vet check is OK.
It also sounds like there is no consensus about broader compiler changes.

Since the title of the issue is tracking the vet change, this (the vet change) seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Sep 16, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 23, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Sep 23, 2020
@rsc rsc modified the milestones: Proposal, Backlog Sep 23, 2020
@rsc rsc changed the title proposal: cmd/vet: warn about variables/values of type reflect.{Slice,String}Header cmd/vet: warn about variables/values of type reflect.{Slice,String}Header Sep 23, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Oct 16, 2020
This CL updates unsafeptr to report about *x and &x expressions where
the pointed-to variable has type reflect.SliceHeader or
reflect.StringHeader.

- Disallowing *x means that reflect.{Slice,String}Header.Data can only
  be accessed using field selection via a *reflect.{Slice,String}Header
  value.

- Disallowing &x means that a *reflect.{Slice,String}Header value can
  only be created by converting from an unsafe.Pointer.

Well, almost only. There are still tricks that can be played to
workaround both of these. For example, a pointer can be dereferenced
via reflection, or a user could write a conversion like:

    type T reflect.SliceHeader
    _ = (*reflect.SliceHeader)(&T{})

But presumably this at least raises the bar enough that someone is
likely to pause to figure out the correct way to use
reflect.{Slice,String}Header.

Notably, disallowing *x and &x does *not* emit warnings for code that
uses reflect.{Slice,String}Header purely as values. For example, the
tests in internal/unsafeheader. Such code is arguably still a
violation of the unsafe.Pointer safety rules ("reflect.SliceHeader and
reflect.StringHeader should be used [...] never as plain structs"),
but is benign.

Updates golang/go#40701.

Change-Id: Id21996bfee07acc0d927a525797dca344bc804d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248192
Reviewed-by: Michael Matloob <matloob@golang.org>
Trust: Matthew Dempsky <mdempsky@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Nov 7, 2020
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Nov 10, 2020

This was done in a78bdc4.

@mdempsky mdempsky closed this Nov 10, 2020
philhofer added a commit to tinylib/msgp that referenced this issue Nov 11, 2020
At some point the guidelines for unsafe[1] were changed
such that allocating a variable of type 'reflect.StringHeader'
or 'reflect.SliceHeader' is no longer permitted, and 'go vet'
will soon reject code that does so[2].

Change the implementation of UnsafeString to avoid using
an intermediate StringHeader altogether, and deprecate UnsafeBytes,
as the new guidelines seem to indicate it cannot be implemented
safely (escape analysis may not handle it correctly).

[1] https://golang.org/pkg/unsafe/
[2] golang/go#40701
philhofer added a commit to tinylib/msgp that referenced this issue Nov 11, 2020
At some point the guidelines for unsafe[1] were changed
such that allocating a variable of type 'reflect.StringHeader'
or 'reflect.SliceHeader' is no longer permitted, and 'go vet'
will soon reject code that does so[2].

Change the implementation of UnsafeString to avoid using
an intermediate StringHeader altogether, and deprecate UnsafeBytes,
as the new guidelines seem to indicate it cannot be implemented
safely (escape analysis may not handle it correctly).

[1] https://golang.org/pkg/unsafe/
[2] golang/go#40701
@rski
Copy link

@rski rski commented Nov 13, 2020

There should be no false positives for this

This check points out an issue in our mmap implementation. It's basically the same as the one in sys/unix, so this change is false positive-y on the golang/sys package too, this is the line: https://github.com/golang/sys/blob/0a15ea8d9b02651b828a1b41989a6af25c24cb64/unix/syscall_unix.go#L120. I think this shouldn't be a problem, since the memory pointed to is managed by the user code, not the go runtime.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Nov 13, 2020

There should be no false positives for this

This check points out an issue in our mmap implementation. It's basically the same as the one in sys/unix, so this change is false positive-y on the golang/sys package too, this is the line: https://github.com/golang/sys/blob/0a15ea8d9b02651b828a1b41989a6af25c24cb64/unix/syscall_unix.go#L120. I think this shouldn't be a problem, since the memory pointed to is managed by the user code, not the go runtime.

The warning is not about reflect.SliceHeader (the code doesn't use it anyway). The warning is about misuse of unsafe.Pointer.

go1.15.5 also warns about that.

jcburley added a commit to jcburley/joker that referenced this issue Dec 17, 2020
Doesn't pass tests because of the type-alias issue, which can be fixed at the test level (if not by actually supporting them in Joker).

But this addresses the failure to vet as a result of:

golang/go#40701
akalin added a commit to akalin/gopar that referenced this issue Feb 21, 2021
akalin added a commit to akalin/gopar that referenced this issue Feb 21, 2021
dvyukov added a commit to dvyukov/syzkaller that referenced this issue Feb 22, 2021
Pointed by golangci-lint.
For context see golang/go#40701
dvyukov added a commit to google/syzkaller that referenced this issue Feb 22, 2021
Pointed by golangci-lint.
For context see golang/go#40701
ncopa added a commit to ncopa/heplify-server that referenced this issue Jun 7, 2021
Fixes go vet errors:

    # github.com/sipcapture/heplify-server/rotator
    rotator/rotator.go:222:2: unreachable code
    # github.com/sipcapture/heplify-server/decoder
    decoder/decoder.go:299:35: possible misuse of reflect.SliceHeader

see golang/go#40701
OmidHekayati added a commit to SabzCity/libgo that referenced this issue Jun 10, 2021
- Fix bad usage of reflect package to convert unsafe!
- checked by `go build -gcflags=-m` show all `req does not escape`
- read more : golang/go#40701
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants