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

Fix unsafe pointer conversions caught by Go 1.14 checkptr #201

Merged
merged 1 commit into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions bolt_386.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,3 @@ const maxMapSize = 0x7FFFFFFF // 2GB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0xFFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_amd64.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
19 changes: 0 additions & 19 deletions bolt_arm.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,3 @@ const maxMapSize = 0x7FFFFFFF // 2GB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0xFFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned bool

func init() {
// Simple check to see whether this arch handles unaligned load/stores
// correctly.

// ARM9 and older devices require load/stores to be from/to aligned
// addresses. If not, the lower 2 bits are cleared and that address is
// read in a jumbled up order.

// 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is now failing on arm:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


brokenUnaligned = val != 0x11222211
}
3 changes: 0 additions & 3 deletions bolt_arm64.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_mips64x.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0x8000000000 // 512GB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_mipsx.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0x40000000 // 1GB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0xFFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_ppc.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0x7FFFFFFF // 2GB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0xFFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_ppc64.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_ppc64le.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
3 changes: 0 additions & 3 deletions bolt_riscv64.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = true
3 changes: 0 additions & 3 deletions bolt_s390x.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,3 @@ const maxMapSize = 0xFFFFFFFFFFFF // 256TB

// maxAllocSize is the size used when creating array pointers.
const maxAllocSize = 0x7FFFFFFF

