From 93bc3721fc56c919c5cf5e30591fc9e60c1339fc Mon Sep 17 00:00:00 2001 From: Piotr <17101802+thampiotr@users.noreply.github.com> Date: Thu, 31 Jul 2025 13:57:27 +0100 Subject: [PATCH] Fix memory corruption in name and unit labels Signed-off-by: Piotr <17101802+thampiotr@users.noreply.github.com> --- model/labels/labels_dedupelabels.go | 8 ++++++- model/labels/labels_slicelabels.go | 6 +++++ model/labels/labels_stringlabels.go | 8 ++++++- model/textparse/protobufparse.go | 34 +++++++++++++++++++---------- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/model/labels/labels_dedupelabels.go b/model/labels/labels_dedupelabels.go index edc6ff8e82..d649a4c223 100644 --- a/model/labels/labels_dedupelabels.go +++ b/model/labels/labels_dedupelabels.go @@ -787,7 +787,7 @@ func (b *ScratchBuilder) Add(name, value string) { } // Add a name/value pair, using []byte instead of string to reduce memory allocations. -// The values must remain live until Labels() is called. +// The values must remain live until Labels() or Overwrite() is called. func (b *ScratchBuilder) UnsafeAddBytes(name, value []byte) { b.add = append(b.add, Label{Name: yoloString(name), Value: yoloString(value)}) } @@ -830,6 +830,12 @@ func (b *ScratchBuilder) Overwrite(ls *Labels) { ls.data = yoloString(b.overwriteBuffer) } +// UnsafeString must be used when passing to Add strings that are reusing buffers via the unsafe package. +// For example, b.Add("__name__", labels.UnsafeString(yoloString(buf))). +func UnsafeString(s string) string { + return s // No-op - the call to Labels() or Overwrite() will create a copy +} + // SizeOfLabels returns the approximate space required for n copies of a label. func SizeOfLabels(name, value string, n uint64) uint64 { return uint64(len(name)+len(value)) + n*4 // Assuming most symbol-table entries are 2 bytes long. diff --git a/model/labels/labels_slicelabels.go b/model/labels/labels_slicelabels.go index a6e5654fa7..8494de7307 100644 --- a/model/labels/labels_slicelabels.go +++ b/model/labels/labels_slicelabels.go @@ -506,6 +506,12 @@ func (b *ScratchBuilder) Overwrite(ls *Labels) { *ls = append((*ls)[:0], b.add...) } +// UnsafeString must be used when passing to Add strings that are reusing buffers via the unsafe package. +// For example, b.Add("__name__", labels.UnsafeString(yoloString(buf))). +func UnsafeString(s string) string { + return strings.Clone(s) // We need to clone such strings for slicelabels implementation. +} + // SizeOfLabels returns the approximate space required for n copies of a label. func SizeOfLabels(name, value string, n uint64) uint64 { return (uint64(len(name)) + uint64(unsafe.Sizeof(name)) + uint64(len(value)) + uint64(unsafe.Sizeof(value))) * n diff --git a/model/labels/labels_stringlabels.go b/model/labels/labels_stringlabels.go index 4b9bfd15af..4aae327462 100644 --- a/model/labels/labels_stringlabels.go +++ b/model/labels/labels_stringlabels.go @@ -619,7 +619,7 @@ func (b *ScratchBuilder) Add(name, value string) { } // UnsafeAddBytes adds a name/value pair using []byte instead of string to reduce memory allocations. -// The values must remain live until Labels() is called. +// The values must remain live until Labels() or Overwrite() is called. func (b *ScratchBuilder) UnsafeAddBytes(name, value []byte) { b.add = append(b.add, Label{Name: yoloString(name), Value: yoloString(value)}) } @@ -680,6 +680,12 @@ func (b *ScratchBuilder) SetSymbolTable(_ *SymbolTable) { // no-op } +// UnsafeString must be used when passing to Add strings that are reusing buffers via the unsafe package. +// For example, b.Add("__name__", labels.UnsafeString(yoloString(buf))). +func UnsafeString(s string) string { + return s // No-op - the call to Labels() or Overwrite() will create a copy +} + // SizeOfLabels returns the approximate space required for n copies of a label. func SizeOfLabels(name, value string, n uint64) uint64 { return uint64(labelSize(&Label{Name: name, Value: value})) * n diff --git a/model/textparse/protobufparse.go b/model/textparse/protobufparse.go index 2ca6c03af7..fcbf8531bd 100644 --- a/model/textparse/protobufparse.go +++ b/model/textparse/protobufparse.go @@ -555,13 +555,19 @@ func (p *ProtobufParser) Next() (Entry, error) { func (p *ProtobufParser) onSeriesOrHistogramUpdate() error { p.builder.Reset() + name, safe := p.getMagicName() + if !safe { + name = labels.UnsafeString(name) // Make sure the name is safe to use in labels. + } + if p.enableTypeAndUnitLabels { _, typ := p.Type() m := schema.Metadata{ - Name: p.getMagicName(), + Name: name, Type: typ, - Unit: p.dec.GetUnit(), + // After the next call to dec.NextMetricFamily, the unit value becomes invalid, use labels.UnsafeString. + Unit: labels.UnsafeString(p.dec.GetUnit()), } m.AddToLabels(&p.builder) if err := p.dec.Label(schema.IgnoreOverriddenMetadataLabelsScratchBuilder{ @@ -571,7 +577,7 @@ func (p *ProtobufParser) onSeriesOrHistogramUpdate() error { return err } } else { - p.builder.Add(labels.MetricName, p.getMagicName()) + p.builder.Add(labels.MetricName, name) if err := p.dec.Label(&p.builder); err != nil { return err } @@ -600,24 +606,28 @@ func (p *ProtobufParser) onSeriesOrHistogramUpdate() error { return nil } -// getMagicName usually just returns p.mf.GetType() but adds a magic suffix -// ("_count", "_sum", "_bucket") if needed according to the current parser -// state. -func (p *ProtobufParser) getMagicName() string { +// getMagicName returns the name of the metric. It usually just returns +// p.mf.GetName() but adds a magic suffix ("_count", "_sum", "_bucket") if +// needed according to the current parser state. +// The second return value is set to true if the metric name is safe to use +// in labels without wrapping with labels.UnsafeString. When it is false, the +// returned name will become invalid with the next call to dec.NextMetricFamily +// and must be wrapped with labels.UnsafeString when used in labels. +func (p *ProtobufParser) getMagicName() (string, bool) { t := p.dec.GetType() if p.state == EntryHistogram || (t != dto.MetricType_HISTOGRAM && t != dto.MetricType_GAUGE_HISTOGRAM && t != dto.MetricType_SUMMARY) { - return p.dec.GetName() + return p.dec.GetName(), false } if p.fieldPos == -2 { - return p.dec.GetName() + "_count" + return p.dec.GetName() + "_count", true } if p.fieldPos == -1 { - return p.dec.GetName() + "_sum" + return p.dec.GetName() + "_sum", true } if t == dto.MetricType_HISTOGRAM || t == dto.MetricType_GAUGE_HISTOGRAM { - return p.dec.GetName() + "_bucket" + return p.dec.GetName() + "_bucket", true } - return p.dec.GetName() + return p.dec.GetName(), false } // getMagicLabel returns if a magic label ("quantile" or "le") is needed and, if