retrieveUnexportedField violates Go 1.14's checkptr validation, fails under -race #167

bradfitz · 2019-10-23T22:14:11Z

With Go tip (to become Go 1.14) and its new checkptr mode (enabled by default in -race mode on anything but Windows for now):

=== CONT  TestDiff/EqualMethod/StructE1
panic: runtime error: unsafe pointer arithmetic [recovered]
        panic: runtime error: unsafe pointer arithmetic [recovered]
        panic: runtime error: unsafe pointer arithmetic

goroutine 142 [running]:
testing.tRunner.func1(0xc000238d00)
        /home/bradfitz/go/src/testing/testing.go:881 +0x69f
panic(0x6f3c00, 0xc00000f5e0)
        /home/bradfitz/go/src/runtime/panic.go:679 +0x1b2
github.com/google/go-cmp/cmp_test.TestDiff.func1.1.1(0xc000097eb8)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare_test.go:70 +0xfe
panic(0x6f3c00, 0xc00000f5e0)
        /home/bradfitz/go/src/runtime/panic.go:679 +0x1b2
github.com/google/go-cmp/cmp.retrieveUnexportedField(0x6e33c0, 0xc000128ad8, 0x199, 0x6ad073, 0xd, 0x6b70c3, 0x31, 0x778440, 0x6d34e0, 0x0, ...)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/export_unsafe.go:22 +0x7d
github.com/google/go-cmp/cmp.StructField.Values(0xc0002bd300, 0x8, 0x42c5e0, 0x0, 0x0, 0xc00026f6c0, 0x203000)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/path.go:190 +0x187
github.com/google/go-cmp/cmp.(*valueNode).PushStep(0xc0000a8420, 0x775540, 0xc0002bd300, 0x45bc9a)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/report_value.go:54 +0x5d
github.com/google/go-cmp/cmp.(*defaultReporter).PushStep(0xc00027d680, 0x775540, 0xc0002bd300)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/report.go:24 +0x6e
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0002be900, 0x775540, 0xc0002bd300)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:214 +0x22d
github.com/google/go-cmp/cmp.(*state).compareStruct(0xc0002be900, 0x778440, 0x6e33c0, 0x6e33c0, 0xc000128ad8, 0x199, 0x6e33c0, 0xc000128ae0, 0x199)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:383 +0x548
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0002be900, 0x775480, 0xc000012a00)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:252 +0x1a58
github.com/google/go-cmp/cmp.(*state).comparePtr(0xc0002be900, 0x778440, 0x6d8140, 0x6d8140, 0xc000128ad8, 0x16, 0x6d8140, 0xc000128ae0, 0x16)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:513 +0x526
github.com/google/go-cmp/cmp.(*state).compareAny(0xc0002be900, 0x775280, 0xc0000129c0)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:258 +0x18e6
github.com/google/go-cmp/cmp.Equal(0x6d8140, 0xc000128ad8, 0x6d8140, 0xc000128ae0, 0xc00000f580, 0x2, 0x2, 0x580c98)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:107 +0x406
github.com/google/go-cmp/cmp.Diff(0x6d8140, 0xc000128ad8, 0x6d8140, 0xc000128ae0, 0xc00012ab70, 0x1, 0x1, 0xc000106000, 0xc000106070)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare.go:127 +0x1ce
github.com/google/go-cmp/cmp_test.TestDiff.func1.1(0xc000241eb8, 0xc00023c600, 0xc0002f7ec8)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare_test.go:74 +0x138
github.com/google/go-cmp/cmp_test.TestDiff.func1(0xc000238d00)
        /home/bradfitz/pkg/mod/github.com/google/[email protected]/cmp/compare_test.go:75 +0x99
testing.tRunner(0xc000238d00, 0xc0001c1bd0)
        /home/bradfitz/go/src/testing/testing.go:916 +0x19a
created by testing.(*T).Run
        /home/bradfitz/go/src/testing/testing.go:967 +0x652
FAIL    github.com/google/go-cmp/cmp    0.116s

The text was updated successfully, but these errors were encountered:

bradfitz · 2019-10-23T22:14:22Z

