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 cockroachdb#3556.

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

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,

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
+[email protected] f.RANGEDEL.72057594037927935
+[email protected]:a9 [email protected]:a6 a.RANGEDEL.5:f
+[email protected] [email protected]
+[email protected]:f6
+----
+Level 1
+  file 0: [a@9#9,SET-f#72057594037927935,RANGEDEL]
+  file 1: [f@6#6,SET-f@6#6,SET]
+
+check
+----