panic: runtime error: unsafe pointer conversion (Go 1.14)

[klauspost]

In Go 1.14 there is now checks of pointer conversions with the requirement that they should align to the "natural size".

It seems like they decided to make this enabled when -race is specified: golang/go#34964

This leads to crashes like this:

    --- FAIL: TestNewBoltStore/Basic (0.00s)
panic: runtime error: unsafe pointer conversion [recovered]
	panic: runtime error: unsafe pointer conversion
goroutine 6 [running]:
testing.tRunner.func1(0xc000100360)
	/home/travis/.gimme/versions/go/src/testing/testing.go:916 +0xaeb
panic(0x6b5ee0, 0xc00000e260)
	/home/travis/.gimme/versions/go/src/runtime/panic.go:967 +0x396
github.com/etcd-io/bbolt.(*freelist).write(0xc0000fc080, 0xc0000ff000, 0xc0000ff000, 0x0)
	/home/travis/gopath/pkg/mod/github.com/etcd-io/[email protected]/freelist.go:318 +0xe9
github.com/etcd-io/bbolt.(*Tx).commitFreelist(0xc00011a0e0, 0x2, 0x7f603f3d1000)
	/home/travis/gopath/pkg/mod/github.com/etcd-io/[email protected]/tx.go:234 +0x204
github.com/etcd-io/bbolt.(*Tx).Commit(0xc00011a0e0, 0x0, 0x0)
	/home/travis/gopath/pkg/mod/github.com/etcd-io/[email protected]/tx.go:175 +0x7d6
github.com/etcd-io/bbolt.(*DB).Update(0xc000106000, 0xc00005abf8, 0x0, 0x0)
	/home/travis/gopath/pkg/mod/github.com/etcd-io/[email protected]/db.go:701 +0x174
...

[typeless]

Maybe this https://go-review.googlesource.com/c/net/+/203400/2/ipv4/control_bsd.go gives a hint.

[jrick]

I made this unpleasant discovery while looking into this.