/cc @mdempsky

dsnet · 2019-10-23T22:16:20Z

The logic for this is here:

go-cmp/cmp/export_unsafe.go

Line 22 in b1c9c48

return reflect.NewAt(f.Type, unsafe.Pointer(v.UnsafeAddr()+f.Offset)).Elem()

The ability to compare unexported fields is critical to many tests. Matt, you got any ideas for how to more safely retrieve an unexported field?

mdempsky · 2019-10-23T22:41:26Z

You have to convert UnsafeAddr to unsafe.Pointer before converting back to uintptr for pointer arithmetic.

I admit that's annoying, but it's what the unsafe rules strictly require (at least my reading of them).

(Edit: Explained further below: #167 (comment))

dsnet · 2019-10-24T18:38:02Z

IIUC, you are suggesting this?

return reflect.NewAt(f.Type, unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr()))+f.Offset)).Elem()

bradfitz · 2019-10-24T18:42:12Z

@dsnet, try it and see? Does that make checkptr happy?

mdempsky · 2019-10-24T19:20:14Z

@dsnet Yeah, that should work.

mdempsky · 2019-10-24T20:39:25Z

I find the situation funny, and I'm glad others share my compiler nerd sense of humor. :)

I thought I'd elaborate a bit though to make sure folks understand this isn't a strictly theoretical/pedantic issue.

When compiling an expression with multiple function calls like f() + g(), we need to save f()'s return value on the stack while evaluating g(). We normally save it as the same type as what f() returns.

In the case of an expression like unsafe.Pointer(v.UnsafeAddr() + g()), we would save v.UnsafeAddr()'s return value on the stack as a uintptr variable. So if g() triggers the garbage collector, the runtime will scan the stack; but because the address is saved as a uintptr, it will be skipped over. If this is the only remaining live reference to the variable, it could be garbage collected prematurely.

The compiler specially recognizes calls of the form unsafe.Pointer(f()) (where f() has type uintptr and is immediately converted to unsafe.Pointer), and it saves them as an unsafe.Pointer variable instead. That guarantees expressions like unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr())) + g()) are safe regardless of what g() does.

In go-cmp's code above, g() is actually just f.Offset, where this is a simple field access of a local variable. I can't off-hand think of any ways this code today could trigger the garbage collector, but in general, that's a bit iffy to rely on. E.g., if f was a pointer (so f.Offset is shorthand for (*f).Offset), the pointer dereference would involve instrumented calls into the runtime for -race and -msan modes; and for fuzzing, we're adding other forms of instrumentation around certain expression evaluations, which in general can also require runtime calls.

(Hopefully this was worthwhile to write up and share.)

dsnet · 2019-10-24T20:42:13Z

Personally, I think the oddity is more so due to the reflect package lacking type safety. I feel like golang/go#24654 or golang/go#26070 would have helped the situation.

Had the reflect package natively returned an unsafe.Pointer from UnsafeAddr, the first cast from a uintptr to an unsafe.Pointer would not have been necessary in the first place. Given an unsafe.Pointer on hand, the user would have been forced to do the right thing, by converting it to an uintptr to add the uintptr offset, and then convert back to unsafe.Pointer.

mdempsky · 2019-10-24T21:17:54Z

@dsnet Yeah, I think the design of "safe" mode has had unfortunate long-term consequences for packages reflect and unsafe.

…lidation Fixes google#167

Fixes #167

To fix our race build. Updates google/go-cmp#167 Change-Id: I8d57cb4b7e0244f7d0944717b2e472afdfec218d Reviewed-on: https://go-review.googlesource.com/c/build/+/203880 Run-TryBot: Brad Fitzpatrick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>

CI was failing under -race on go 1.14 due to go-cmp. This is now fixed for google/go-cmp#167 in go-cmp master.

To fix our -race test under Go 1.14. See google/go-cmp#167

To fix our race build. Updates google/go-cmp#167 Change-Id: I8d57cb4b7e0244f7d0944717b2e472afdfec218d Reviewed-on: https://go-review.googlesource.com/c/build/+/203880 Run-TryBot: Brad Fitzpatrick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>

