From c6618729c9054e00c126e03338f670109d8de7af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Thu, 11 May 2023 21:02:02 +0200 Subject: [PATCH] Fix HistogramAppender.Appendable array out of bound error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code did not handle spans with 0 length properly. Spans with length zero are now skipped in the comparison. Span index check not done against length-1, since length is a unit32, thus subtracting 1 leads to 2^32, not -1. Fixes and unit tests for both integer and float histograms added. Signed-off-by: György Krajcsovits --- tsdb/chunkenc/float_histogram.go | 8 ++-- tsdb/chunkenc/float_histogram_test.go | 58 ++++++++++++++++++++++++ tsdb/chunkenc/histogram.go | 8 ++-- tsdb/chunkenc/histogram_meta.go | 7 +++ tsdb/chunkenc/histogram_test.go | 63 +++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 8 deletions(-) diff --git a/tsdb/chunkenc/float_histogram.go b/tsdb/chunkenc/float_histogram.go index 0349de9abe..d49885a1c5 100644 --- a/tsdb/chunkenc/float_histogram.go +++ b/tsdb/chunkenc/float_histogram.go @@ -358,9 +358,9 @@ func counterResetInAnyFloatBucket(oldBuckets []xorValue, newBuckets []float64, o if oldIdx <= newIdx { // Moving ahead old bucket and span by 1 index. - if oldInsideSpanIdx == oldSpans[oldSpanSliceIdx].Length-1 { + if oldInsideSpanIdx+1 >= oldSpans[oldSpanSliceIdx].Length { // Current span is over. - oldSpanSliceIdx++ + oldSpanSliceIdx = nextNonEmptySpanSliceIdx(oldSpanSliceIdx, oldSpans) oldInsideSpanIdx = 0 if oldSpanSliceIdx >= len(oldSpans) { // All old spans are over. @@ -377,9 +377,9 @@ func counterResetInAnyFloatBucket(oldBuckets []xorValue, newBuckets []float64, o if oldIdx > newIdx { // Moving ahead new bucket and span by 1 index. - if newInsideSpanIdx == newSpans[newSpanSliceIdx].Length-1 { + if newInsideSpanIdx+1 >= newSpans[newSpanSliceIdx].Length { // Current span is over. - newSpanSliceIdx++ + newSpanSliceIdx = nextNonEmptySpanSliceIdx(newSpanSliceIdx, newSpans) newInsideSpanIdx = 0 if newSpanSliceIdx >= len(newSpans) { // All new spans are over. diff --git a/tsdb/chunkenc/float_histogram_test.go b/tsdb/chunkenc/float_histogram_test.go index 90c16d1ea9..c662e5ffa1 100644 --- a/tsdb/chunkenc/float_histogram_test.go +++ b/tsdb/chunkenc/float_histogram_test.go @@ -365,6 +365,64 @@ func TestFloatHistogramChunkAppendable(t *testing.T) { } } +func TestFloatHistogramChunkAppendableWithEmptySpan(t *testing.T) { + h1 := &histogram.FloatHistogram{ + Schema: 0, + Count: 21, + Sum: 1234.5, + ZeroThreshold: 0.001, + ZeroCount: 4, + PositiveSpans: []histogram.Span{ + {Offset: 0, Length: 4}, + {Offset: 0, Length: 0}, + {Offset: 0, Length: 3}, + }, + PositiveBuckets: []float64{1, 2, 1, 1, 1, 1, 1}, + NegativeSpans: []histogram.Span{ + {Offset: 1, Length: 4}, + {Offset: 2, Length: 0}, + {Offset: 2, Length: 3}, + }, + NegativeBuckets: []float64{1, 2, 1, 2, 2, 2, 2}, + } + h2 := &histogram.FloatHistogram{ + Schema: 0, + Count: 37, + Sum: 2345.6, + ZeroThreshold: 0.001, + ZeroCount: 5, + PositiveSpans: []histogram.Span{ + {Offset: 0, Length: 4}, + {Offset: 0, Length: 0}, + {Offset: 0, Length: 3}, + }, + PositiveBuckets: []float64{1, 3, 1, 2, 1, 1, 1}, + NegativeSpans: []histogram.Span{ + {Offset: 1, Length: 4}, + {Offset: 2, Length: 0}, + {Offset: 2, Length: 3}, + }, + NegativeBuckets: []float64{1, 4, 2, 7, 5, 5, 2}, + } + + c := Chunk(NewFloatHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + require.Equal(t, 0, c.NumSamples()) + + app.AppendFloatHistogram(1, h1) + require.Equal(t, 1, c.NumSamples()) + hApp, _ := app.(*FloatHistogramAppender) + + pI, nI, okToAppend, counterReset := hApp.Appendable(h2) + require.Empty(t, pI) + require.Empty(t, nI) + require.True(t, okToAppend) + require.False(t, counterReset) +} + func TestFloatHistogramChunkAppendableGauge(t *testing.T) { c := Chunk(NewFloatHistogramChunk()) diff --git a/tsdb/chunkenc/histogram.go b/tsdb/chunkenc/histogram.go index f9a63d18f7..2350b2af27 100644 --- a/tsdb/chunkenc/histogram.go +++ b/tsdb/chunkenc/histogram.go @@ -386,9 +386,9 @@ func counterResetInAnyBucket(oldBuckets, newBuckets []int64, oldSpans, newSpans if oldIdx <= newIdx { // Moving ahead old bucket and span by 1 index. - if oldInsideSpanIdx == oldSpans[oldSpanSliceIdx].Length-1 { + if oldInsideSpanIdx+1 >= oldSpans[oldSpanSliceIdx].Length { // Current span is over. - oldSpanSliceIdx++ + oldSpanSliceIdx = nextNonEmptySpanSliceIdx(oldSpanSliceIdx, oldSpans) oldInsideSpanIdx = 0 if oldSpanSliceIdx >= len(oldSpans) { // All old spans are over. @@ -405,9 +405,9 @@ func counterResetInAnyBucket(oldBuckets, newBuckets []int64, oldSpans, newSpans if oldIdx > newIdx { // Moving ahead new bucket and span by 1 index. - if newInsideSpanIdx == newSpans[newSpanSliceIdx].Length-1 { + if newInsideSpanIdx+1 >= newSpans[newSpanSliceIdx].Length { // Current span is over. - newSpanSliceIdx++ + newSpanSliceIdx = nextNonEmptySpanSliceIdx(newSpanSliceIdx, newSpans) newInsideSpanIdx = 0 if newSpanSliceIdx >= len(newSpans) { // All new spans are over. diff --git a/tsdb/chunkenc/histogram_meta.go b/tsdb/chunkenc/histogram_meta.go index 027eee1129..7a21bc20bd 100644 --- a/tsdb/chunkenc/histogram_meta.go +++ b/tsdb/chunkenc/histogram_meta.go @@ -487,3 +487,10 @@ func counterResetHint(crh CounterResetHeader, numRead uint16) histogram.CounterR return histogram.UnknownCounterReset } } + +// Handle pathological case of empty span when advancing span idx. +func nextNonEmptySpanSliceIdx(idx int, spans []histogram.Span) (newIdx int) { + for idx++; idx < len(spans) && spans[idx].Length == 0; idx++ { + } + return idx +} diff --git a/tsdb/chunkenc/histogram_test.go b/tsdb/chunkenc/histogram_test.go index 45f31a3b4d..9ef7e24a4c 100644 --- a/tsdb/chunkenc/histogram_test.go +++ b/tsdb/chunkenc/histogram_test.go @@ -14,6 +14,7 @@ package chunkenc import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -387,6 +388,64 @@ func TestHistogramChunkAppendable(t *testing.T) { } } +func TestHistogramChunkAppendableWithEmptySpan(t *testing.T) { + h1 := &histogram.Histogram{ + Schema: 0, + Count: 21, + Sum: 1234.5, + ZeroThreshold: 0.001, + ZeroCount: 4, + PositiveSpans: []histogram.Span{ + {Offset: 0, Length: 4}, + {Offset: 0, Length: 0}, + {Offset: 0, Length: 3}, + }, + PositiveBuckets: []int64{1, 1, -1, 0, 0, 0, 0}, + NegativeSpans: []histogram.Span{ + {Offset: 1, Length: 4}, + {Offset: 2, Length: 0}, + {Offset: 2, Length: 3}, + }, + NegativeBuckets: []int64{1, 1, -1, 1, 0, 0, 0}, + } + h2 := &histogram.Histogram{ + Schema: 0, + Count: 37, + Sum: 2345.6, + ZeroThreshold: 0.001, + ZeroCount: 5, + PositiveSpans: []histogram.Span{ + {Offset: 0, Length: 4}, + {Offset: 0, Length: 0}, + {Offset: 0, Length: 3}, + }, + PositiveBuckets: []int64{1, 2, -2, 1, -1, 0, 0}, + NegativeSpans: []histogram.Span{ + {Offset: 1, Length: 4}, + {Offset: 2, Length: 0}, + {Offset: 2, Length: 3}, + }, + NegativeBuckets: []int64{1, 3, -2, 5, -2, 0, -3}, + } + + c := Chunk(NewHistogramChunk()) + + // Create fresh appender and add the first histogram. + app, err := c.Appender() + require.NoError(t, err) + require.Equal(t, 0, c.NumSamples()) + + app.AppendHistogram(1, h1) + require.Equal(t, 1, c.NumSamples()) + hApp, _ := app.(*HistogramAppender) + + pI, nI, okToAppend, counterReset := hApp.Appendable(h2) + require.Empty(t, pI) + require.Empty(t, nI) + require.True(t, okToAppend) + require.False(t, counterReset) +} + func TestAtFloatHistogram(t *testing.T) { input := []histogram.Histogram{ { @@ -514,6 +573,10 @@ func TestAtFloatHistogram(t *testing.T) { app, err := chk.Appender() require.NoError(t, err) for i := range input { + if i > 0 { + _, _, okToAppend, _ := app.(*HistogramAppender).Appendable(&input[i]) + require.True(t, okToAppend, fmt.Sprintf("idx: %d", i)) + } app.AppendHistogram(int64(i), &input[i]) } it := chk.Iterator(nil)