-
Notifications
You must be signed in to change notification settings - Fork 636
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
unaligned := uintptr(unsafe.Pointer(&value[0]))&unalignedMask != 0 | ||
if unaligned { | ||
value = cloneBytes(value) | ||
} | ||
|
@@ -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. | ||
|
@@ -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) | ||
} | ||
|
||
|
@@ -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 | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package bbolt | |
|
||
import ( | ||
"fmt" | ||
"reflect" | ||
"sort" | ||
"unsafe" | ||
) | ||
|
@@ -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 | ||
|
@@ -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 { | ||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here must be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code this replaced had len count as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see code before was: [idx:idx+count] There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
module go.etcd.io/bbolt | ||
|
||
go 1.12 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we bump up to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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= |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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