Upgrade google/go-cmp to version 0.4.0. This release contains fixes for unsafe pointer conversions which would trigger a test failure under the race detector for Go 1.14 and later. See google/go-cmp#167 Signed-off-by: Dave Cheney <[email protected]>

Mostly to fix google/go-cmp#167, but we also pinned at some random commit, rather than at a release version.

* update go-cmpt to work with 1.14. Mostly to fix google/go-cmp#167, but we also pinned at some random commit, rather than at a release version. * add new pkg

* update go-cmpt to work with 1.14. Mostly to fix google/go-cmp#167, but we also pinned at some random commit, rather than at a release version. * add new pkg * Make sure we use same versions in pkg and serving

* update go-cmpt to work with 1.14. Mostly to fix google/go-cmp#167, but we also pinned at some random commit, rather than at a release version. * add new pkg * Make sure we use same versions in pkg and serving * Fix the defaulting for the leader election cm This was missing, but now we can provide the default values. * Fix the defaulting for the leader election cm This was missing, but now we can provide the default values.

When running with -race, checkptr complained. The reason (and the solution) is decribed in google/go-cmp#167 (comment)

The unsafe.Pointer safety rules have three separate rules allowing conversion of uintptr to unsafe.Pointer: (3) allows converting unsafe.Pointer to uintptr, performing arithmetic, and converting back; (5) allows converting the results from calling reflect.Value.{Pointer,UnsafeAddr} to unsafe.Pointer; and (6) allows converting the Data field of reflect.{Slice,String}Header to unsafe.Pointer. Notably, these are three separate rules, and they're not allowed to be arbitrarily mixed. For example, this is not allowed: unsafe.Pointer(v.UnsafeAddr() + x) // BAD It needs to instead be written as: unsafe.Pointer(uintptr(unsafe.Pointer(v.UnsafeAddr())) + x) (For further explanation on why this is necessary, see google/go-cmp#167 (comment).) This CL brings cmd/vet's unsafeptr check inline with the unsafe.Pointer rules and cmd/compile's implementation thereof. Change-Id: I1844e0f71dcc8fb7aafacc144b86cc80a2b83b42 Reviewed-on: https://go-review.googlesource.com/c/tools/+/248191 Run-TryBot: Matthew Dempsky <[email protected]> gopls-CI: kokoro <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Trust: Matthew Dempsky <[email protected]>

According to google/go-cmp#167 go-cmp triggers go's "-race" validation. It's supposedly fixed in 0.4.0 but 0.5.2 is the latest so let's try that.

bradfitz added a commit to bradfitz/go-cmp that referenced this issue Oct 28, 2019

cmp: make retrieveUnexportedField pass Go 1.14's upcoming checkptr va…

000c372

…lidation Fixes google#167

bradfitz mentioned this issue Oct 28, 2019

cmp: make retrieveUnexportedField pass Go 1.14's upcoming checkptr validation #169

Merged

dsnet closed this as completed in #169 Oct 28, 2019

dsnet pushed a commit that referenced this issue Oct 28, 2019

Make retrieveUnexportedField pass Go 1.14's checkptr validation (#169)

481baca

Fixes #167

didrocks added a commit to ubuntu/zsys that referenced this issue Oct 29, 2019

Switch to master go-cmp, with a fix for reflect on go 1.14

bebded5

CI was failing under -race on go 1.14 due to go-cmp. This is now fixed for google/go-cmp#167 in go-cmp master.

anthonyfok added a commit to anthonyfok/hugo that referenced this issue Nov 1, 2019

Update past go-cmp's checkptr fix

64eb32f

To fix our -race test under Go 1.14. See google/go-cmp#167

anthonyfok mentioned this issue Nov 1, 2019

Update past go-cmp's checkptr fix gohugoio/hugo#6470

Merged

anthonyfok added a commit to gohugoio/hugo that referenced this issue Nov 1, 2019

Update past go-cmp's checkptr fix

c3d433a

To fix our -race test under Go 1.14. See google/go-cmp#167

rhcarvalho mentioned this issue Dec 20, 2019

Bump github.com/google/go-cmp to 0.4.0 (required for Go 1.14) getsentry/sentry-go#129

