Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

jrick · 2020-02-07T19:00:27Z

Go 1.14 introduces a checkptr debug flag for the compiler, which is enabled by default with -race and can be enabled separately with -gcflags=all=-d=checkptr. checkptr enables two checks during type conversions of unsafe.Pointer:

All unsafe pointer conversions to type *T2 must have proper alignment for type T2
All unsafe pointer conversions from *T1 to unsafe.Pointer to *T2 must not cause the new allocation to span more than two separate Go heap allocations.

bbolt was performing unsafe pointer conversions that were failing each of these checks.

This change modifies bbolt to avoid type conversions that are both caught by the new checks and were previously invalid according to the Go memory model. Rather than subslicing arrays of "max allocations" number of elements, slices are created with the precise sizes required. This change also removes all unaligned access even on architectures where the hardware permits it.

The performance hit, if any, for removing unaligned access appears negligible according to the project's benchmarks:

% go test -run none -bench 'Benchmark*'
seed: 84000
quick settings: count=5, items=1000, ksize=1024, vsize=1024
goos: darwin
goarch: amd64
pkg: go.etcd.io/bbolt
Benchmark_FreelistRelease10K-24       	    3757	    302905 ns/op
Benchmark_FreelistRelease100K-24      	     294	   4076518 ns/op
Benchmark_FreelistRelease1000K-24     	      16	  66904544 ns/op
Benchmark_FreelistRelease10000K-24    	       1	1183743396 ns/op
BenchmarkDBBatchAutomatic-24          	      63	  16815505 ns/op
BenchmarkDBBatchSingle-24             	       1	14088192506 ns/op
BenchmarkDBBatchManual10x100-24       	       8	 141973506 ns/op
PASS
ok  	go.etcd.io/bbolt	25.682s
% git checkout master                  
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
% go test -run none -bench 'Benchmark*' -args -quick.seed=84000
seed: 84000
quick settings: count=5, items=1000, ksize=1024, vsize=1024
goos: darwin
goarch: amd64
pkg: go.etcd.io/bbolt
Benchmark_FreelistRelease10K-24       	    3794	    303829 ns/op
Benchmark_FreelistRelease100K-24      	     294	   4084449 ns/op
Benchmark_FreelistRelease1000K-24     	      15	  66988066 ns/op
Benchmark_FreelistRelease10000K-24    	       1	1156598746 ns/op
BenchmarkDBBatchAutomatic-24          	      62	  17064184 ns/op
BenchmarkDBBatchSingle-24             	       1	13940793747 ns/op
BenchmarkDBBatchManual10x100-24       	       8	 141596004 ns/op
PASS
ok  	go.etcd.io/bbolt	24.129s

Fixes #187.

jrick · 2020-02-08T08:17:10Z

bucket.go

+	const unalignedMask = unsafe.Alignof(struct {
+		bucket
+		page
+	}{}) - 1


I don't have any 32-bit hardware available (i386, armv7, etc) so if someone could test whether this still passes with -gcflags=all=-d=checkptr on these architectures I'd love to know. My concern is that the allocation for the copied byte slice below will be pointer aligned (4 bytes), and not aligned for the 64-bit fields of these structs (8 bytes).

qbit · 2020-02-08T14:35:23Z

