Skip to content

surgeon: detect cycles in XRay.traverse #1193

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

Merged
ahrtr merged 2 commits into from
Apr 22, 2026
Merged

surgeon: detect cycles in XRay.traverse #1193

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b06ba6a
surgeon: detect cycles in XRay.traverse
ivangsm Apr 18, 2026
29cb88e
surgeon: test FindPathsToKey returns one stack per bucket
ivangsm Apr 22, 2026
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
18 changes: 12 additions & 6 deletions internal/surgeon/xray.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,14 @@ func NewXRay(path string) XRay {
return XRay{path}
}

func (n XRay) traverse(stack []common.Pgid, callback func(page *common.Page, stack []common.Pgid) error) error {
p, data, err := guts_cli.ReadPage(n.path, uint64(stack[len(stack)-1]))
func (n XRay) traverse(stack []common.Pgid, visited map[common.Pgid]struct{}, callback func(page *common.Page, stack []common.Pgid) error) error {
Copy link
Copy Markdown
Member

@ahrtr ahrtr Apr 20, 2026

Choose a reason for hiding this comment

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

The visited map is shared across all paths. It may result in false alarm. Multiple keys with the same value might be included in multiple buckets/pages.

Copy link
Copy Markdown
Contributor

@tjungblu tjungblu Apr 21, 2026

Choose a reason for hiding this comment

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

how can the same page be reused in different buckets?

Copy link
Copy Markdown
Member

@ahrtr ahrtr Apr 21, 2026

Choose a reason for hiding this comment

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

The visited caches all pages it ever visited, including the root page. Note the root page is the starting page to access any keys. Some branch pages might be also the shared path prefix for some keys.

Copy link
Copy Markdown
Member

@ahrtr ahrtr Apr 21, 2026

Choose a reason for hiding this comment

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

Re-read the traverse, it just traverses the whole B+tree top down, so normally it will never revisit page, no matter how many keys match. The only possible reason is the db is corrupted.

Copy link
Copy Markdown
Member

@ahrtr ahrtr Apr 21, 2026

Choose a reason for hiding this comment

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

Can you add a test case something like below?

  • create two buckets, and add the same key into both buckets. call FindPathsToKey with the key, it should return two stacks.

Copy link
Copy Markdown
Contributor Author

@ivangsm ivangsm Apr 22, 2026

Choose a reason for hiding this comment

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

Thanks for re-reading it — agreed that traverse is top-down, so in a well-formed tree no pgid is revisited and a revisit only happens under real corruption.

Added TestFindPathsToKey_MultipleBuckets in internal/surgeon/xray_test.go covering the case you described: two sibling top-level buckets, each filled with 500 entries plus the same "shared" key (the fill ensures neither bucket is inlined, so their own pgids actually pass through the visited map). The test asserts that FindPathsToKey("shared") returns exactly 2 paths and that the two terminal leaf pgids are distinct — which directly exercises the concern that a shared visited could otherwise suppress the second hit.

pgid := stack[len(stack)-1]
if _, ok := visited[pgid]; ok {
return fmt.Errorf("cycle detected at page %d (stack %v)", pgid, stack)
}
visited[pgid] = struct{}{}

p, data, err := guts_cli.ReadPage(n.path, uint64(pgid))
if err != nil {
return fmt.Errorf("failed reading page (stack %v): %w", stack, err)
}
Expand All @@ -35,13 +41,13 @@ func (n XRay) traverse(stack []common.Pgid, callback func(page *common.Page, sta
{
m := common.LoadPageMeta(data)
r := m.RootBucket().RootPage()
return n.traverse(append(stack, r), callback)
return n.traverse(append(stack, r), visited, callback)
}
case "branch":
{
for i := uint16(0); i < p.Count(); i++ {
bpe := p.BranchPageElement(i)
if err := n.traverse(append(stack, bpe.Pgid()), callback); err != nil {
if err := n.traverse(append(stack, bpe.Pgid()), visited, callback); err != nil {
return err
}
}
Expand All @@ -52,7 +58,7 @@ func (n XRay) traverse(stack []common.Pgid, callback func(page *common.Page, sta
if lpe.IsBucketEntry() {
pgid := lpe.Bucket().RootPage()
if pgid > 0 {
if err := n.traverse(append(stack, pgid), callback); err != nil {
if err := n.traverse(append(stack, pgid), visited, callback); err != nil {
return err
}
} else {
Expand Down Expand Up @@ -81,7 +87,7 @@ func (n XRay) FindPathsToKey(key []byte) ([][]common.Pgid, error) {
if err != nil {
return nil, err
}
err = n.traverse([]common.Pgid{rootPage},
err = n.traverse([]common.Pgid{rootPage}, map[common.Pgid]struct{}{},
func(page *common.Page, stack []common.Pgid) error {
if page.Typ() == "leaf" {
for i := uint16(0); i < page.Count(); i++ {
Expand Down
49 changes: 49 additions & 0 deletions internal/surgeon/xray_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"go.etcd.io/bbolt"
"go.etcd.io/bbolt/internal/btesting"
"go.etcd.io/bbolt/internal/common"
"go.etcd.io/bbolt/internal/guts_cli"
"go.etcd.io/bbolt/internal/surgeon"
)
Expand All @@ -34,6 +35,54 @@ func TestFindPathsToKey(t *testing.T) {
assert.LessOrEqual(t, []byte("0451"), p.LeafPageElement(p.Count()-1).Key())
}

// TestFindPathsToKey_CycleDetected corrupts the db so that a branch page
// points at its own ancestor, then verifies that FindPathsToKey returns an
// error instead of recursing until stack overflow. See issue #701 for the
// real-world corruption pattern (power-off creating a page cycle).
func TestFindPathsToKey_CycleDetected(t *testing.T) {
db := btesting.MustCreateDB(t)
require.NoError(t,
db.Fill([]byte("data"), 1, 500,
func(tx int, k int) []byte { return []byte(fmt.Sprintf("%04d", k)) },
func(tx int, k int) []byte { return make([]byte, 100) },
))
require.NoError(t, db.Close())

// Find the path to an arbitrary key so we can pick a branch ancestor
// and one of its leaf descendants to corrupt.
navigator := surgeon.NewXRay(db.Path())
paths, err := navigator.FindPathsToKey([]byte("0001"))
require.NoError(t, err)
require.NotEmpty(t, paths)
path := paths[0]
require.GreaterOrEqual(t, len(path), 2, "need at least one branch above the leaf")

// Overwrite the leaf page with a copy of its branch ancestor. The
// rewritten page is still a branch and now references its own pgid
// through the ancestor's element list, forming a cycle.
ancestor := path[len(path)-2]
leaf := path[len(path)-1]
require.NoError(t, surgeon.CopyPage(db.Path(), ancestor, leaf))

// Confirm the ancestor actually lists the leaf as one of its children,
// so copying creates a real cycle rather than two disjoint branches.
ancestorPage, _, err := guts_cli.ReadPage(db.Path(), uint64(ancestor))
require.NoError(t, err)
require.True(t, ancestorPage.IsBranchPage())
var hasLeafAsChild bool
for i := uint16(0); i < ancestorPage.Count(); i++ {
if ancestorPage.BranchPageElement(i).Pgid() == common.Pgid(leaf) {
hasLeafAsChild = true
break
}
}
require.True(t, hasLeafAsChild, "expected ancestor to reference the leaf directly")

_, err = surgeon.NewXRay(db.Path()).FindPathsToKey([]byte("0001"))
require.Error(t, err)
require.Contains(t, err.Error(), "cycle detected")
}

func TestFindPathsToKey_Bucket(t *testing.T) {
rootBucket := []byte("data")
subBucket := []byte("0451A")
Expand Down