From 98ecdf35891cfd65d7cf01dc228c3d5d0dc82f6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gy=C3=B6rgy=20Krajcsovits?= Date: Tue, 6 Aug 2024 16:51:20 +0200 Subject: [PATCH] Fix corrupting spans via iterator sharing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Iterator may share spans without copy, so we always have to make a copy before modification - copy-on-write. Signed-off-by: György Krajcsovits --- tsdb/chunkenc/float_histogram.go | 5 +++-- tsdb/chunkenc/float_histogram_test.go | 4 ++++ tsdb/chunkenc/histogram.go | 5 +++-- tsdb/chunkenc/histogram_test.go | 4 ++++ 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tsdb/chunkenc/float_histogram.go b/tsdb/chunkenc/float_histogram.go index 2af8dc507..a5f123bc9 100644 --- a/tsdb/chunkenc/float_histogram.go +++ b/tsdb/chunkenc/float_histogram.go @@ -782,9 +782,10 @@ func (a *FloatHistogramAppender) AppendFloatHistogram(prev *FloatHistogramAppend // of the chunk. if len(pForwardInserts) == 0 && len(nForwardInserts) == 0 { // No new chunks from the histogram, so the spans of the appender can accommodate the new buckets. - h.PositiveSpans = resize(h.PositiveSpans, len(a.pSpans)) + // However we need to make a copy in case the input is sharing spans from an iterator. + h.PositiveSpans = make([]histogram.Span, len(a.pSpans)) copy(h.PositiveSpans, a.pSpans) - h.NegativeSpans = resize(h.NegativeSpans, len(a.nSpans)) + h.NegativeSpans = make([]histogram.Span, len(a.nSpans)) copy(h.NegativeSpans, a.nSpans) } else { // Spans need pre-adjusting to accommodate the new buckets. diff --git a/tsdb/chunkenc/float_histogram_test.go b/tsdb/chunkenc/float_histogram_test.go index 41e76ef59..689696f5a 100644 --- a/tsdb/chunkenc/float_histogram_test.go +++ b/tsdb/chunkenc/float_histogram_test.go @@ -411,6 +411,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) { {Offset: 3, Length: 2}, {Offset: 5, Length: 1}, } + savedH2Spans := h2.PositiveSpans h2.PositiveBuckets = []float64{7, 4, 3, 5, 2} posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2) @@ -426,6 +427,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) { // Check that h2 was recoded. require.Equal(t, []float64{7, 0, 4, 3, 5, 0, 2}, h2.PositiveBuckets) require.Equal(t, emptyBucketH.PositiveSpans, h2.PositiveSpans) + require.NotEqual(t, savedH2Spans, h2.PositiveSpans, "recoding must make a copy") } { // New histogram that has new buckets AND buckets missing but the buckets missing were empty. @@ -439,6 +441,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) { {Offset: 3, Length: 2}, {Offset: 5, Length: 2}, } + savedH2Spans := h2.PositiveSpans h2.PositiveBuckets = []float64{7, 4, 3, 5, 2, 3} posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2) @@ -460,6 +463,7 @@ func TestFloatHistogramChunkAppendable(t *testing.T) { {Offset: 3, Length: 1}, // Added empty bucket. {Offset: 1, Length: 2}, // Existing + the extra bucket. }, h2.PositiveSpans) + require.NotEqual(t, savedH2Spans, h2.PositiveSpans, "recoding must make a copy") } { // New histogram that has a counter reset while buckets are same. diff --git a/tsdb/chunkenc/histogram.go b/tsdb/chunkenc/histogram.go index bdf4344af..fafae48d3 100644 --- a/tsdb/chunkenc/histogram.go +++ b/tsdb/chunkenc/histogram.go @@ -816,9 +816,10 @@ func (a *HistogramAppender) AppendHistogram(prev *HistogramAppender, t int64, h // of the chunk. if len(pForwardInserts) == 0 && len(nForwardInserts) == 0 { // No new chunks from the histogram, so the spans of the appender can accommodate the new buckets. - h.PositiveSpans = resize(h.PositiveSpans, len(a.pSpans)) + // However we need to make a copy in case the input is sharing spans from an iterator. + h.PositiveSpans = make([]histogram.Span, len(a.pSpans)) copy(h.PositiveSpans, a.pSpans) - h.NegativeSpans = resize(h.NegativeSpans, len(a.nSpans)) + h.NegativeSpans = make([]histogram.Span, len(a.nSpans)) copy(h.NegativeSpans, a.nSpans) } else { // Spans need pre-adjusting to accommodate the new buckets. diff --git a/tsdb/chunkenc/histogram_test.go b/tsdb/chunkenc/histogram_test.go index d44be69df..59187ed17 100644 --- a/tsdb/chunkenc/histogram_test.go +++ b/tsdb/chunkenc/histogram_test.go @@ -428,6 +428,7 @@ func TestHistogramChunkAppendable(t *testing.T) { {Offset: 4, Length: 1}, {Offset: 1, Length: 1}, } + savedH2Spans := h2.PositiveSpans h2.PositiveBuckets = []int64{7, -5, 1, 0, 1} // counts: 7, 2, 3, 3, 4 (total 18) posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2) @@ -443,6 +444,7 @@ func TestHistogramChunkAppendable(t *testing.T) { // Check that h2 was recoded. require.Equal(t, []int64{7, -7, 2, 1, -3, 3, 1}, h2.PositiveBuckets) // counts: 7, 0, 2, 3 , 0, 3, 4 (total 18) require.Equal(t, emptyBucketH.PositiveSpans, h2.PositiveSpans) + require.NotEqual(t, savedH2Spans, h2.PositiveSpans, "recoding must make a copy") } { // New histogram that has new buckets AND buckets missing but the buckets missing were empty. @@ -457,6 +459,7 @@ func TestHistogramChunkAppendable(t *testing.T) { {Offset: 4, Length: 1}, {Offset: 1, Length: 2}, } + savedH2Spans := h2.PositiveSpans h2.PositiveBuckets = []int64{7, -5, 1, 0, 1, 1} // counts: 7, 2, 3, 3, 4, 5 (total 23) posInterjections, negInterjections, backwardPositiveInserts, backwardNegativeInserts, ok, cr := hApp.appendable(h2) @@ -478,6 +481,7 @@ func TestHistogramChunkAppendable(t *testing.T) { {Offset: 3, Length: 1}, // Existing - offset adjusted. {Offset: 1, Length: 2}, // Existing. }, h2.PositiveSpans) + require.NotEqual(t, savedH2Spans, h2.PositiveSpans, "recoding must make a copy") } { // New histogram that has a counter reset while buckets are same.