Closed

wfernandes mentioned this issue Jan 24, 2020

FLBPluginSetContext violates Go 1.14's checkptr validation, fails under -race fluent/fluent-bit-go#26

Closed

davecheney mentioned this issue Jan 27, 2020

go.mod: upgrade google/go-cmp projectcontour/contour#2161

Merged

tstromberg mentioned this issue Feb 18, 2020

go.mod: upgrade google/go-cmp to v0.4.0 GoogleContainerTools/skaffold#3698

Merged

vangent mentioned this issue Feb 26, 2020

all: drop go 1.11 support and make go 1.14 the current version. google/go-cloud#2752

Merged

vangent added a commit to vangent/go-cloud that referenced this issue Feb 26, 2020

update go-cmp due to google/go-cmp#167

4158f49

guggero mentioned this issue Feb 26, 2020

build: update travis to build against go 1.14 lightningnetwork/lnd#4033

Merged

geoah mentioned this issue Feb 26, 2020

Fix go 1.14's checkptr validation spaolacci/murmur3#31

Closed

geoah mentioned this issue Feb 26, 2020

Flaky "unsafe pointer conversion"-panics with -race and go1.14rc1 spaolacci/murmur3#29

Open

vagababov added a commit to vagababov/pkg that referenced this issue Mar 20, 2020

update go-cmpt to work with 1.14.

7c0f63a

Mostly to fix google/go-cmp#167, but we also pinned at some random commit, rather than at a release version.

vagababov mentioned this issue Mar 20, 2020

update go-cmpt to work with 1.14. knative/pkg#1167

Merged

mdempsky mentioned this issue May 1, 2020

Go: NewTensor & Value performance improvement tensorflow/tensorflow#36578

Merged

lporcheron pushed a commit to bleemeo/glouton that referenced this issue Jul 6, 2020

Fix running with -race

aa8cf53

When running with -race, checkptr complained. The reason (and the solution) is decribed in google/go-cmp#167 (comment)

ghost pushed a commit to purelb/purelb that referenced this issue Dec 21, 2021

Upgrade our "go-cmd" dependency

6eb80cc

According to google/go-cmp#167 go-cmp triggers go's "-race" validation. It's supposedly fixed in 0.4.0 but 0.5.2 is the latest so let's try that.

smyrman mentioned this issue Jan 27, 2023

fatal error: checkptr: pointer arithmetic computed bad pointer value (test -race) #321

Closed

apparentlymart mentioned this issue Mar 12, 2024

Standardize on only two value dumping/diffing libraries hashicorp/hcl#669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retrieveUnexportedField violates Go 1.14's checkptr validation, fails under -race #167

retrieveUnexportedField violates Go 1.14's checkptr validation, fails under -race #167

bradfitz commented Oct 23, 2019

bradfitz commented Oct 23, 2019

dsnet commented Oct 23, 2019

mdempsky commented Oct 23, 2019 •

edited

Loading

dsnet commented Oct 24, 2019

bradfitz commented Oct 24, 2019

mdempsky commented Oct 24, 2019

mdempsky commented Oct 24, 2019 •

edited

Loading

dsnet commented Oct 24, 2019 •

edited

Loading

mdempsky commented Oct 24, 2019

retrieveUnexportedField violates Go 1.14's checkptr validation, fails under -race #167

retrieveUnexportedField violates Go 1.14's checkptr validation, fails under -race #167

Comments

bradfitz commented Oct 23, 2019

bradfitz commented Oct 23, 2019

dsnet commented Oct 23, 2019

mdempsky commented Oct 23, 2019 • edited Loading

dsnet commented Oct 24, 2019

bradfitz commented Oct 24, 2019

mdempsky commented Oct 24, 2019

mdempsky commented Oct 24, 2019 • edited Loading

dsnet commented Oct 24, 2019 • edited Loading

mdempsky commented Oct 24, 2019

mdempsky commented Oct 23, 2019 •

edited

Loading

mdempsky commented Oct 24, 2019 •

edited

Loading

dsnet commented Oct 24, 2019 •

edited

Loading