% grep -n '&p.ptr' *.go
freelist.go:273:		count = int(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[0])
freelist.go:280:		ids := ((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[idx : idx+count]
freelist.go:318:		f.copyall(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[:])
freelist.go:321:		((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[0] = pgid(lenids)
freelist.go:322:		f.copyall(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[1:])
node.go:210:	b := (*[maxAllocSize]byte)(unsafe.Pointer(&p.ptr))[n.pageElementSize()*len(n.inodes):]
page.go:54:	return (*meta)(unsafe.Pointer(&p.ptr))
page.go:59:	n := &((*[0x7FFFFFF]leafPageElement)(unsafe.Pointer(&p.ptr)))[index]
page.go:68:	return ((*[0x7FFFFFF]leafPageElement)(unsafe.Pointer(&p.ptr)))[:]
page.go:73:	return &((*[0x7FFFFFF]branchPageElement)(unsafe.Pointer(&p.ptr)))[index]
page.go:81:	return ((*[0x7FFFFFF]branchPageElement)(unsafe.Pointer(&p.ptr)))[:]

10/11 of these type conversions scream wrong to me, and I have little confidence that the 11th is far better.

Quoting from unsafe.Pointer's docs:

(1) Conversion of a *T1 to Pointer to *T2.

Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, this conversion allows reinterpreting data of one type as data of another type.

The conversions to arrays of 0x7FFFFFF elements are clearly violating this rule, and the ones to maxAllocSize are probably invalid as well.

Taking the address &p.ptr is just about as dubious. This is the last field of the page struct, and is used as a way of accessing the address to the beginning of the data (where all of the fields before this in the struct are the "header"). I'm not sure why this was done vs using pointer arithmetic with an offset of unsafe.Sizeof(page{}) and leaving the ptr field out of it.

[jrick]

I've made some initial progress towards fixing this here: https://github.com/jrick/bbolt/tree/checkptr (compare)

The approach I've taken is to only create slices of the necessary size, rather than subslicing arrays which are too large. I've also removed unaligned access even on arches where the hardware permits it.

However, I've introduced a data consistency error that isn't caught by the tests on master:

% go test -args -quick.seed=62000
seed: 62000
quick settings: count=5, items=1000, ksize=1024, vsize=1024
00010000000000000000000000000000


consistency check failed (11 errors)
page 144115188075856502: already freed
page 630: unreachable unfreed
page 631: unreachable unfreed
page 632: unreachable unfreed
page 633: unreachable unfreed
page 634: unreachable unfreed
page 635: unreachable unfreed
page 636: unreachable unfreed
page 637: unreachable unfreed
page 638: unreachable unfreed
page 639: unreachable unfreed

db saved to:
/var/folders/rr/dzprnv4x4yvd_0xpz423brv00000gn/T/bolt-746811937


exit status 255
FAIL	go.etcd.io/bbolt	159.437s

Help is appreciated since this is an unfamiliar codebase for me.

edit: fixed in 272f6cd.

[dominikh]

The conversions to arrays of 0x7FFFFFF elements are clearly violating this rule, and the ones to maxAllocSize are probably invalid as well.

After looking into the checkptr code, re-reading https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices and experimenting myself, it is my understanding that this isn't true per se. Specifically, there is a valid pattern of converting between two slices involving these huge arrays, namely

(*[0x7FFFFFF]Type2)(unsafe.Pointer(&theSlice[0]))[:length:length]

The overall value of this expression, if considered as a single unit, does not produce any pointers that straddle more than one object (given the correct value for length)

This pattern has in the past been preferred over constructing a reflect.SliceHeader from scratch. In particular, reflect.SliceHeader stores a uintptr, so strictly speaking, this would be invalid:

hdr := reflect.SliceHeader{...}
s := *(*[]T)(&hdr)

as both the construction of the header and the conversion to the slice type would have to occur as one.

I can't really speak to any of the code flagged here, though; most of these slices look bizarre, as does the use of &p.ptr.

[jrick]

Thanks for pointing that out; had not seen that before.

hdr := reflect.SliceHeader{...}
s := *(*[]T)(&hdr)

This would be invalid if it was a Go pointer converted to uintptr, since a garbage collection may occur between statements and the uintptr is not sufficient to keep the object alive. However it would not be invalid for any allocations with manual lifetime management (C malloc, or mmap pages as bbolt deals with).

In any case, my patch used the pattern

s := *(*[]T)(&reflect.SliceHeader{...})

instead of defining a reflect.SliceHeader variable in scope. This should have the same properties: treated as a single expression, the conversion occurs "at the same time".

In any case, I tried writing some short programs doing the "max array length slice" pattern, and was not able to trip checkptr. The backing memory allocations were being created using C calloc.

[ncw]

Now that go1.14 is released I switched the rclone CI over to using it, I'm now seeing these in the race enabled tests,

fatal error: checkptr: unsafe pointer conversion

goroutine 23 [running]:
runtime.throw(0x11be3a3, 0x23)
	/opt/hostedtoolcache/go/1.14.0/x64/src/runtime/panic.go:1112 +0x72 fp=0xc0000af258 sp=0xc0000af228 pc=0x463452
runtime.checkptrAlignment(0x7f70f0ac20a1, 0x10c3ca0, 0x1)
	/opt/hostedtoolcache/go/1.14.0/x64/src/runtime/checkptr.go:13 +0xd0 fp=0xc0000af288 sp=0xc0000af258 pc=0x434870
github.com/etcd-io/bbolt.(*Bucket).openBucket(0xc0002a0398, 0x7f70f0ac20a1, 0x20, 0x20, 0x4)
	/home/runner/work/rclone/pkg/mod/github.com/etcd-io/[email protected]/bucket.go:138 +0x44e fp=0xc0000af3a0 sp=0xc0000af288 pc=0xb4898e
github.com/etcd-io/bbolt.(*Bucket).Bucket(0xc0002a0398, 0xc0000af620, 0x4, 0x4, 0x4)
	/home/runner/work/rclone/pkg/mod/github.com/etcd-io/[email protected]/bucket.go:113 +0x2a1 fp=0xc0000af4b0 sp=0xc0000af3a0 pc=0xb48301
github.com/etcd-io/bbolt.(*Bucket).DeleteBucket(0xc0002a0398, 0xc0000af620, 0x4, 0x4, 0xc0000af650, 0xb55349)
	/home/runner/work/rclone/pkg/mod/github.com/etcd-io/[email protected]/bucket.go:229 +0x2d5 fp=0xc0000af5e8 sp=0xc0000af4b0 pc=0xb49775
github.com/etcd-io/bbolt.(*Tx).DeleteBucket(...)
	/home/runner/work/rclone/pkg/mod/github.com/etcd-io/[email protected]/tx.go:121
github.com/rclone/rclone/backend/cache.(*Persistent).Purge.func1(0xc0002a0380, 0x7f70f0ac0001, 0xc0002a0380)
	/home/runner/work/rclone/src/github.com/rclone/rclone/backend/cache/storage_persistent.go:696 +0x6c fp=0xc0000af660 sp=0xc0000af5e8 pc=0xbab19c

This is using github.com/etcd-io/bbolt v1.3.3

Would it be possible to

• merge this fix?
• make a release with it in so go mod users pick it up?

Thank you :-)

[egonelbre]

CC: @xiang90, @gyuho seem to have been recently merging things.

[liggitt]

cc @jpbetz

[jared2501]

Have hit this when upgrading to 1.14 too! Would be awesome to get the fix merged

[Roasbeef]

So after updating the dependencies of our project after this issue got closed, with Go 1.14.1 we seem to be running into something distinct when running our tests w/ the race condition detector on:

runtime: pointer 0xc0002ee000 to unallocated span span.base()=0xc0002ee000 span.limit=0xc0002effe0 span.state=0
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

I'm unsure if this is something entirely new, or was simply uncovered due to the recent fixes in this area. If it's unrelated, I'll open a distinct issue for it. Has anyone else run into similar issues?

[Roasbeef]

Never-mind, looks like we're running into this, which will be fixed in Go 1.14.2.

[Roasbeef]

Running into another issue that might be attributed to #201, detailed it so far here: #214

[forsaken628]

same issue

fatal error: checkptr: converted pointer straddles multiple allocations

goroutine 6 [running]:
runtime.throw(0x21d4b20, 0x3a)
	/usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc00023ef20 sp=0xc00023eef0 pc=0xa14952
runtime.checkptrAlignment(0xc000572430, 0x20d4920, 0x1)
	/usr/local/go/src/runtime/checkptr.go:20 +0xc9 fp=0xc00023ef50 sp=0xc00023ef20 pc=0x9e3589
go.etcd.io/bbolt.(*Bucket).write(0xc00023f0c0, 0x0, 0x0, 0x0)
	/var/jenkins/gopath/pkg/mod/go.etcd.io/[email protected]/bucket.go:624 +0x15f fp=0xc00023efb8 sp=0xc00023ef50 pc=0xc2941f

[jrick]

@forsaken628 use bbolt 1.3.5.