From 4959a9189af91620244ad614183534bb15c1d9a2 Mon Sep 17 00:00:00 2001
From: Rahul Aggarwal <rahul.aggarwal@cockroachlabs.com>
Date: Thu, 29 Jun 2023 13:42:20 -0400
Subject: [PATCH] manifest: Added Virtual SSTable Metadata Bounds Checking

Added a check inside of `FileMetadata.ValidateVirtual()` to ensure that
the keys returned from a virtual sstable are within the bounds of the
Smallest and Largest metadata keys.

Informs: #2593
---
 db.go               | 36 ++++++++++++++++++++++++++++++++++++
 ingest.go           |  7 +++++++
 table_cache_test.go |  2 ++
 version_set_test.go |  4 ++++
 4 files changed, 49 insertions(+)

diff --git a/db.go b/db.go
index bdccfb8cfe9..9717e26b6f5 100644
--- a/db.go
+++ b/db.go
@@ -2503,3 +2503,39 @@ func (d *DB) SetCreatorID(creatorID uint64) error {
 func (d *DB) ObjProvider() objstorage.Provider {
 	return d.objProvider
 }
+
+func (d *DB) checkVirtualBounds(m *fileMetadata) {
+	if !invariants.Enabled {
+		return
+	}
+
+	it, rangeIter, err := d.newIters(context.TODO(), m, nil, internalIterOpts{})
+	if err != nil {
+		panic(fmt.Sprintf("pebble: error creating iterator :%s", err.Error()))
+	}
+
+	// Check that virtual sstable bounds are tight from both ends.
+	first, _ := it.First()
+	if d.cmp(first.UserKey, m.Smallest.UserKey) != 0 {
+		panic(fmt.Sprintf("pebble: virtual sstable %s lower bound is not tight: %s != %s",
+			m.FileNum, d.opts.Comparer.FormatKey(m.Smallest.UserKey), d.opts.Comparer.FormatKey(first.UserKey)))
+	}
+	last, _ := it.Last()
+	if d.cmp(last.UserKey, m.Largest.UserKey) != 0 {
+		panic(fmt.Sprintf("pebble: virtual sstable %s upper bound is not tight: %s != %s",
+			m.FileNum, d.opts.Comparer.FormatKey(m.Largest.UserKey), d.opts.Comparer.FormatKey(last.UserKey)))
+	}
+
+	// Check that iterator keys are within bounds.
+	for key, _ := it.First(); key != nil; key, _ = it.Next() {
+		if d.cmp(key.UserKey, m.Smallest.UserKey) < 0 || d.cmp(key.UserKey, m.Largest.UserKey) > 0 {
+			panic("pebble: virtual sstable key is not within bounds")
+		}
+	}
+	if err := it.Close(); err != nil {
+		panic(fmt.Sprintf("pebble: error closing virtual sstable iterator: %s", err.Error()))
+	}
+	if rangeIter != nil {
+		rangeIter.Close()
+	}
+}
diff --git a/ingest.go b/ingest.go
index 07e55eb3d70..1611da566c5 100644
--- a/ingest.go
+++ b/ingest.go
@@ -1097,6 +1097,9 @@ func (d *DB) ingest(
 	if err := ingestLink(jobID, d.opts, d.objProvider, loadResult, shared); err != nil {
 		return IngestOperationStats{}, err
 	}
+	for _, sharedMeta := range loadResult.sharedMeta {
+		d.checkVirtualBounds(sharedMeta)
+	}
 	// Make the new tables durable. We need to do this at some point before we
 	// update the MANIFEST (via logAndApply), otherwise a crash can have the
 	// tables referenced in the MANIFEST, but not present in the provider.
@@ -1490,6 +1493,8 @@ func (d *DB) excise(
 			if err := leftFile.Validate(d.cmp, d.opts.Comparer.FormatKey); err != nil {
 				return nil, err
 			}
+			leftFile.ValidateVirtual(m)
+			d.checkVirtualBounds(leftFile)
 			ve.NewFiles = append(ve.NewFiles, newFileEntry{Level: level, Meta: leftFile})
 			ve.CreatedBackingTables = append(ve.CreatedBackingTables, leftFile.FileBacking)
 			backingTableCreated = true
@@ -1597,6 +1602,8 @@ func (d *DB) excise(
 			// for it here.
 			rightFile.Size = 1
 		}
+		rightFile.ValidateVirtual(m)
+		d.checkVirtualBounds(rightFile)
 		ve.NewFiles = append(ve.NewFiles, newFileEntry{Level: level, Meta: rightFile})
 		if !backingTableCreated {
 			ve.CreatedBackingTables = append(ve.CreatedBackingTables, rightFile.FileBacking)
diff --git a/table_cache_test.go b/table_cache_test.go
index 5efca158e60..bf90f3cfc2b 100644
--- a/table_cache_test.go
+++ b/table_cache_test.go
@@ -324,7 +324,9 @@ func TestVirtualReadsWiring(t *testing.T) {
 	v2.SmallestPointKey = v2.Smallest
 
 	v1.ValidateVirtual(parentFile)
+	d.checkVirtualBounds(v1)
 	v2.ValidateVirtual(parentFile)
+	d.checkVirtualBounds(v2)
 
 	// Write the version edit.
 	fileMetrics := func(ve *versionEdit) map[int]*LevelMetrics {
diff --git a/version_set_test.go b/version_set_test.go
index 3bf2b929dfe..beedc1b7fe5 100644
--- a/version_set_test.go
+++ b/version_set_test.go
@@ -100,7 +100,9 @@ func TestLatestRefCounting(t *testing.T) {
 	m2.SmallestPointKey = m2.Smallest
 
 	m1.ValidateVirtual(f)
+	d.checkVirtualBounds(m1)
 	m2.ValidateVirtual(f)
+	d.checkVirtualBounds(m2)
 
 	fileMetrics := func(ve *versionEdit) map[int]*LevelMetrics {
 		metrics := newFileMetrics(ve.NewFiles)
@@ -282,7 +284,9 @@ func TestVirtualSSTableManifestReplay(t *testing.T) {
 	m2.Stats.NumEntries = 1
 
 	m1.ValidateVirtual(f)
+	d.checkVirtualBounds(m1)
 	m2.ValidateVirtual(f)
+	d.checkVirtualBounds(m2)
 
 	fileMetrics := func(ve *versionEdit) map[int]*LevelMetrics {
 		metrics := newFileMetrics(ve.NewFiles)