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.

Sign up for GitHub

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

Jump to bottom

Validate page being fetched at possition 'p' self identifies as page 'p' #358

Merged
merged 3 commits into from
Dec 23, 2022
Merged

Validate page being fetched at possition 'p' self identifies as page 'p' #358

merged 3 commits into from
Dec 23, 2022

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Dec 17, 2022

It's the easiest verification whether the page is actually written, or its 'random' garbage in the block.

For example for corruption from '#335' it will fail quickly with:

$ go build ./cmd/bbolt/ && ./bbolt check ~/Downloads/db
2022/12/17 15:45:48 Page expected to be: 821, but self identifies as 8314893338927566090
panic: Page expected to be: 821, but self identifies as 8314893338927566090

goroutine 1 [running]:
log.Panicf({0x102a8718e?, 0x14000118960?}, {0x14000118508?, 0x140000660c0?, 0x18b?})
        /usr/local/go/src/log/log.go:395 +0x68
go.etcd.io/bbolt.(*page).fastCheck(...)
        /Users/ptab/gits/bbolt/page.go:59
go.etcd.io/bbolt.(*Tx).page(0x12aa76000?, 0x10?)
        /Users/ptab/gits/bbolt/tx.go:619 +0x168
go.etcd.io/bbolt.(*Tx).forEachPage(0x12a5f3000?, 0x12aa55000?, 0x2, 0x14000118678)
        /Users/ptab/gits/bbolt/tx.go:625 +0x28
go.etcd.io/bbolt.(*Tx).forEachPage(0x12b60d000?, 0x0?, 0x1, 0x14000118678)
        /Users/ptab/gits/bbolt/tx.go:634 +0x8c
go.etcd.io/bbolt.(*Tx).forEachPage(0x140001086a8?, 0x102a711a4?, 0x0, 0x14000118678)
        /Users/ptab/gits/bbolt/tx.go:634 +0x8c
go.etcd.io/bbolt.(*Tx).checkBucket(0x14000110000, 0x14000022200, 0x14000118960, 0x14000118930, 0x140000660c0)
        /Users/ptab/gits/bbolt/tx.go:464 +0x98
go.etcd.io/bbolt.(*Tx).checkBucket.func2({0x12aa551b9?, 0x102a710c8?, 0x12aa55000?}, {0x102a67e84?, 0x14000108768?, 0x102a67e64?})
        /Users/ptab/gits/bbolt/tx.go:489 +0x70
go.etcd.io/bbolt.(*Bucket).ForEach(0x14000110018, 0x14000118798)
        /Users/ptab/gits/bbolt/bucket.go:390 +0xa8
go.etcd.io/bbolt.(*Tx).checkBucket(0x14000110000, 0x14000110018, 0x14000108960, 0x14000108930, 0x140000660c0)
        /Users/ptab/gits/bbolt/tx.go:487 +0xe0
go.etcd.io/bbolt.(*DB).freepages(0x102a7e12f?)
        /Users/ptab/gits/bbolt/db.go:1059 +0x154
go.etcd.io/bbolt.(*DB).loadFreelist.func1()
        /Users/ptab/gits/bbolt/db.go:320 +0xb8
sync.(*Once).doSlow(0x1400010c1c8?, 0x102ac05c0?)
        /usr/local/go/src/sync/once.go:74 +0x104
sync.(*Once).Do(...)
        /usr/local/go/src/sync/once.go:65
go.etcd.io/bbolt.(*DB).loadFreelist(0x1400010c000?)
        /Users/ptab/gits/bbolt/db.go:316 +0x48
go.etcd.io/bbolt.Open({0x16d45ba96, 0x18}, 0x1b6?, 0x0)
        /Users/ptab/gits/bbolt/db.go:293 +0x450
main.(*CheckCommand).Run(0x14000119e48, {0x140000101d0, 0x1, 0x1})
        /Users/ptab/gits/bbolt/cmd/bbolt/main.go:204 +0x16c
main.(*Main).Run(0x14000098f38, {0x140000101c0, 0x2, 0x2})
        /Users/ptab/gits/bbolt/cmd/bbolt/main.go:114 +0x794
main.main()
        /Users/ptab/gits/bbolt/cmd/bbolt/main.go:72 +0xb0

Signed-off-by: Piotr Tabor ptab@google.com

ahrtr
ahrtr reviewed Dec 22, 2022
View reviewed changes
page.go Outdated
@@ -53,6 +54,19 @@ func (p *page) meta() *meta {
return (*meta)(unsafeAdd(unsafe.Pointer(p), unsafe.Sizeof(*p)))
}

func (p *page) fastCheck(id pgid) {
if p.id != id {
log.Panicf("Page expected to be: %v, but self identifies as %v", id, p.id)
Copy link
Member

@ahrtr ahrtr Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we just panic? Usually bbolt is supposed to be embedded in the client application, and we either return an error or panic directly. It might not be good to write a log to stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Used assert.

page.go Outdated
p.flags != leafPageFlag &&
p.flags != metaPageFlag &&
p.flags != freelistPageFlag {
log.Panicf("page %v: has unexpected type/flags: %x", p.id, p.flags)
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Used assert.

@ptabor
Validate page being fetched at possition 'p' self identifies as page …
d1aa803 
…'p'.

It's the easiest verification whether the page is actually written, or its 'random' garbage in the block.

Signed-off-by: Piotr Tabor <ptab@google.com>
@ptabor
Detect pages that have multiple flags set.
3cbd9c9 
Signed-off-by: Piotr Tabor <ptab@google.com>
@ptabor
Defensive: Expect page that node writes onto to be zeroed.
86ce028 
I've seen data corruption that seen like a random bit-flip
or application on already allocated page.

Signed-off-by: Piotr Tabor <ptab@google.com>
ahrtr
ahrtr approved these changes Dec 23, 2022
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @ptabor

@ptabor
Copy link
Contributor Author

ptabor commented Dec 23, 2022

Thank you.

@ptabor ptabor merged commit d4831e6 into etcd-io:master Dec 23, 2022
@ahrtr ahrtr added this to the 1.3.7 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
1.3.7
Development

Successfully merging this pull request may close these issues.
2 participants
@ptabor @ahrtr