From 0a521c0b487d04a28e15b3fe4978271310503a6e Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Wed, 9 Aug 2023 16:28:14 +0300 Subject: [PATCH 1/5] bucket: allow to allocate key on stack in Put() As per `go build -gcflags -m ./... 2>&1`: Old behaviour: ``` ./bucket.go:148:31: leaking param: key ./bucket.go:192:42: leaking param: key ./bucket.go:271:22: leaking param: key ``` Now: ``` ./bucket.go:148:31: key does not escape ./bucket.go:192:42: key does not escape ./bucket.go:271:22: key does not escape ``` Signed-off-by: Evgenii Stratonikov (cherry picked from commit 71a59caf31ca42c3deb8967ceee0a4804d220bf4) Signed-off-by: Wei Fu --- bucket.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/bucket.go b/bucket.go index 054467af3..ada05e3b7 100644 --- a/bucket.go +++ b/bucket.go @@ -183,15 +183,17 @@ func (b *Bucket) CreateBucket(key []byte) (*Bucket, error) { var value = bucket.write() // Insert into node. - key = cloneBytes(key) - c.node().put(key, key, value, 0, bucketLeafFlag) + // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent + // it from being marked as leaking, and accordingly cannot be allocated on stack. + newKey := cloneBytes(key) + c.node().put(newKey, newKey, value, 0, bucketLeafFlag) // Since subbuckets are not allowed on inline buckets, we need to // dereference the inline page, if it exists. This will cause the bucket // to be treated as a regular, non-inline bucket for the rest of the tx. b.page = nil - return b.Bucket(key), nil + return b.Bucket(newKey), nil } // CreateBucketIfNotExists creates a new bucket if it doesn't already exist and returns a reference to it. @@ -298,8 +300,10 @@ func (b *Bucket) Put(key []byte, value []byte) error { } // Insert into node. - key = cloneBytes(key) - c.node().put(key, key, value, 0, 0) + // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent + // it from being marked as leaking, and accordingly cannot be allocated on stack. + newKey := cloneBytes(key) + c.node().put(newKey, newKey, value, 0, 0) return nil } From d520aaac7c4c01503d359fce74d094124ceda67b Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Tue, 12 Dec 2023 19:44:03 +0800 Subject: [PATCH 2/5] *: introduce failpoint beforeBucketPut Signed-off-by: Wei Fu (cherry picked from commit 324df9cd264b7f2c4504ea61758a4ea89373e4ce) Signed-off-by: Wei Fu --- bucket.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bucket.go b/bucket.go index ada05e3b7..fc8f91a24 100644 --- a/bucket.go +++ b/bucket.go @@ -299,6 +299,8 @@ func (b *Bucket) Put(key []byte, value []byte) error { return ErrIncompatibleValue } + // gofail: var beforeBucketPut struct{} + // Insert into node. // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent // it from being marked as leaking, and accordingly cannot be allocated on stack. From b3bdd17686f00f5441b136180fee724f6dac4e08 Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Tue, 12 Dec 2023 21:40:08 +0800 Subject: [PATCH 3/5] tests/robustness: add issue72 reproducer Signed-off-by: Wei Fu (cherry picked from commit 1b080787075bb3ec06a414c754da9ef66bda4071) Signed-off-by: Wei Fu --- tests/failpoint/db_failpoint_test.go | 115 +++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/failpoint/db_failpoint_test.go b/tests/failpoint/db_failpoint_test.go index ef7d7ca63..d9201ef1f 100644 --- a/tests/failpoint/db_failpoint_test.go +++ b/tests/failpoint/db_failpoint_test.go @@ -94,3 +94,118 @@ func TestFailpoint_mLockFail_When_remap(t *testing.T) { require.NoError(t, err) } + +// TestIssue72 reproduces issue 72. +// +// When bbolt is processing a `Put` invocation, the key might be concurrently +// updated by the application which calls the `Put` API (although it shouldn't). +// It might lead to a situation that bbolt use an old key to find a proper +// position to insert the key/value pair, but actually inserts a new key. +// Eventually it might break the rule that all keys should be sorted. In a +// worse case, it might cause page elements to point to already freed pages. +// +// REF: https://github.com/etcd-io/bbolt/issues/72 +func TestIssue72(t *testing.T) { + db := btesting.MustCreateDBWithOption(t, &bolt.Options{PageSize: 4096}) + + bucketName := []byte(t.Name()) + err := db.Update(func(tx *bolt.Tx) error { + _, txerr := tx.CreateBucket(bucketName) + return txerr + }) + require.NoError(t, err) + + // The layout is like: + // + // +--+--+--+ + // +------+1 |3 |10+---+ + // | +-++--+--+ | + // | | | + // | | | + // +v-+--+ +v-+--+ +-v+--+--+ + // |1 |2 | |3 |4 | |10|11|12| + // +--+--+ +--+--+ +--+--+--+ + // + err = db.Update(func(tx *bolt.Tx) error { + bk := tx.Bucket(bucketName) + + for _, id := range []int{1, 2, 3, 4, 10, 11, 12} { + if txerr := bk.Put(idToBytes(id), make([]byte, 1000)); txerr != nil { + return txerr + } + } + return nil + }) + require.NoError(t, err) + + require.NoError(t, gofail.Enable("beforeBucketPut", `sleep(5000)`)) + + // +--+--+--+ + // +------+1 |3 |1 +---+ + // | +-++--+--+ | + // | | | + // | | | + // +v-+--+ +v-+--+ +-v+--+--+--+ + // |1 |2 | |3 |4 | |1 |10|11|12| + // +--+--+ +--+--+ +--+--+--+--+ + // + key := idToBytes(13) + updatedKey := idToBytes(1) + err = db.Update(func(tx *bolt.Tx) error { + bk := tx.Bucket(bucketName) + + go func() { + time.Sleep(3 * time.Second) + copy(key, updatedKey) + }() + return bk.Put(key, make([]byte, 100)) + }) + require.NoError(t, err) + + require.NoError(t, gofail.Disable("beforeBucketPut")) + + // bbolt inserts 100 into last branch page. Since there are two `1` + // keys in branch, spill operation will update first `1` pointer and + // then last one won't be updated and continues to point to freed page. + // + // + // +--+--+--+ + // +---------------+1 |3 |1 +---------+ + // | +--++-+--+ | + // | | | + // | | | + // | +--+--+ +v-+--+ +-----v-----+ + // | |1 |2 | |3 |4 | |freed page | + // | +--+--+ +--+--+ +-----------+ + // | + // +v-+--+--+--+---+ + // |1 |10|11|12|100| + // +--+--+--+--+---+ + err = db.Update(func(tx *bolt.Tx) error { + return tx.Bucket(bucketName).Put(idToBytes(100), make([]byte, 100)) + }) + require.NoError(t, err) + + defer func() { + if r := recover(); r != nil { + t.Logf("panic info:\n %v", r) + } + }() + + // Add more keys to ensure branch node to spill. + err = db.Update(func(tx *bolt.Tx) error { + bk := tx.Bucket(bucketName) + + for _, id := range []int{101, 102, 103, 104, 105} { + if txerr := bk.Put(idToBytes(id), make([]byte, 1000)); txerr != nil { + return txerr + } + } + return nil + }) + require.NoError(t, err) +} + +func idToBytes(id int) []byte { + return []byte(fmt.Sprintf("%010d", id)) +} From 50ddad0f002ce4585f65a1d0273b17cb65d8b88e Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Tue, 12 Dec 2023 21:53:05 +0800 Subject: [PATCH 4/5] bucket: copy key before Put Application might change key value after seeking and before real put. This unexpected behaviour could corrupt database. When users file issue, maintainers doesn't know application behaviour. It could be caused by data race. This patch is to prevent such case and save maintainers' time. Signed-off-by: Wei Fu (cherry picked from commit a05ec68aaafcf77e22b9da83bd4069cad8cba39d) Signed-off-by: Wei Fu --- bucket.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/bucket.go b/bucket.go index fc8f91a24..ee152ee19 100644 --- a/bucket.go +++ b/bucket.go @@ -290,21 +290,22 @@ func (b *Bucket) Put(key []byte, value []byte) error { return ErrValueTooLarge } + // Insert into node. + // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent + // it from being marked as leaking, and accordingly cannot be allocated on stack. + newKey := cloneBytes(key) + // Move cursor to correct position. c := b.Cursor() - k, _, flags := c.seek(key) + k, _, flags := c.seek(newKey) // Return an error if there is an existing key with a bucket value. - if bytes.Equal(key, k) && (flags&bucketLeafFlag) != 0 { + if bytes.Equal(newKey, k) && (flags&bucketLeafFlag) != 0 { return ErrIncompatibleValue } // gofail: var beforeBucketPut struct{} - // Insert into node. - // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent - // it from being marked as leaking, and accordingly cannot be allocated on stack. - newKey := cloneBytes(key) c.node().put(newKey, newKey, value, 0, 0) return nil From fabe2fb55ef5408575e9b0f9769d12cf602c8297 Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Sun, 17 Dec 2023 21:47:49 +0800 Subject: [PATCH 5/5] *: copy key before comparing during CreateBucket It's follow-up of #637. Signed-off-by: Wei Fu (cherry picked from commit 62d80260de277168b2d59779bfb03c6ebfda08f4) Signed-off-by: Wei Fu --- bucket.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/bucket.go b/bucket.go index ee152ee19..f3533d344 100644 --- a/bucket.go +++ b/bucket.go @@ -162,12 +162,17 @@ func (b *Bucket) CreateBucket(key []byte) (*Bucket, error) { return nil, ErrBucketNameRequired } + // Insert into node. + // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent + // it from being marked as leaking, and accordingly cannot be allocated on stack. + newKey := cloneBytes(key) + // Move cursor to correct position. c := b.Cursor() - k, _, flags := c.seek(key) + k, _, flags := c.seek(newKey) // Return an error if there is an existing key. - if bytes.Equal(key, k) { + if bytes.Equal(newKey, k) { if (flags & bucketLeafFlag) != 0 { return nil, ErrBucketExists } @@ -182,10 +187,6 @@ func (b *Bucket) CreateBucket(key []byte) (*Bucket, error) { } var value = bucket.write() - // Insert into node. - // Tip: Use a new variable `newKey` instead of reusing the existing `key` to prevent - // it from being marked as leaking, and accordingly cannot be allocated on stack. - newKey := cloneBytes(key) c.node().put(newKey, newKey, value, 0, bucketLeafFlag) // Since subbuckets are not allowed on inline buckets, we need to