On Sat, 08 Feb 2020 at 00:17:10 -0800, Josh Rickmar wrote: jrick commented on this pull request. > @@ -123,8 +123,11 @@ func (b *Bucket) Bucket(name []byte) *Bucket { func (b *Bucket) openBucket(value []byte) *Bucket { var child = newBucket(b.tx) - // Unaligned pointer access requires a copy to be made. - const unalignedMask = unsafe.Alignof(uintptr(0)) - 1 + // Unaligned access requires a copy to be made. + const unalignedMask = unsafe.Alignof(struct { + bucket + page + }{}) - 1 I don't have any 32-bit hardware available (i386, armv7, etc) so if someone could test whether this still passes with `-gcflags=all=-d=checkptr` on these architectures I'd love to know. My concern is that the allocation for the copied byte slice below will be pointer aligned (4 bytes), and not aligned for the 64-bit fields of these structs (8 bytes).

Tested on OpenBSD i386: go test -gcflags=all=-d=checkptr seed: 24103 quick settings: count=5, items=1000, ksize=1024, vsize=1024 00010000000000000000000000000000 unexpected fault address 0x6f010040 fatal error: fault [signal SIGSEGV: segmentation violation code=0x1 addr=0x6f010040 pc=0x8169ebd] goroutine 42 [running]: runtime.throw(0x8202ba8, 0x5) /home/qbit/go/src/runtime/panic.go:1112 +0x64 fp=0x4f086e44 sp=0x4f086e30 pc=0x8077674 runtime.sigpanic() /home/qbit/go/src/runtime/signal_unix.go:694 +0x2fe fp=0x4f086e5c sp=0x4f086e44 pc=0x808bb0e go.etcd.io/bbolt.(*DB).meta(0x4f07c000, 0x3) /home/qbit/bbolt/db.go:901 +0x1d fp=0x4f086e74 sp=0x4f086e5c pc=0x8169ebd go.etcd.io/bbolt.(*DB).hasSyncedFreelist(...) /home/qbit/bbolt/db.go:323 go.etcd.io/bbolt.(*Tx).rollback(0x4f068480) /home/qbit/bbolt/tx.go:280 +0x63 fp=0x4f086e90 sp=0x4f086e74 pc=0x8173343 go.etcd.io/bbolt.(*Tx).Commit(0x4f068480, 0x0, 0x0) /home/qbit/bbolt/tx.go:162 +0x481 fp=0x4f086f30 sp=0x4f086e90 pc=0x8172e01 go.etcd.io/bbolt.(*DB).Update(0x4f07c000, 0x4f086f84, 0x0, 0x0) /home/qbit/bbolt/db.go:701 +0xd1 fp=0x4f086f60 sp=0x4f086f30 pc=0x8169511 go.etcd.io/bbolt_test.TestDB_Put_VeryLarge(0x4f05c000) /home/qbit/bbolt/bucket_test.go:221 +0xfb fp=0x4f086fa8 sp=0x4f086f60 pc=0x818001b testing.tRunner(0x4f05c000, 0x820fa78) /home/qbit/go/src/testing/testing.go:993 +0xae fp=0x4f086fe8 sp=0x4f086fa8 pc=0x812640e runtime.goexit() /home/qbit/go/src/runtime/asm_386.s:1337 +0x1 fp=0x4f086fec sp=0x4f086fe8 pc=0x80a2d51 created by testing.(*T).Run /home/qbit/go/src/testing/testing.go:1044 +0x2a7 goroutine 1 [chan receive]: testing.(*T).Run(0x4f05c000, 0x82063f5, 0x14, 0x820fa78, 0x301) /home/qbit/go/src/testing/testing.go:1045 +0x2c6 testing.runTests.func1(0x4f05c140) /home/qbit/go/src/testing/testing.go:1286 +0x54 testing.tRunner(0x4f05c140, 0x4f02068c) /home/qbit/go/src/testing/testing.go:993 +0xae testing.runTests(0x4f00c060, 0x83683e0, 0xab, 0xab, 0x1) /home/qbit/go/src/testing/testing.go:1284 +0x227 testing.(*M).Run(0x4f038100, 0x0) /home/qbit/go/src/testing/testing.go:1201 +0x10e go.etcd.io/bbolt_test.TestMain(0x4f038100) /home/qbit/bbolt/quick_test.go:37 +0x3bc main.main() _testmain.go:418 +0xfa exit status 2 FAIL go.etcd.io/bbolt 6.815s

…

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #201 (review)

-- PGP: 0x1F81112D62A9ADCE / 3586 3350 BFEA C101 DB1A 4AF0 1F81 112D 62A9 ADCE

jrick · 2020-02-08T19:50:11Z

That looks like #183.

arjunsingri · 2020-03-02T00:59:27Z

Is there a timeline for when this will be merged?

jeffallen · 2020-03-18T13:24:09Z

Ping @xiang90, @gyuho @vorburger ? This is a pretty important change, not having it checked in prevents all users of boltdb from upgrading to Go 1.14. Do you need more committers on the boltdb team?

vorburger · 2020-03-18T18:14:12Z

Sorry I'm not currently active in this project.

gyuho · 2020-03-19T00:37:50Z

@jrick Can we squash commits?

jrick · 2020-03-19T01:19:50Z

@gyuho done.

gyuho · 2020-03-19T01:40:32Z

go.mod

@@ -1,3 +1,5 @@
 module go.etcd.io/bbolt

 go 1.12


Can we bump up to 1.14 as well?

Shouldn't be necessary. The Go version in the go.mod is the minimum supported Go version, and this still works fine with Go 1.12. Changing it is also unrelated to this fix, but you could change it in a different commit later...

gyuho · 2020-03-19T18:26:34Z

@jrick Thanks for the fix.

tmm1 · 2020-03-19T19:13:24Z

bolt_arm.go

-	// See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka15414.html
-
-	raw := [6]byte{0xfe, 0xef, 0x11, 0x22, 0x22, 0x11}
-	val := *(*uint32)(unsafe.Pointer(uintptr(unsafe.Pointer(&raw)) + 2))


Build is now failing on arm:

bolt_arm.go:3:8: imported and not used: "unsafe"

sorry :( didn't try a cross compile, which would have caught that

AskAlexSharov · 2020-03-25T07:54:06Z

freelist.go

-		ids := ((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[idx : idx+count]
+		ids := *(*[]pgid)(unsafe.Pointer(&reflect.SliceHeader{
+			Data: uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p) + idx*unsafe.Sizeof(pgid(0)),
+			Len:  int(count),


here must be idx?

the code this replaced had len count as well.

i see code before was: [idx:idx+count]
Means len=idx and cap=idx+count ?

This includes fix to boltdb unsafe pointer conversions discovered by extra checks added in Go 1.14 enabled in -race mode. Also changes CI to use Go 1.14. See etcd-io/bbolt#201 for background.

#11594 ## Description of change - Fix deferreturn panic on go1.14 on arm64 and ppc64 by disabling optimizations - Fix unaligned ptrs by moving to go.etcd.io/bbolt ## QA steps - Run unit tests - Build for ppc64 and arm64 (juju bins will be quite large) - Deploy a HA controller (+ anything else you can think to test raft) ## Documentation changes N/A ## Bug reference https://golang.org/issue/39049 etcd-io/bbolt#201

This unbreaks bbolt (as part of containerd) on 1.14+ (see etcd-io/bbolt#201 and etcd-io/bbolt#220), pulls in my patch to ignore image-defined volumes (containerd/cri#1504) and gets us some robustness fixes in containerd CNI/CRI integration (containerd/cri#1405). This also updates K8s at the same time since they share a lot of dependencies and only updating one is very annoying. On the K8s side we mostly get the standard stream of fixes plus some patches that are no longer necessary. One annoying on the K8s side (but with no impact to the functionality) are these messages in the logs of various components: ``` W0714 11:51:26.323590 1 warnings.go:67] policy/v1beta1 PodSecurityPolicy is deprecated in v1.22+, unavailable in v1.25+ ``` They are caused by KEP-1635, but there's not explanation why this gets logged so aggressively considering the operators cannot do anything about it. There's no newer version of PodSecurityPolicy and you are pretty much required to use it if you use RBAC. Test Plan: Covered by existing tests Bug: T753 X-Origin-Diff: phab/D597 GitOrigin-RevId: f6c447da1de037c27646f9ec9f45ebd5d6660ab0

To fix some issues caught by chkptr. See: etcd-io/bbolt#201

jrick commented Feb 8, 2020

View reviewed changes

jrick mentioned this pull request Feb 15, 2020

fatal error: fault (segmentation violation) during Commit #202

Closed

This was referenced Feb 20, 2020

build: go 1.14 decred/dcrpool#154

Merged

build: go 1.14 decred/dcrdex#184

Merged

Roasbeef mentioned this pull request Mar 1, 2020

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

Merged

arjunsingri mentioned this pull request Mar 2, 2020

Is there paid support available? #205

Closed

nkryuchkov mentioned this pull request Mar 3, 2020

Tests fail when using Go 1.14 skycoin/skywire#182

Closed

Foxboron mentioned this pull request Mar 7, 2020

Update to golang 1.15.2 containerd/containerd#4050

Merged

dims mentioned this pull request Mar 8, 2020

Update Golang to v1.14.4 kubernetes/kubernetes#88638

Merged

tharvik mentioned this pull request Mar 11, 2020

Timeout ldsec/unlynx#38

Merged

fyrchik mentioned this pull request Mar 17, 2020

.circleci: add go1.14 tests nspcc-dev/neo-go#775

Merged

Fix unsafe pointer conversions caught by Go 1.14 checkptr

543c40a

jrick force-pushed the checkptr branch from 52cf6d0 to 543c40a Compare March 19, 2020 01:19

gyuho reviewed Mar 19, 2020

View reviewed changes

gyuho approved these changes Mar 19, 2020

View reviewed changes

gyuho merged commit 2fc6815 into etcd-io:master Mar 19, 2020

jrick deleted the checkptr branch March 19, 2020 19:04

artyom mentioned this pull request Mar 19, 2020

Switch to etcd-io/bbolt and Go 1.14 Doist/bitmapist-server#18

Closed

tmm1 reviewed Mar 19, 2020

View reviewed changes

tmm1 mentioned this pull request Mar 19, 2020

Remove unused import to fix arm builds #210

Merged

prestonvanloon mentioned this pull request Mar 24, 2020

Migrate boltdb to bbolt prysmaticlabs/prysm#5186

Closed

AskAlexSharov reviewed Mar 25, 2020

View reviewed changes

jack-kelly mentioned this pull request Mar 25, 2020

Update to bolt v1.3.4 for go 1.14 unsafe pointer conversion fixes asdine/storm#265

Closed

This was referenced Apr 3, 2020

sweep increased allocation count with v1.3.4 #214

Closed

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

Closed

This was referenced Apr 24, 2020

Upgrade etcd to 3.4.7 google/trillian#2089

Closed

Upgrade etcd to 3.4.7 google/trillian#2090

Merged

hpidcock mentioned this pull request May 18, 2020

Fix deferreturn and bolt juju/juju#11594

Merged

nikkictl mentioned this pull request May 18, 2020

Upgrade to Go 1.14 sensu/sensu-go#3760

Closed

dnephin mentioned this pull request Jul 8, 2020

ci: add 'go test -race' hashicorp/consul#8269

Merged

amyangfei mentioned this pull request Jul 15, 2020

upgrade go.etcd.io/etcd to fix panic in unit test with go1.14 pingcap/tiflow#755

Merged

JmPotato mentioned this pull request Jul 29, 2020

go.mod: upgrade go.etcd.io/bbolt to fix panic in unit test with go1.14 tikv/pd#2690

Merged

leninkumar31 mentioned this pull request Oct 13, 2020

Please update boltdb/bolt to etcd-io / bbolt rapidloop/skv#6

Open

HunDunDM mentioned this pull request Jan 5, 2021

go.mod: upgrade go.etcd.io/bbolt to fix panic in unit test with go1.14 (#2690) tikv/pd#3331

Merged

zimulala mentioned this pull request May 20, 2021

*: update go.etcd.io/bbolt pingcap/tidb#24799

Merged

izaaklauer mentioned this pull request May 25, 2021

Upgrade from bolt to bbolt hashicorp/waypoint#1537

Closed

hexfusion mentioned this pull request Jun 18, 2021

[release-3.4]: vendor: update bbolt v1.3.4, zap v1.14.1 etcd-io/etcd#13125

Closed

rhafer added a commit to rhafer/idm that referenced this pull request Dec 7, 2021

Update to latest bbolt release

4a8b0e0

To fix some issues caught by chkptr. See: etcd-io/bbolt#201

rhafer mentioned this pull request Dec 7, 2021

Update to latest bbolt release libregraph/idm#26

Merged

rhafer added a commit to libregraph/idm that referenced this pull request Dec 7, 2021

Update to latest bbolt release

fef15b4

To fix some issues caught by chkptr. See: etcd-io/bbolt#201

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

jrick commented Feb 7, 2020

jrick Feb 8, 2020

qbit commented Feb 8, 2020 via email

jrick commented Feb 8, 2020

arjunsingri commented Mar 2, 2020 •

edited

Loading

jeffallen commented Mar 18, 2020

vorburger commented Mar 18, 2020

gyuho commented Mar 19, 2020

jrick commented Mar 19, 2020

gyuho Mar 19, 2020

jrick Mar 19, 2020

gyuho commented Mar 19, 2020

tmm1 Mar 19, 2020

jrick Mar 19, 2020

AskAlexSharov Mar 25, 2020

jrick Mar 25, 2020

AskAlexSharov Mar 25, 2020

jrick Mar 25, 2020

Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

Conversation

jrick commented Feb 7, 2020

Choose a reason for hiding this comment

qbit commented Feb 8, 2020 via email

jrick commented Feb 8, 2020

arjunsingri commented Mar 2, 2020 • edited Loading

jeffallen commented Mar 18, 2020

vorburger commented Mar 18, 2020

gyuho commented Mar 19, 2020

jrick commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjunsingri commented Mar 2, 2020 •

edited

Loading