diff --git a/internal/surgeon/xray.go b/internal/surgeon/xray.go index b3f4e2bb1..4f60f9c4b 100644 --- a/internal/surgeon/xray.go +++ b/internal/surgeon/xray.go @@ -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 { + 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) } @@ -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 } } @@ -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 { @@ -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++ { diff --git a/internal/surgeon/xray_test.go b/internal/surgeon/xray_test.go index 5f38d96ee..840a3d3d5 100644 --- a/internal/surgeon/xray_test.go +++ b/internal/surgeon/xray_test.go @@ -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" ) @@ -34,6 +35,98 @@ 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") +} + +// TestFindPathsToKey_MultipleBuckets ensures the shared visited map in +// traverse does not suppress legitimate paths when the same key appears +// in more than one bucket. Each top-level bucket owns a disjoint page +// sub-tree, so both occurrences of the key must be returned. +func TestFindPathsToKey_MultipleBuckets(t *testing.T) { + sharedKey := []byte("shared") + bucketA := []byte("bucketA") + bucketB := []byte("bucketB") + + db := btesting.MustCreateDB(t) + // Fill each bucket with enough entries that it owns a real (non-inline) + // root page and its own leaf pages, so the shared key lands on a pgid + // the visited map actually tracks. + require.NoError(t, db.Update(func(tx *bbolt.Tx) error { + for _, name := range [][]byte{bucketA, bucketB} { + b, err := tx.CreateBucket(name) + require.NoError(t, err) + for i := 0; i < 500; i++ { + k := []byte(fmt.Sprintf("%04d", i)) + require.NoError(t, b.Put(k, make([]byte, 100))) + } + require.NoError(t, b.Put(sharedKey, []byte("v"))) + } + return nil + })) + require.NoError(t, db.Close()) + + paths, err := surgeon.NewXRay(db.Path()).FindPathsToKey(sharedKey) + require.NoError(t, err) + require.Len(t, paths, 2, "expected one path per bucket containing the shared key") + + leafPgids := make(map[common.Pgid]struct{}) + for _, p := range paths { + require.NotEmpty(t, p) + leaf := p[len(p)-1] + leafPgids[leaf] = struct{}{} + + page, _, err := guts_cli.ReadPage(db.Path(), uint64(leaf)) + require.NoError(t, err) + require.True(t, page.IsLeafPage(), "terminal page in stack must be a leaf") + } + require.Len(t, leafPgids, 2, "the two paths must terminate on distinct leaf pages") +} + func TestFindPathsToKey_Bucket(t *testing.T) { rootBucket := []byte("data") subBucket := []byte("0451A")