From c9dd34ac6b202aa8e291804eb9bdc1d1325aee0f Mon Sep 17 00:00:00 2001 From: Julien Pivotto <291750+roidelapluie@users.noreply.github.com> Date: Tue, 31 Mar 2026 16:07:20 +0200 Subject: [PATCH] histogram, textparse: fix two panics in compactBuckets for malformed input Two cases in compactBuckets caused a panic when fed malformed histogram data (e.g. via a crafted protobuf message): 1. All spans have zero length: after the zero-length span removal pass, spans becomes empty. The subsequent loop called emptyBucketsHere(), which accessed spans[0] and panicked with index out of range. Fixed by the early return added in the previous commit (already on this branch via the roidelapluie/histogram-compact-zero-spans fix). 2. More buckets than spans describe: iSpan can reach len(spans) before all buckets are consumed, causing emptyBucketsHere() to access spans[iSpan] out of bounds. Fixed by adding iSpan < len(spans) to the loop guard. Both fixes in compactBuckets are defensive layers. The primary fix is in the protobuf parser: checkNativeHistogramConsistency now validates that span total length matches bucket count before calling Compact(), returning a proper error for malformed input instead of panicking. Found by FuzzParseProtobuf. Signed-off-by: Julien Pivotto <291750+roidelapluie@users.noreply.github.com> --- model/histogram/generic.go | 10 +++- model/histogram/histogram_test.go | 35 ++++++++++++ model/textparse/protobufparse.go | 39 +++++++++++++ model/textparse/protobufparse_test.go | 79 +++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) diff --git a/model/histogram/generic.go b/model/histogram/generic.go index 9ec9e9cd4b..287d914d64 100644 --- a/model/histogram/generic.go +++ b/model/histogram/generic.go @@ -332,10 +332,18 @@ func compactBuckets[IBC InternalBucketCount]( spans = spans[:iSpan] iSpan = 0 + // If all spans were zero-length, no buckets remain valid. + if len(spans) == 0 { + if compensationBuckets != nil { + compensationBuckets = compensationBuckets[:0] + } + return primaryBuckets[:0], compensationBuckets, spans + } + // Cut out empty buckets from start and end of spans, no matter // what. Also cut out empty buckets from the middle of a span but only // if there are more than maxEmptyBuckets consecutive empty buckets. - for iBucket < len(primaryBuckets) { + for iBucket < len(primaryBuckets) && iSpan < len(spans) { if deltaBuckets { currentBucketAbsolute += primaryBuckets[iBucket] } else { diff --git a/model/histogram/histogram_test.go b/model/histogram/histogram_test.go index a2b4c7c0a8..421227ff48 100644 --- a/model/histogram/histogram_test.go +++ b/model/histogram/histogram_test.go @@ -1338,6 +1338,41 @@ func TestHistogramCompact(t *testing.T) { CustomValues: []float64{5, 10, 15, 20}, }, }, + { + "all zero-length spans with non-empty buckets", + &Histogram{ + PositiveSpans: []Span{{0, 0}, {2, 0}}, + PositiveBuckets: []int64{1, 3}, + NegativeSpans: []Span{{1, 0}}, + NegativeBuckets: []int64{2}, + }, + 0, + &Histogram{ + PositiveSpans: []Span{}, + PositiveBuckets: []int64{}, + NegativeSpans: []Span{}, + NegativeBuckets: []int64{}, + }, + }, + { + // The loop guard prevents a panic; extra buckets beyond span + // coverage are preserved in the output. Callers must validate + // span/bucket consistency before invoking Compact. + "more buckets than spans account for", + &Histogram{ + PositiveSpans: []Span{{0, 1}, {2, 1}}, + PositiveBuckets: []int64{1, 2, 3, 4}, + NegativeSpans: []Span{{0, 1}}, + NegativeBuckets: []int64{5, 6}, + }, + 0, + &Histogram{ + PositiveSpans: []Span{{0, 1}, {2, 1}}, + PositiveBuckets: []int64{1, 2, 3, 4}, + NegativeSpans: []Span{{0, 1}}, + NegativeBuckets: []int64{5, 6}, + }, + }, { "eliminate multiple zero length spans with custom buckets", &Histogram{ diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index 3795c28589..57db889871 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -484,6 +484,9 @@ func (p *ProtobufParser) Next() (Entry, error) { p.fieldPos = -3 // We have not returned anything, let p.Next() increment it to -2. return p.Next() } + if err := checkNativeHistogramConsistency(p.dec.GetHistogram()); err != nil { + return EntryInvalid, fmt.Errorf("histogram %q: %w", p.dec.GetName(), err) + } p.state = EntryHistogram } else { p.state = EntrySeries @@ -527,6 +530,9 @@ func (p *ProtobufParser) Next() (Entry, error) { // it means we might need to do NHCB conversion. if t == dto.MetricType_HISTOGRAM || t == dto.MetricType_GAUGE_HISTOGRAM { if !isClassicHistogram { + if err := checkNativeHistogramConsistency(p.dec.GetHistogram()); err != nil { + return EntryInvalid, fmt.Errorf("histogram %q: %w", p.dec.GetName(), err) + } p.state = EntryHistogram } else if p.convertClassicHistogramsToNHCB { // We still need to spit out the NHCB. @@ -748,3 +754,36 @@ func (p *ProtobufParser) convertToNHCB(t dto.MetricType) (*histogram.Histogram, } return ch, cfh, nil } + +// checkNativeHistogramConsistency returns an error if the span bucket counts +// do not match the number of bucket values in a native histogram protobuf +// message. It catches malformed input before it reaches compactBuckets, where +// a mismatch would cause a panic. +func checkNativeHistogramConsistency(h *dto.Histogram) error { + isFloat := h.GetSampleCountFloat() > 0 || h.GetZeroCountFloat() > 0 + var positiveBuckets, negativeBuckets int + if isFloat { + positiveBuckets = len(h.GetPositiveCount()) + negativeBuckets = len(h.GetNegativeCount()) + } else { + positiveBuckets = len(h.GetPositiveDelta()) + negativeBuckets = len(h.GetNegativeDelta()) + } + if err := checkProtoSpanBucketConsistency("positive", h.GetPositiveSpan(), positiveBuckets); err != nil { + return err + } + return checkProtoSpanBucketConsistency("negative", h.GetNegativeSpan(), negativeBuckets) +} + +// checkProtoSpanBucketConsistency returns an error when the total length +// described by spans does not match numBuckets. +func checkProtoSpanBucketConsistency(side string, spans []dto.BucketSpan, numBuckets int) error { + var total int + for _, s := range spans { + total += int(s.GetLength()) + } + if total != numBuckets { + return fmt.Errorf("%s side: spans require %d buckets, have %d", side, total, numBuckets) + } + return nil +} diff --git a/model/textparse/protobufparse_test.go b/model/textparse/protobufparse_test.go index 12e4b959a7..246cbdf879 100644 --- a/model/textparse/protobufparse_test.go +++ b/model/textparse/protobufparse_test.go @@ -5937,6 +5937,85 @@ metric: < }) } +func TestProtobufParseHistogramSpanBucketMismatch(t *testing.T) { + for _, tc := range []struct { + name string + input string + }{ + { + name: "all zero-length positive spans with non-empty positive deltas", + input: `name: "test_histogram" +help: "Test." +type: HISTOGRAM +metric: < + histogram: < + sample_count: 2 + sample_sum: 1.0 + schema: 1 + zero_threshold: 0 + zero_count: 0 + positive_span: < + offset: 0 + length: 0 + > + positive_span: < + offset: 2 + length: 0 + > + positive_delta: 1 + positive_delta: 3 + > +> +`, + }, + { + name: "more positive deltas than positive spans account for", + input: `name: "test_histogram" +help: "Test." +type: HISTOGRAM +metric: < + histogram: < + sample_count: 10 + sample_sum: 1.0 + schema: 1 + zero_threshold: 0 + zero_count: 0 + positive_span: < + offset: 0 + length: 1 + > + positive_span: < + offset: 2 + length: 1 + > + positive_delta: 1 + positive_delta: 2 + positive_delta: 3 + positive_delta: 4 + > +> +`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + buf := metricFamiliesToProtobuf(t, []string{tc.input}) + p := NewProtobufParser(buf.Bytes(), false, false, false, false, labels.NewSymbolTable()) + var gotErr error + for { + _, err := p.Next() + if errors.Is(err, io.EOF) { + break + } + if err != nil { + gotErr = err + break + } + } + require.ErrorContains(t, gotErr, "positive side") + }) + } +} + func generateString(r *rand.Rand, firstRunes, restRunes []rune) string { result := make([]rune, 1+r.Intn(20)) for i := range result {