From 1953680b70084c9f78b28e00979c420000caea54 Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Sat, 3 Oct 2026 12:06:01 +0200 Subject: [PATCH] pkg/profile: do not write a function block the sizer did not budget EncodeArrowLocation allocates its buffer from serializedArrowLocationSize and then writes into it, so the two have to agree field for field. They did not. The sizer budgets the per-line function block only when the line's function name is valid: size++ // 1 byte for whether there is a function if lineFunctionName.IsValid(i) { ... start line, name, system name, filename ... } The writer emitted the flag as 0x1 and the block unconditionally. For a line whose function name is null the sizer allowed 10 bytes, the writer needed more, and it panicked in writeInt64AsUvarint writing the start line past the end of the buffer. A line with a number but no function is a representable shape, not a corruption: DecodeInto emits exactly that from a pprof Line with FunctionId 0, appending nulls to the function name, system name, filename and start line. Any record carrying one through this encoder died, and EncodeArrowLocation is on the normalizer's ingest path, which has no recover() -- so this was a failed process rather than a failed request. Writing 0x1 there was also a lie about the data even when the buffer happened to be large enough: it claimed a function for a line that has none, with the name read from whatever a null dictionary index resolves to. The writer now mirrors the sizer's condition and writes 0x0 with no block when there is no function name, which the decoders already round trip. The test asserts the invariant rather than the symptom: for a line with a function, a line without one, and both together, the bytes written must equal the bytes budgeted and must decode cleanly through DecodeInto. Length alone would not catch a writer that wrote too few bytes, since the buffer is allocated at the budget and would simply carry trailing zeroes -- decoding is what shows the bytes are well formed. --- pkg/profile/encode.go | 30 ++++++-- pkg/profile/encode_arrow_test.go | 124 +++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 8 deletions(-) create mode 100644 pkg/profile/encode_arrow_test.go diff --git a/pkg/profile/encode.go b/pkg/profile/encode.go index 35796e2758e..4867c50d975 100644 --- a/pkg/profile/encode.go +++ b/pkg/profile/encode.go @@ -359,17 +359,31 @@ func EncodeArrowLocation( offset = writeUint64(buf, offset, 0) } - buf[offset] = 0x1 - offset++ + // This must mirror serializedArrowLocationSize, which budgets the + // function block only under this same IsValid check. Writing the flag + // as 0x1 and the block unconditionally overran a buffer sized for the + // flag alone: for a line whose function name is null the sizer budgets + // 10 bytes and the writer needed more, panicking in + // writeInt64AsUvarint on the start line. A line with a number but no + // function is a representable shape -- DecodeInto emits exactly that + // from a pprof Line with FunctionId 0 -- and the decoders already round + // trip the 0x0 case. + if lineFunctionName.IsValid(i) { + buf[offset] = 0x1 + offset++ - offset = writeInt64AsUvarint(buf, offset, lineFunctionStartLine.Value(i)) - offset = writeString(buf, offset, string(lineFunctionNameDict.Value(int(lineFunctionName.GetValueIndex(i))))) - offset = writeString(buf, offset, string(lineFunctionSystemNameDict.Value(int(lineFunctionSystemName.GetValueIndex(i))))) + offset = writeInt64AsUvarint(buf, offset, lineFunctionStartLine.Value(i)) + offset = writeString(buf, offset, string(lineFunctionNameDict.Value(int(lineFunctionName.GetValueIndex(i))))) + offset = writeString(buf, offset, string(lineFunctionSystemNameDict.Value(int(lineFunctionSystemName.GetValueIndex(i))))) - if lineFunctionFilenameDict.IsValid(lineFunctionFilename.GetPhysicalIndex(i)) { - offset = writeString(buf, offset, string(lineFunctionFilenameDictValues.Value(int(lineFunctionFilenameDict.GetValueIndex(lineFunctionFilename.GetPhysicalIndex(i)))))) + if lineFunctionFilenameDict.IsValid(lineFunctionFilename.GetPhysicalIndex(i)) { + offset = writeString(buf, offset, string(lineFunctionFilenameDictValues.Value(int(lineFunctionFilenameDict.GetValueIndex(lineFunctionFilename.GetPhysicalIndex(i)))))) + } else { + offset = writeString(buf, offset, "") + } } else { - offset = writeString(buf, offset, "") + buf[offset] = 0x0 + offset++ } } diff --git a/pkg/profile/encode_arrow_test.go b/pkg/profile/encode_arrow_test.go new file mode 100644 index 00000000000..69fa1a679d9 --- /dev/null +++ b/pkg/profile/encode_arrow_test.go @@ -0,0 +1,124 @@ +// Copyright 2026 The Parca Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package profile + +import ( + "testing" + + "github.com/apache/arrow-go/v18/arrow" + "github.com/apache/arrow-go/v18/arrow/array" + "github.com/apache/arrow-go/v18/arrow/memory" + "github.com/stretchr/testify/require" +) + +// binaryDict builds a Dictionary whose entry at nullAt is null. +func binaryDict(t *testing.T, mem memory.Allocator, vals []string, nullAt int) (*array.Dictionary, *array.Binary) { + t.Helper() + b := array.NewBuilder(mem, &arrow.DictionaryType{ + IndexType: arrow.PrimitiveTypes.Uint32, + ValueType: arrow.BinaryTypes.Binary, + }).(*array.BinaryDictionaryBuilder) + for i, v := range vals { + if i == nullAt { + b.AppendNull() + continue + } + require.NoError(t, b.AppendString(v)) + } + a := b.NewArray().(*array.Dictionary) + return a, a.Dictionary().(*array.Binary) +} + +// EncodeArrowLocation must write exactly what serializedArrowLocationSize +// budgeted. +// +// The writer emitted the hasFunction flag as 0x1 and the function block +// unconditionally, while the sizer budgets that block only when the line's +// function name is valid. For a line whose function name is null the sizer +// allowed 10 bytes and the writer ran past the end of them, panicking in +// writeInt64AsUvarint on the start line. +// +// A line with a number but no function is a representable shape: DecodeInto +// emits precisely that from a pprof Line with FunctionId 0, and the decoders +// already round trip a 0x0 flag. +func TestEncodeArrowLocationMatchesItsBudget(t *testing.T) { + t.Parallel() + mem := memory.DefaultAllocator + + lineNumbers := array.NewInt64Builder(mem) + lineNumbers.Append(42) + lineNumbers.Append(43) + lineNumber := lineNumbers.NewArray().(*array.Int64) + + columns := array.NewUint64Builder(mem) + columns.Append(0) + columns.Append(7) + lineColumnNumber := columns.NewArray().(*array.Uint64) + + // Index 0 carries a function, index 1 does not. The dictionary is + // non-empty either way, so a lookup at index 1 would succeed and the only + // thing that goes wrong is the length of what gets written. + fnName, fnNameDict := binaryDict(t, mem, []string{"main.main", "unused"}, 1) + fnSys, fnSysDict := binaryDict(t, mem, []string{"main.main", "unused"}, 1) + + reeBuilder := array.NewBuilder(mem, arrow.RunEndEncodedOf( + arrow.PrimitiveTypes.Int32, + &arrow.DictionaryType{IndexType: arrow.PrimitiveTypes.Uint32, ValueType: arrow.BinaryTypes.Binary}, + )).(*array.RunEndEncodedBuilder) + reeBuilder.Append(2) + require.NoError(t, reeBuilder.ValueBuilder().(*array.BinaryDictionaryBuilder).AppendString("/x/main.go")) + fnFile := reeBuilder.NewArray().(*array.RunEndEncoded) + fnFileDict := fnFile.Values().(*array.Dictionary) + fnFileDictValues := fnFileDict.Dictionary().(*array.Binary) + + startLines := array.NewInt64Builder(mem) + startLines.Append(10) + startLines.Append(10) + lineFunctionStartLine := startLines.NewArray().(*array.Int64) + + for _, tc := range []struct { + name string + from, to int + }{ + {"with a function name", 0, 1}, + {"with a null function name", 1, 2}, + {"both lines", 0, 2}, + } { + t.Run(tc.name, func(t *testing.T) { + budget := serializedArrowLocationSize( + 0xdeadbeef, false, 0, 0, 0, nil, nil, tc.from, tc.to, nil, nil, + lineNumber, lineColumnNumber, fnName, fnNameDict, fnSys, fnSysDict, + fnFile, fnFileDict, fnFileDictValues, lineFunctionStartLine, + ) + + var buf []byte + require.NotPanics(t, func() { + buf = EncodeArrowLocation( + 0xdeadbeef, false, 0, 0, 0, nil, nil, tc.from, tc.to, nil, nil, + lineNumber, lineColumnNumber, fnName, fnNameDict, fnSys, fnSysDict, + fnFile, fnFileDict, fnFileDictValues, lineFunctionStartLine, + ) + }) + + // The buffer is allocated at the budget, so a writer that wrote + // fewer bytes than budgeted would leave trailing zeroes rather + // than fail. Decoding is what proves the bytes are well-formed. + require.Len(t, buf, budget) + + lw := NewLocationsWriter(mem) + _, err := DecodeInto(lw, buf, nil) + require.NoError(t, err) + }) + } +}