// Are unaligned load/stores broken on this arch?
var brokenUnaligned = false
30 changes: 16 additions & 14 deletions bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,12 @@ func (b *Bucket) Bucket(name []byte) *Bucket {
func (b *Bucket) openBucket(value []byte) *Bucket {
var child = newBucket(b.tx)

// If unaligned load/stores are broken on this arch and value is
// unaligned simply clone to an aligned byte array.
unaligned := brokenUnaligned && uintptr(unsafe.Pointer(&value[0]))&3 != 0

// Unaligned access requires a copy to be made.
const unalignedMask = unsafe.Alignof(struct {
bucket
page
}{}) - 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

unaligned := uintptr(unsafe.Pointer(&value[0]))&unalignedMask != 0
if unaligned {
value = cloneBytes(value)
}
Expand Down Expand Up @@ -409,24 +411,24 @@ func (b *Bucket) Stats() BucketStats {

if p.count != 0 {
// If page has any elements, add all element headers.
used += leafPageElementSize * int(p.count-1)
used += leafPageElementSize * uintptr(p.count-1)

// Add all element key, value sizes.
// The computation takes advantage of the fact that the position
// of the last element's key/value equals to the total of the sizes
// of all previous elements' keys and values.
// It also includes the last element's header.
lastElement := p.leafPageElement(p.count - 1)
used += int(lastElement.pos + lastElement.ksize + lastElement.vsize)
used += uintptr(lastElement.pos + lastElement.ksize + lastElement.vsize)
}

if b.root == 0 {
// For inlined bucket just update the inline stats
s.InlineBucketInuse += used
s.InlineBucketInuse += int(used)
} else {
// For non-inlined bucket update all the leaf stats
s.LeafPageN++
s.LeafInuse += used
s.LeafInuse += int(used)
s.LeafOverflowN += int(p.overflow)

// Collect stats from sub-buckets.
Expand All @@ -447,13 +449,13 @@ func (b *Bucket) Stats() BucketStats {

// used totals the used bytes for the page
// Add header and all element headers.
used := pageHeaderSize + (branchPageElementSize * int(p.count-1))
used := pageHeaderSize + (branchPageElementSize * uintptr(p.count-1))

// Add size of all keys and values.
// Again, use the fact that last element's position equals to
// the total of key, value sizes of all previous elements.
used += int(lastElement.pos + lastElement.ksize)
s.BranchInuse += used
used += uintptr(lastElement.pos + lastElement.ksize)
s.BranchInuse += int(used)
s.BranchOverflowN += int(p.overflow)
}

Expand Down Expand Up @@ -593,7 +595,7 @@ func (b *Bucket) inlineable() bool {
// our threshold for inline bucket size.
var size = pageHeaderSize
for _, inode := range n.inodes {
size += leafPageElementSize + len(inode.key) + len(inode.value)
size += leafPageElementSize + uintptr(len(inode.key)) + uintptr(len(inode.value))

if inode.flags&bucketLeafFlag != 0 {
return false
Expand All @@ -606,8 +608,8 @@ func (b *Bucket) inlineable() bool {
}

// Returns the maximum total size of a bucket to make it a candidate for inlining.
func (b *Bucket) maxInlineBucketSize() int {
return b.tx.db.pageSize / 4
func (b *Bucket) maxInlineBucketSize() uintptr {
return uintptr(b.tx.db.pageSize / 4)
}

// write allocates and writes a bucket to a byte slice.
Expand Down
2 changes: 1 addition & 1 deletion db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1358,7 +1358,7 @@ func ExampleDB_View() {
// John's last name is doe.
}

func ExampleDB_Begin_ReadOnly() {
func ExampleDB_Begin() {
// Open the database.
db, err := bolt.Open(tempfile(), 0666, nil)
if err != nil {
Expand Down
37 changes: 29 additions & 8 deletions freelist.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package bbolt

import (
"fmt"
"reflect"
"sort"
"unsafe"
)
Expand Down Expand Up @@ -71,7 +72,7 @@ func (f *freelist) size() int {
// The first element will be used to store the count. See freelist.write.
n++
}
return pageHeaderSize + (int(unsafe.Sizeof(pgid(0))) * n)
return int(pageHeaderSize) + (int(unsafe.Sizeof(pgid(0))) * n)
}

// count returns count of pages on the freelist
Expand All @@ -93,8 +94,24 @@ func (f *freelist) pending_count() int {
return count
}

// copyall copies into dst a list of all free ids and all pending ids in one sorted list.
// copyallunsafe copies a list of all free ids and all pending ids in one sorted list.
// f.count returns the minimum length required for dst.
func (f *freelist) copyallunsafe(dstptr unsafe.Pointer) { // dstptr is []pgid data pointer
m := make(pgids, 0, f.pending_count())
for _, txp := range f.pending {
m = append(m, txp.ids...)
}
sort.Sort(m)
fpgids := f.getFreePageIDs()
sz := len(fpgids) + len(m)
dst := *(*[]pgid)(unsafe.Pointer(&reflect.SliceHeader{
Data: uintptr(dstptr),
Len: sz,
Cap: sz,
}))
mergepgids(dst, fpgids, m)
}

func (f *freelist) copyall(dst []pgid) {
m := make(pgids, 0, f.pending_count())
for _, txp := range f.pending {
Expand Down Expand Up @@ -267,17 +284,21 @@ func (f *freelist) read(p *page) {
}
// If the page.count is at the max uint16 value (64k) then it's considered
// an overflow and the size of the freelist is stored as the first element.
idx, count := 0, int(p.count)
var idx, count uintptr = 0, uintptr(p.count)
if count == 0xFFFF {
idx = 1
count = int(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[0])
count = uintptr(*(*pgid)(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p))))
}

// Copy the list of page ids from the freelist.
if count == 0 {
f.ids = nil
} else {
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),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here must be idx?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code this replaced had len count as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no

Cap: int(count),
}))

// copy the ids, so we don't modify on the freelist page directly
idsCopy := make([]pgid, count)
Expand Down Expand Up @@ -315,11 +336,11 @@ func (f *freelist) write(p *page) error {
p.count = uint16(lenids)
} else if lenids < 0xFFFF {
p.count = uint16(lenids)
f.copyall(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[:])
f.copyallunsafe(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p)))
} else {
p.count = 0xFFFF
((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[0] = pgid(lenids)
f.copyall(((*[maxAllocSize]pgid)(unsafe.Pointer(&p.ptr)))[1:])
*(*pgid)(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p))) = pgid(lenids)
f.copyallunsafe(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p) + unsafe.Sizeof(pgid(0))))
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion freelist_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ func TestFreelist_read(t *testing.T) {
page.count = 2

// Insert 2 page ids.
ids := (*[3]pgid)(unsafe.Pointer(&page.ptr))
ids := (*[3]pgid)(unsafe.Pointer(uintptr(unsafe.Pointer(page)) + unsafe.Sizeof(*page)))
ids[0] = 23
ids[1] = 50

Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
module go.etcd.io/bbolt

go 1.12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we bump up to 1.14 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


require golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5 h1:LfCXLvNmTYH9kEmVgqbnsWfruoXZIrh4YBgqVHtDvw0=
golang.org/x/sys v0.0.0-20200202164722-d101bd2416d5/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
Loading