From eaed618db1e584b26bc01b0526bc7c844914cfdd Mon Sep 17 00:00:00 2001 From: Vishal Raj Date: Wed, 24 Jul 2024 12:30:39 +0100 Subject: [PATCH] Fix NPE due to reslice only initializing new slice elements (#327) --- .../modeldecoder/modeldecoderutil/http.go | 2 +- .../modeldecoderutil/keyvaluepb.go | 2 +- .../modeldecoder/modeldecoderutil/slice.go | 40 +++++++------- .../modeldecoderutil/slice_test.go | 29 ++++++++--- .../internal/modeldecoder/rumv3/decoder.go | 28 +++++++--- .../internal/modeldecoder/v2/decoder.go | 52 ++++++++++++++----- 6 files changed, 107 insertions(+), 46 deletions(-) diff --git a/input/elasticapm/internal/modeldecoder/modeldecoderutil/http.go b/input/elasticapm/internal/modeldecoder/modeldecoderutil/http.go index efe68842..6fd5759f 100644 --- a/input/elasticapm/internal/modeldecoder/modeldecoderutil/http.go +++ b/input/elasticapm/internal/modeldecoder/modeldecoderutil/http.go @@ -49,7 +49,7 @@ func HTTPHeadersToModelpb(h http.Header, out []*modelpb.HTTPHeader) []*modelpb.H if len(h) == 0 { return nil } - out = Reslice(out, len(h), modelpb.HTTPHeaderFromVTPool) + out = ResliceAndPopulateNil(out, len(h), modelpb.HTTPHeaderFromVTPool) i := 0 for k, v := range h { out[i].Key = k diff --git a/input/elasticapm/internal/modeldecoder/modeldecoderutil/keyvaluepb.go b/input/elasticapm/internal/modeldecoder/modeldecoderutil/keyvaluepb.go index c73d7bb5..e7cc4e17 100644 --- a/input/elasticapm/internal/modeldecoder/modeldecoderutil/keyvaluepb.go +++ b/input/elasticapm/internal/modeldecoder/modeldecoderutil/keyvaluepb.go @@ -28,7 +28,7 @@ func ToKv(m map[string]any, out []*modelpb.KeyValue) []*modelpb.KeyValue { return nil } - out = Reslice(out, len(m), modelpb.KeyValueFromVTPool) + out = ResliceAndPopulateNil(out, len(m), modelpb.KeyValueFromVTPool) i := 0 for k, v := range m { diff --git a/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice.go b/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice.go index 5b7a1674..d628e3ff 100644 --- a/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice.go +++ b/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice.go @@ -17,28 +17,30 @@ package modeldecoderutil -// Reslice increases the slice's capacity, if necessary, to match n. -// If specified, the newFn function is used to create the elements -// to populate the additional space appended to the slice. -func Reslice[Slice ~[]model, model any](slice Slice, n int, newFn func() model) Slice { - if diff := n - cap(slice); diff > 0 { - // start of the extra space - idx := cap(slice) +import ( + "slices" +) - // Grow the slice - // Note: append gives no guarantee on the capacity of the resulting slice - // and might overallocate as long as there's enough space for n elements. - slice = append([]model(slice)[:cap(slice)], make([]model, diff)...) - if newFn != nil { - // extend the slice to its capacity - slice = slice[:cap(slice)] +// Reslice returns a slice with length n. It will reallocate the +// slice if the capacity is not enough. +func Reslice[Slice ~[]model, model any](slice Slice, n int) Slice { + if n > cap(slice) { + slice = slices.Grow(slice, n-len(slice)) + } + return slice[:n] +} - // populate the extra space - for ; idx < len(slice); idx++ { - slice[idx] = newFn() +// ResliceAndPopulateNil ensures a slice of pointers has atleast +// capacity for n elements and populates any non-nil elements +// in the resulting slice with the value returned from newFn. +func ResliceAndPopulateNil[Slice ~[]*model, model any](slice Slice, n int, newFn func() *model) Slice { + slice = Reslice(slice, n) + if newFn != nil { + for i := 0; i < len(slice); i++ { + if slice[i] == nil { + slice[i] = newFn() } } } - - return slice[:n] + return slice } diff --git a/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice_test.go b/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice_test.go index 49224287..8a159056 100644 --- a/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice_test.go +++ b/input/elasticapm/internal/modeldecoder/modeldecoderutil/slice_test.go @@ -28,17 +28,33 @@ func TestReslice(t *testing.T) { var s []*modelpb.APMEvent originalSize := 10 - s = Reslice(s, originalSize, modelpb.APMEventFromVTPool) + s = Reslice(s, originalSize) + assert.Equal(t, originalSize, len(s)) + + downsize := 4 + s = Reslice(s, downsize) + assert.Equal(t, downsize, len(s)) + + upsize := 21 + s = Reslice(s, upsize) + assert.Equal(t, upsize, len(s)) +} + +func TestResliceAndPopulateNil(t *testing.T) { + var s []*modelpb.APMEvent + + originalSize := 10 + s = ResliceAndPopulateNil(s, originalSize, modelpb.APMEventFromVTPool) validateBackingArray(t, s, originalSize) assert.Equal(t, originalSize, len(s)) downsize := 4 - s = Reslice(s, downsize, nil) + s = ResliceAndPopulateNil(s, downsize, nil) validateBackingArray(t, s, downsize) assert.Equal(t, downsize, len(s)) upsize := 21 - s = Reslice(s, upsize, modelpb.APMEventFromVTPool) + s = ResliceAndPopulateNil(s, upsize, modelpb.APMEventFromVTPool) validateBackingArray(t, s, upsize) assert.Equal(t, upsize, len(s)) } @@ -49,9 +65,8 @@ func validateBackingArray(t *testing.T, out []*modelpb.APMEvent, expectedLen int // validate length assert.Equal(t, expectedLen, len(out)) - // validate backing array is fully populated - backing := out[:cap(out)] - for i := 0; i < cap(backing); i++ { - assert.NotNil(t, backing[i]) + // validate all elements of backing array are non-nil + for i := 0; i < len(out); i++ { + assert.NotNil(t, out[i]) } } diff --git a/input/elasticapm/internal/modeldecoder/rumv3/decoder.go b/input/elasticapm/internal/modeldecoder/rumv3/decoder.go index 34cd271d..b814d9ed 100644 --- a/input/elasticapm/internal/modeldecoder/rumv3/decoder.go +++ b/input/elasticapm/internal/modeldecoder/rumv3/decoder.go @@ -251,7 +251,11 @@ func mapToErrorModel(from *errorEvent, event *modelpb.APMEvent) { log.ParamMessage = from.Log.ParamMessage.Val } if len(from.Log.Stacktrace) > 0 { - log.Stacktrace = modeldecoderutil.Reslice(log.Stacktrace, len(from.Log.Stacktrace), modelpb.StacktraceFrameFromVTPool) + log.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + log.Stacktrace, + len(from.Log.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Log.Stacktrace, log.Stacktrace) } out.Log = log @@ -291,7 +295,11 @@ func mapToExceptionModel(from *errorException, out *modelpb.Exception) { out.Code = modeldecoderutil.ExceptionCodeString(from.Code.Val) } if len(from.Cause) > 0 { - out.Cause = modeldecoderutil.Reslice(out.Cause, len(from.Cause), modelpb.ExceptionFromVTPool) + out.Cause = modeldecoderutil.ResliceAndPopulateNil( + out.Cause, + len(from.Cause), + modelpb.ExceptionFromVTPool, + ) for i := 0; i < len(from.Cause); i++ { mapToExceptionModel(&from.Cause[i], out.Cause[i]) } @@ -307,7 +315,11 @@ func mapToExceptionModel(from *errorException, out *modelpb.Exception) { out.Module = from.Module.Val } if len(from.Stacktrace) > 0 { - out.Stacktrace = modeldecoderutil.Reslice(out.Stacktrace, len(from.Stacktrace), modelpb.StacktraceFrameFromVTPool) + out.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + out.Stacktrace, + len(from.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Stacktrace, out.Stacktrace) } if from.Type.IsSet() { @@ -676,7 +688,11 @@ func mapToSpanModel(from *span, event *modelpb.APMEvent) { out.RepresentativeCount = 1 / from.SampleRate.Val } if len(from.Stacktrace) > 0 { - out.Stacktrace = modeldecoderutil.Reslice(out.Stacktrace, len(from.Stacktrace), modelpb.StacktraceFrameFromVTPool) + out.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + out.Stacktrace, + len(from.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Stacktrace, out.Stacktrace) } if from.Sync.IsSet() { @@ -720,11 +736,11 @@ func mapToStracktraceModel(from []stacktraceFrame, out []*modelpb.StacktraceFram fr.Module = eventFrame.Module.Val } if len(eventFrame.PostContext) > 0 { - fr.PostContext = modeldecoderutil.Reslice(fr.PostContext, len(eventFrame.PostContext), nil) + fr.PostContext = modeldecoderutil.Reslice(fr.PostContext, len(eventFrame.PostContext)) copy(fr.PostContext, eventFrame.PostContext) } if len(eventFrame.PreContext) > 0 { - fr.PreContext = modeldecoderutil.Reslice(fr.PreContext, len(eventFrame.PreContext), nil) + fr.PreContext = modeldecoderutil.Reslice(fr.PreContext, len(eventFrame.PreContext)) copy(fr.PreContext, eventFrame.PreContext) } } diff --git a/input/elasticapm/internal/modeldecoder/v2/decoder.go b/input/elasticapm/internal/modeldecoder/v2/decoder.go index 7b74216d..a3d9b3ab 100644 --- a/input/elasticapm/internal/modeldecoder/v2/decoder.go +++ b/input/elasticapm/internal/modeldecoder/v2/decoder.go @@ -477,7 +477,11 @@ func mapToErrorModel(from *errorEvent, event *modelpb.APMEvent) { log.ParamMessage = from.Log.ParamMessage.Val } if len(from.Log.Stacktrace) > 0 { - log.Stacktrace = modeldecoderutil.Reslice(log.Stacktrace, len(from.Log.Stacktrace), modelpb.StacktraceFrameFromVTPool) + log.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + log.Stacktrace, + len(from.Log.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Log.Stacktrace, log.Stacktrace) } out.Log = log @@ -521,7 +525,11 @@ func mapToExceptionModel(from *errorException, out *modelpb.Exception) { out.Code = modeldecoderutil.ExceptionCodeString(from.Code.Val) } if len(from.Cause) > 0 { - out.Cause = modeldecoderutil.Reslice(out.Cause, len(from.Cause), modelpb.ExceptionFromVTPool) + out.Cause = modeldecoderutil.ResliceAndPopulateNil( + out.Cause, + len(from.Cause), + modelpb.ExceptionFromVTPool, + ) for i := 0; i < len(from.Cause); i++ { mapToExceptionModel(&from.Cause[i], out.Cause[i]) } @@ -537,7 +545,11 @@ func mapToExceptionModel(from *errorException, out *modelpb.Exception) { out.Module = from.Module.Val } if len(from.Stacktrace) > 0 { - out.Stacktrace = modeldecoderutil.Reslice(out.Stacktrace, len(from.Stacktrace), modelpb.StacktraceFrameFromVTPool) + out.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + out.Stacktrace, + len(from.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Stacktrace, out.Stacktrace) } if from.Type.IsSet() { @@ -809,18 +821,22 @@ func mapToMetricsetModel(from *metricset, event *modelpb.APMEvent) bool { } if len(from.Samples) > 0 { - event.Metricset.Samples = modeldecoderutil.Reslice(event.Metricset.Samples, len(from.Samples), modelpb.MetricsetSampleFromVTPool) + event.Metricset.Samples = modeldecoderutil.ResliceAndPopulateNil( + event.Metricset.Samples, + len(from.Samples), + modelpb.MetricsetSampleFromVTPool, + ) i := 0 for name, sample := range from.Samples { ms := event.Metricset.Samples[i] if len(sample.Counts) != 0 || len(sample.Values) != 0 { ms.Histogram = modelpb.HistogramFromVTPool() if n := len(sample.Values); n > 0 { - ms.Histogram.Values = modeldecoderutil.Reslice(ms.Histogram.Values, n, nil) + ms.Histogram.Values = modeldecoderutil.Reslice(ms.Histogram.Values, n) copy(ms.Histogram.Values, sample.Values) } if n := len(sample.Counts); n > 0 { - ms.Histogram.Counts = modeldecoderutil.Reslice(ms.Histogram.Counts, n, nil) + ms.Histogram.Counts = modeldecoderutil.Reslice(ms.Histogram.Counts, n) copy(ms.Histogram.Counts, sample.Counts) } } @@ -1086,7 +1102,7 @@ func mapToSpanModel(from *span, event *modelpb.APMEvent) { out.Composite = composite } if len(from.ChildIDs) > 0 { - event.ChildIds = modeldecoderutil.Reslice(event.ChildIds, len(from.ChildIDs), nil) + event.ChildIds = modeldecoderutil.Reslice(event.ChildIds, len(from.ChildIDs)) copy(event.ChildIds, from.ChildIDs) } if from.Context.Database.IsSet() { @@ -1278,7 +1294,11 @@ func mapToSpanModel(from *span, event *modelpb.APMEvent) { out.RepresentativeCount = 1 } if len(from.Stacktrace) > 0 { - out.Stacktrace = modeldecoderutil.Reslice(out.Stacktrace, len(from.Stacktrace), modelpb.StacktraceFrameFromVTPool) + out.Stacktrace = modeldecoderutil.ResliceAndPopulateNil( + out.Stacktrace, + len(from.Stacktrace), + modelpb.StacktraceFrameFromVTPool, + ) mapToStracktraceModel(from.Stacktrace, out.Stacktrace) } if from.Sync.IsSet() { @@ -1305,7 +1325,11 @@ func mapToSpanModel(from *span, event *modelpb.APMEvent) { mapOTelAttributesSpan(from.OTel, event) } if len(from.Links) > 0 { - out.Links = modeldecoderutil.Reslice(out.Links, len(from.Links), modelpb.SpanLinkFromVTPool) + out.Links = modeldecoderutil.ResliceAndPopulateNil( + out.Links, + len(from.Links), + modelpb.SpanLinkFromVTPool, + ) mapSpanLinks(from.Links, out.Links) } if out.Type == "" { @@ -1347,11 +1371,11 @@ func mapToStracktraceModel(from []stacktraceFrame, out []*modelpb.StacktraceFram fr.Module = eventFrame.Module.Val } if len(eventFrame.PostContext) > 0 { - fr.PostContext = modeldecoderutil.Reslice(fr.PostContext, len(eventFrame.PostContext), nil) + fr.PostContext = modeldecoderutil.Reslice(fr.PostContext, len(eventFrame.PostContext)) copy(fr.PostContext, eventFrame.PostContext) } if len(eventFrame.PreContext) > 0 { - fr.PreContext = modeldecoderutil.Reslice(fr.PreContext, len(eventFrame.PreContext), nil) + fr.PreContext = modeldecoderutil.Reslice(fr.PreContext, len(eventFrame.PreContext)) copy(fr.PreContext, eventFrame.PreContext) } if len(eventFrame.Vars) > 0 { @@ -1575,7 +1599,11 @@ func mapToTransactionModel(from *transaction, event *modelpb.APMEvent) { if event.Span == nil { event.Span = modelpb.SpanFromVTPool() } - event.Span.Links = modeldecoderutil.Reslice(event.Span.Links, len(from.Links), modelpb.SpanLinkFromVTPool) + event.Span.Links = modeldecoderutil.ResliceAndPopulateNil( + event.Span.Links, + len(from.Links), + modelpb.SpanLinkFromVTPool, + ) mapSpanLinks(from.Links, event.Span.Links) } if out.Type == "" {