From 4f5cb17042c5fc299724748b918f07e67361b8b7 Mon Sep 17 00:00:00 2001
From: Aliaksandr Valialkin <valyala@victoriametrics.com>
Date: Wed, 31 Jan 2024 10:25:53 +0200
Subject: [PATCH] app/vmselect/netstorage: properly handle the case when an
 empty brsPool points to the end of brs.brs

This case is possible after a new brsPool is allocated. The fix is to verify whether len(brsPool) >= len(brs.brs)
before trying to append a new item to brsPool and sharing its contents with brs.brs.

Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5733
---
 app/vmselect/netstorage/netstorage.go | 23 ++++++++++++++++++-----
 docs/CHANGELOG.md                     |  1 +
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/app/vmselect/netstorage/netstorage.go b/app/vmselect/netstorage/netstorage.go
index 0faa59c813..5e855892e8 100644
--- a/app/vmselect/netstorage/netstorage.go
+++ b/app/vmselect/netstorage/netstorage.go
@@ -1382,10 +1382,23 @@ type blockAddrs struct {
 	addrs []tmpBlockAddr
 }
 
-func haveSameBlockAddrTails(a, b []tmpBlockAddr) bool {
-	sha := (*reflect.SliceHeader)(unsafe.Pointer(&a))
-	shb := (*reflect.SliceHeader)(unsafe.Pointer(&b))
-	return sha.Data+uintptr(sha.Len)*unsafe.Sizeof(tmpBlockAddr{}) == shb.Data+uintptr(shb.Len)*unsafe.Sizeof(tmpBlockAddr{})
+// canAppendToBlockAddrPool returns true if a points to the pool and the last item in a is the last item in the pool.
+//
+// In this case it is safe appending an item to the pool and then updating the a, so it refers to the extended slice.
+//
+// True is also returned if a is nil, since in this case it is safe appending an item to the pool and pointing a
+// to the last item in the pool.
+func canAppendToBlockAddrPool(pool, a []tmpBlockAddr) bool {
+	if a == nil {
+		return true
+	}
+	if len(a) > len(pool) {
+		// a doesn't belong to pool
+		return false
+	}
+	shPool := (*reflect.SliceHeader)(unsafe.Pointer(&pool))
+	shA := (*reflect.SliceHeader)(unsafe.Pointer(&a))
+	return shPool.Data+uintptr(shPool.Len)*unsafe.Sizeof(tmpBlockAddr{}) == shA.Data+uintptr(shA.Len)*unsafe.Sizeof(tmpBlockAddr{})
 }
 
 func (tbfwLocal *tmpBlocksFileWrapperShard) newBlockAddrs() int {
@@ -1442,7 +1455,7 @@ func (tbfw *tmpBlocksFileWrapper) RegisterAndWriteBlock(mb *storage.MetricBlock,
 		addrsPool = make([]tmpBlockAddr, 0, maxFastAllocBlockSize/unsafe.Sizeof(tmpBlockAddr{}))
 		tbfwLocal.addrsPool = addrsPool
 	}
-	if addrs.addrs == nil || haveSameBlockAddrTails(addrs.addrs, addrsPool) {
+	if canAppendToBlockAddrPool(addrsPool, addrs.addrs) {
 		// It is safe appending addr to addrsPool, since there are no other items added there yet.
 		addrsPool = append(addrsPool, addr)
 		tbfwLocal.addrsPool = addrsPool
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index e546bb2774..a7b8bdc569 100644
--- a/docs/CHANGELOG.md
+++ b/docs/CHANGELOG.md
@@ -28,6 +28,7 @@ The sandbox cluster installation is running under the constant load generated by
 
 ## tip
 
+* BUGFIX: fix `runtime error: slice bounds out of range` panic, which can occur during query execution. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5733). The bug has been introduced in `v1.97.0`.
 * BUGFIX: [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html): properly handle `avg_over_time({some_filter}[d]) keep_metric_names` queries, where [`some_filter`](https://docs.victoriametrics.com/keyconcepts/#filtering) matches multiple time series with multiple names, while `d` is bigger or equal to `3h`. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5556).
 * BUGFIX: dashboards/single: fix typo in query for `version` annotation which falsely produced many version change events.