From bbb6d33d125ebad03415855d9fb14a839dcc94bd Mon Sep 17 00:00:00 2001 From: Jackson Owens Date: Mon, 6 May 2024 11:18:06 -0400 Subject: [PATCH] db: fix level checker use of stale iterator value Recent refactors made it possible for the level checker to visit a point key twice if the point key's levelIter interleaved a boundary sentinel key. The corresponding level's simpleMergingIterItem's key and value were not updated, leaving the old point key on the heap. When the point key remained at the top of the heap, but the underlying levelIter had moved on, it was possible to access freed memory associated with the old point key's value. Fix #3556. --- level_checker.go | 4 ++-- level_checker_test.go | 22 ++++++++++++++-------- testdata/level_checker | 18 ++++++++++++++++++ 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/level_checker.go b/level_checker.go index 7efcb5010d5..b2f0936b1c8 100644 --- a/level_checker.go +++ b/level_checker.go @@ -146,13 +146,13 @@ func (m *simpleMergingIter) step() bool { m.err = errors.CombineErrors(l.iter.Error(), l.iter.Close()) l.iter = nil m.heap.pop() - } else if !l.iterKV.K.IsExclusiveSentinel() { + } else { // Check point keys in an sstable are ordered. Although not required, we check // for memtables as well. A subtle check here is that successive sstables of // L1 and higher levels are ordered. This happens when levelIter moves to the // next sstable in the level, in which case item.key is previous sstable's // last point key. - if base.InternalCompare(m.heap.cmp, item.key, l.iterKV.K) >= 0 { + if !l.iterKV.K.IsExclusiveSentinel() && base.InternalCompare(m.heap.cmp, item.key, l.iterKV.K) >= 0 { m.err = errors.Errorf("out of order keys %s >= %s in %s", item.key.Pretty(m.formatKey), l.iterKV.K.Pretty(m.formatKey), l.iter) return false diff --git a/level_checker_test.go b/level_checker_test.go index 901b22a2de2..947cc35e177 100644 --- a/level_checker_test.go +++ b/level_checker_test.go @@ -16,10 +16,12 @@ import ( "github.com/cockroachdb/datadriven" "github.com/cockroachdb/errors" "github.com/cockroachdb/pebble/internal/base" + "github.com/cockroachdb/pebble/internal/invalidating" "github.com/cockroachdb/pebble/internal/keyspan" "github.com/cockroachdb/pebble/internal/manifest" "github.com/cockroachdb/pebble/internal/private" "github.com/cockroachdb/pebble/internal/rangedel" + "github.com/cockroachdb/pebble/internal/testkeys" "github.com/cockroachdb/pebble/objstorage/objstorageprovider" "github.com/cockroachdb/pebble/sstable" "github.com/cockroachdb/pebble/vfs" @@ -85,7 +87,7 @@ func (f *failMerger) Close() error { func TestCheckLevelsCornerCases(t *testing.T) { memFS := vfs.NewMem() var levels [][]*fileMetadata - formatKey := DefaultComparer.FormatKey + formatKey := testkeys.Comparer.FormatKey // Indexed by fileNum var readers []*sstable.Reader defer func() { @@ -106,8 +108,9 @@ func TestCheckLevelsCornerCases(t *testing.T) { if err != nil { return iterSet{}, err } + return iterSet{ - point: iter, + point: invalidating.MaybeWrapIfInvariants(iter), rangeDeletion: rangeDelIter, }, nil } @@ -143,7 +146,7 @@ func TestCheckLevelsCornerCases(t *testing.T) { largestKey := base.ParseInternalKey(keys[1]) m := (&fileMetadata{ FileNum: fileNum, - }).ExtendPointKeyBounds(DefaultComparer.Compare, smallestKey, largestKey) + }).ExtendPointKeyBounds(testkeys.Comparer.Compare, smallestKey, largestKey) m.InitPhysicalBacking() *li = append(*li, m) @@ -157,7 +160,10 @@ func TestCheckLevelsCornerCases(t *testing.T) { return err.Error() } writeUnfragmented := false - w := sstable.NewWriter(objstorageprovider.NewFileWritable(f), sstable.WriterOptions{}) + w := sstable.NewWriter(objstorageprovider.NewFileWritable(f), sstable.WriterOptions{ + TableFormat: FormatNewest.MaxTableFormat(), + Comparer: testkeys.Comparer, + }) for _, arg := range d.CmdArgs { switch arg.Key { case "disable-key-order-checks": @@ -170,7 +176,7 @@ func TestCheckLevelsCornerCases(t *testing.T) { } var tombstones []keyspan.Span frag := keyspan.Fragmenter{ - Cmp: DefaultComparer.Compare, + Cmp: testkeys.Comparer.Compare, Format: formatKey, Emit: func(fragmented keyspan.Span) { tombstones = append(tombstones, fragmented) @@ -214,7 +220,7 @@ func TestCheckLevelsCornerCases(t *testing.T) { return err.Error() } cacheOpts := private.SSTableCacheOpts(0, base.DiskFileNum(fileNum-1)).(sstable.ReaderOption) - r, err := sstable.NewReader(readable, sstable.ReaderOptions{}, cacheOpts) + r, err := sstable.NewReader(readable, sstable.ReaderOptions{Comparer: testkeys.Comparer}, cacheOpts) if err != nil { return err.Error() } @@ -253,12 +259,12 @@ func TestCheckLevelsCornerCases(t *testing.T) { files[i+1] = levels[i] } version := manifest.NewVersion( - base.DefaultComparer, + testkeys.Comparer, 0, files) readState := &readState{current: version} c := &checkConfig{ - comparer: DefaultComparer, + comparer: testkeys.Comparer, readState: readState, newIters: newIters, seqNum: InternalKeySeqNumMax, diff --git a/testdata/level_checker b/testdata/level_checker index 5fdf7dbf898..54b87c62109 100644 --- a/testdata/level_checker +++ b/testdata/level_checker @@ -356,3 +356,21 @@ Level 1 check merger=fail-merger ---- merge processing error on key a#9,SINGLEDEL in L1: fileNum=000033: finish failed + +# Test a case where we pause at a range deletion end boundary at the end of a +# file and the last point key of the same file has its value stored out-of-band +# in a value block. + +define +L +a@9.SET.9 f.RANGEDEL.72057594037927935 +a@9.SET.9:a9 a@6.SET.6:a6 a.RANGEDEL.5:f +f@6.SET.6 f@6.SET.6 +f@6.SET.6:f6 +---- +Level 1 + file 0: [a@9#9,SET-f#72057594037927935,RANGEDEL] + file 1: [f@6#6,SET-f@6#6,SET] + +check +----