diff --git a/pkg/clickhouse/ingester.go b/pkg/clickhouse/ingester.go index 128a1a29fc9..b9e97a615c0 100644 --- a/pkg/clickhouse/ingester.go +++ b/pkg/clickhouse/ingester.go @@ -55,6 +55,11 @@ func (i *Ingester) Ingest(ctx context.Context, record arrow.RecordBatch) error { schema := record.Schema() + // Counts locations whose encoded bytes could not be fully decoded, so a + // systematic encoder/decoder drift is visible instead of just producing + // profiles with unnamed frames. + malformedLocations := 0 + // Find column indices nameIdx := findColumnIndex(schema, profile.ColumnName) sampleTypeIdx := findColumnIndex(schema, profile.ColumnSampleType) @@ -102,7 +107,8 @@ func (i *Ingester) Ingest(ctx context.Context, record arrow.RecordBatch) error { } // Extract stacktrace data - stacktraceData := extractStacktraceData(record, stacktraceIdx, row) + stacktraceData, malformed := extractStacktraceData(record, stacktraceIdx, row) + malformedLocations += malformed // Append to batch err := batch.Append( @@ -135,6 +141,16 @@ func (i *Ingester) Ingest(ctx context.Context, record arrow.RecordBatch) error { } } + // One line per batch, not per location: a drift affects every location in + // the batch, and logging each one would bury the signal it is meant to be. + if malformedLocations > 0 { + level.Warn(i.logger).Log( + "msg", "could not fully decode some encoded locations; their frames are stored without function info", + "locations", malformedLocations, + "rows", record.NumRows(), + ) + } + if err := batch.Send(); err != nil { return fmt.Errorf("failed to send batch: %w", err) } @@ -167,84 +183,144 @@ type LineInfo struct { } // decodeLineInfo decodes line and function information from the encoded location data. -// It returns the first line's info (most profiles have one line per location). -func decodeLineInfo(data []byte) LineInfo { - var n int +// +// Every read is bounds-checked, and a short or malformed record yields whatever +// had been decoded before the record ran out rather than panicking. The bytes +// are produced by this server's own encoders and so are self-consistent in +// normal operation, which is exactly why the failure mode matters: the realistic +// way to get a malformed record is encoder/decoder drift, and there is no +// recovery interceptor on the ingest path, so an unchecked index there takes the +// process down instead of failing one request. +func decodeLineInfo(data []byte) (LineInfo, bool) { info := LineInfo{} + offset := 0 - // Skip addr - _, offset := varint.Uvarint(data) - - // Read number of lines - numLines, n := varint.Uvarint(data[offset:]) - offset += n - - // Check if has mapping - hasMapping := data[offset] == 0x1 - offset++ - - if hasMapping { - // Skip buildID - length, n := varint.Uvarint(data[offset:]) - offset += n + int(length) - - // Skip filename - length, n = varint.Uvarint(data[offset:]) - offset += n + int(length) - - // Skip memoryStart - _, n = varint.Uvarint(data[offset:]) - offset += n - - // Skip memoryLength - _, n = varint.Uvarint(data[offset:]) + // uvarint reports false when the record has run out or the varint is + // malformed; Uvarint returns n <= 0 for both. + uvarint := func() (uint64, bool) { + if offset >= len(data) { + return 0, false + } + v, n := varint.Uvarint(data[offset:]) + if n <= 0 { + return 0, false + } offset += n + return v, true + } + // str reads a length-prefixed string. + str := func() (string, bool) { + length, ok := uvarint() + // The length is unsigned, so one larger than MaxInt converts to a + // negative int and offset+int(length) lands BELOW offset -- which slips + // past a naive offset+int(length) > len(data) check straight into a + // panicking slice. Compare in the space the length was read in, against + // the bytes that actually remain. + if !ok || length > uint64(len(data)-offset) { + return "", false + } + v := string(data[offset : offset+int(length)]) + offset += int(length) + return v, true + } + flag := func() (bool, bool) { + if offset >= len(data) { + return false, false + } + v := data[offset] == 0x1 + offset++ + return v, true + } + skip := func(n int) bool { + for range n { + if _, ok := uvarint(); !ok { + return false + } + } + return true + } - // Skip mappingOffset - _, n = varint.Uvarint(data[offset:]) - offset += n + if _, ok := uvarint(); !ok { // address + return info, false + } + numLines, ok := uvarint() + if !ok { + return info, false + } + hasMapping, ok := flag() + if !ok { + return info, false + } + if hasMapping { + if _, ok := str(); !ok { // buildID + return info, false + } + if _, ok := str(); !ok { // filename + return info, false + } + // memoryStart, memoryLength, mappingOffset + if !skip(3) { + return info, false + } } - if numLines > 0 { - // Read first line info (we only store one line per location) - lineNum, n := varint.Uvarint(data[offset:]) - offset += n - info.LineNumber = int64(lineNum) + // A location with no lines is a valid shape, not a malformed record. + if numLines == 0 { + return info, true + } - hasFunction := data[offset] == 0x1 - offset++ + // Only the first line is kept: the schema stores one function per location. + // Location.Line[0] is the innermost inlined function, which is the right one + // to keep, but every inlined caller above it is dropped here. + lineNum, ok := uvarint() + if !ok { + return info, false + } + info.LineNumber = int64(lineNum) + + // The column. pprof carries no column information, so EncodePprofLocation + // writes a uvarint zero here -- a single 0x00 byte. Leaving it unread makes + // the hasFunction read below land on the column instead of the flag, where + // it is always false, which silently discards the function name, system + // name, filename and start line of every already-symbolized location. + if _, ok := uvarint(); !ok { + return info, false + } - if hasFunction { - // Read startLine - startLine, n := varint.Uvarint(data[offset:]) - offset += n - info.FunctionStartLine = int64(startLine) - - // Read function name - length, n := varint.Uvarint(data[offset:]) - offset += n - info.FunctionName = string(data[offset : offset+int(length)]) - offset += int(length) - - // Read system name - length, n = varint.Uvarint(data[offset:]) - offset += n - info.FunctionSystemName = string(data[offset : offset+int(length)]) - offset += int(length) - - // Read filename - length, n = varint.Uvarint(data[offset:]) - offset += n - info.FunctionFilename = string(data[offset : offset+int(length)]) - } + hasFunction, ok := flag() + if !ok { + return info, false + } + // A line with no function is a valid shape too. + if !hasFunction { + return info, true } - return info + startLine, ok := uvarint() + if !ok { + return info, false + } + info.FunctionStartLine = int64(startLine) + if info.FunctionName, ok = str(); !ok { + return info, false + } + if info.FunctionSystemName, ok = str(); !ok { + return info, false + } + info.FunctionFilename, ok = str() + return info, ok } // extractStacktraceData extracts stacktrace information from the encoded binary column. // The stacktrace column contains encoded location data that needs to be decoded. -func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) StacktraceData { +// The second return value counts locations whose encoded bytes could not be +// fully decoded. They are still written, with whatever was recovered, so one +// bad location does not discard the rest of the batch -- but the count is +// reported by the caller, because a decoder that silently degrades every +// profile to unnamed frames is indistinguishable from legitimately +// unsymbolized ones. +func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) (StacktraceData, int) { + malformed := 0 data := StacktraceData{ Addresses: []uint64{}, MappingStarts: []uint64{}, @@ -260,17 +336,17 @@ func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) Stacktrace } if colIdx < 0 { - return data + return data, 0 } col := record.Column(colIdx) listCol, ok := col.(*array.List) if !ok { - return data + return data, 0 } if listCol.IsNull(row) { - return data + return data, 0 } start, end := listCol.ValueOffsets(row) @@ -278,12 +354,12 @@ func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) Stacktrace dictCol, ok := values.(*array.Dictionary) if !ok { - return data + return data, 0 } binaryDict, ok := dictCol.Dictionary().(*array.Binary) if !ok { - return data + return data, 0 } for idx := int(start); idx < int(end); idx++ { @@ -305,7 +381,10 @@ func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) Stacktrace data.MappingBuildIDs = append(data.MappingBuildIDs, string(symInfo.BuildID)) // Decode line/function info - lineInfo := decodeLineInfo(encodedLocation) + lineInfo, ok := decodeLineInfo(encodedLocation) + if !ok { + malformed++ + } data.LineNumbers = append(data.LineNumbers, lineInfo.LineNumber) data.FunctionNames = append(data.FunctionNames, lineInfo.FunctionName) data.FunctionSystemNames = append(data.FunctionSystemNames, lineInfo.FunctionSystemName) @@ -313,7 +392,7 @@ func extractStacktraceData(record arrow.RecordBatch, colIdx, row int) Stacktrace data.FunctionStartLines = append(data.FunctionStartLines, lineInfo.FunctionStartLine) } - return data + return data, malformed } func findColumnIndex(schema *arrow.Schema, name string) int { diff --git a/pkg/clickhouse/ingester_test.go b/pkg/clickhouse/ingester_test.go new file mode 100644 index 00000000000..6c59fc1fecf --- /dev/null +++ b/pkg/clickhouse/ingester_test.go @@ -0,0 +1,283 @@ +// 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 clickhouse + +import ( + "encoding/binary" + "testing" + + "github.com/stretchr/testify/require" + pprofextended "go.opentelemetry.io/proto/otlp/profiles/v1development" + + pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof" + "github.com/parca-dev/parca/pkg/profile" +) + +// decodeLineInfo must stay in step with the location encoders, which write a +// column between the line number and the hasFunction flag. +// +// Every encoder in pkg/profile agrees on that layout -- EncodePprofLocation, +// EncodeOtelLocation, EncodeArrowLocation and the normalizer's v2 encoder -- +// and so does the canonical decoder, profile.DecodeInto. A decoder that skips +// the column reads the column's own bytes as the flag, which costs the whole +// function block: name, system name, filename and start line. +// +// These drive the real encoders rather than hand-built blobs on purpose. A blob +// written by hand would be written from whatever the decoder happens to do, and +// so would agree with any bug it was supposed to catch. +func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { + const ( + wantName = "bufio.(*Reader).Peek" + wantSys = "bufio.(*Reader).Peek" + wantFile = "/usr/local/go/src/bufio/bufio.go" + wantLine = 42 + wantStart = 10 + ) + + // pprof carries no column information, so EncodePprofLocation writes a + // uvarint zero -- a single 0x00 byte. The mapping block sits between the + // line count and the lines, so a decoder that is wrong about one can + // easily be right about the other; cover both. Values are non-zero so the + // uvarints are more than one byte each. + t.Run("pprof", func(t *testing.T) { + stringTable := []string{"", wantName, wantSys, wantFile, "build-id", "/bin/svc"} + funcs := []*pprofpb.Function{{Id: 1, Name: 1, SystemName: 2, Filename: 3, StartLine: wantStart}} + + for _, tc := range []struct { + name string + mapping *pprofpb.Mapping + }{ + {"with a mapping", &pprofpb.Mapping{ + Id: 1, BuildId: 4, Filename: 5, + MemoryStart: 0x1000, MemoryLimit: 0x2000, FileOffset: 8, + }}, + {"without a mapping", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + loc := &pprofpb.Location{ + Id: 1, Address: 0xdeadbeef, + Line: []*pprofpb.Line{{FunctionId: 1, Line: wantLine}}, + } + if tc.mapping != nil { + loc.MappingId = tc.mapping.Id + } + + got, ok := decodeLineInfo(profile.EncodePprofLocation(loc, tc.mapping, funcs, stringTable)) + require.True(t, ok, "a well-formed location must decode cleanly") + + require.Equal(t, wantName, got.FunctionName, "the function name was discarded") + require.Equal(t, wantSys, got.FunctionSystemName) + require.Equal(t, wantFile, got.FunctionFilename) + require.EqualValues(t, wantLine, got.LineNumber) + require.EqualValues(t, wantStart, got.FunctionStartLine) + }) + } + }) + + // A zero column is a single 0x00 byte, which cannot tell a uvarint read + // apart from a bare offset++. OTLP locations carry real column numbers, so + // they are what pins the read down -- and a multi-byte column (>= 0x80) is + // what pins down that it is a *uvarint* read rather than a one-byte skip. + t.Run("otel with a non-zero column", func(t *testing.T) { + stringTable := []string{"", wantName, wantSys, wantFile, "build-id", "/bin/svc"} + funcs := []*pprofextended.Function{{ + NameStrindex: 1, SystemNameStrindex: 2, FilenameStrindex: 3, StartLine: wantStart, + }} + + for _, column := range []int64{1, 7, 300, 16384} { + t.Run("column", func(t *testing.T) { + loc := &pprofextended.Location{ + Address: 0xdeadbeef, + Lines: []*pprofextended.Line{{FunctionIndex: 1, Line: wantLine, Column: column}}, + } + + got, ok := decodeLineInfo(profile.EncodeOtelLocation(nil, loc, nil, funcs, stringTable)) + require.True(t, ok, "a well-formed location must decode cleanly") + + require.Equal(t, wantName, got.FunctionName, "column %d desynchronised the decoder", column) + require.Equal(t, wantSys, got.FunctionSystemName) + require.Equal(t, wantFile, got.FunctionFilename) + require.EqualValues(t, wantLine, got.LineNumber) + require.EqualValues(t, wantStart, got.FunctionStartLine) + }) + } + }) + + // A line with no function is a representable shape: the encoder writes the + // flag as 0x0 and no function block. The fields must come back zeroed -- + // which is the truth about this location, not a dropped name. + t.Run("line without a function", func(t *testing.T) { + loc := &pprofpb.Location{ + Id: 1, Address: 0xdeadbeef, + Line: []*pprofpb.Line{{FunctionId: 0, Line: wantLine}}, + } + + got, ok := decodeLineInfo(profile.EncodePprofLocation(loc, nil, nil, []string{""})) + require.True(t, ok, "a line with no function is a valid shape, not a malformed record") + + require.EqualValues(t, wantLine, got.LineNumber) + require.Empty(t, got.FunctionName) + require.Empty(t, got.FunctionSystemName) + require.Empty(t, got.FunctionFilename) + require.Zero(t, got.FunctionStartLine) + }) +} + +// A truncated record must not panic. +// +// decodeLineInfo indexes and slices caller-supplied bytes, and the ClickHouse +// ingest path has no recovery interceptor, so a panic here is not a failed +// request -- it is a dead server. The bytes come from this server's own +// encoders, so the realistic way to get a malformed one is encoder/decoder +// drift, which is precisely the situation in which the decoder is already +// walking the record wrongly. +// +// Every prefix of a real encoded record is fed in, so the assertion covers +// running out mid-varint, mid-string, and exactly on a boundary, at every field +// in the layout rather than at a hand-picked few. +func TestDecodeLineInfoSurvivesTruncation(t *testing.T) { + stringTable := []string{"", "main.main", "main.main", "/x/main.go", "build-id", "/bin/svc"} + funcs := []*pprofpb.Function{{Id: 1, Name: 1, SystemName: 2, Filename: 3, StartLine: 10}} + mapping := &pprofpb.Mapping{ + Id: 1, BuildId: 4, Filename: 5, + MemoryStart: 0x1000, MemoryLimit: 0x2000, FileOffset: 8, + } + withFunc := []*pprofpb.Line{{FunctionId: 1, Line: 42}} + + // Each shape reaches a different set of reads, and the unguarded decoder + // faulted in all of them: 55 of the 66 prefixes of the full record, and + // 13 of 14 for the no-function one. + for _, tc := range []struct { + name string + mapping *pprofpb.Mapping + lines []*pprofpb.Line + }{ + {"mapping and function", mapping, withFunc}, + {"no mapping", nil, withFunc}, + {"no function", mapping, []*pprofpb.Line{{FunctionId: 0, Line: 42}}}, + {"no lines", mapping, nil}, + {"no lines, no mapping", nil, nil}, + {"several lines", mapping, []*pprofpb.Line{ + {FunctionId: 1, Line: 42}, {FunctionId: 1, Line: 43}, {FunctionId: 1, Line: 44}, + }}, + } { + t.Run(tc.name, func(t *testing.T) { + loc := &pprofpb.Location{Id: 1, Address: 0xdeadbeef, Line: tc.lines} + if tc.mapping != nil { + loc.MappingId = tc.mapping.Id + } + encoded := profile.EncodePprofLocation(loc, tc.mapping, funcs, stringTable) + require.NotEmpty(t, encoded) + + // encoded[:i:i], not encoded[:i]: a two-index slice keeps the + // original cap, and Go does not panic reading past len while still + // inside cap -- so the cheaper spelling silently passes on inputs + // that really do over-read. Capping is what makes this measure the + // bound. It is not academic here: in production these bytes come + // from an Arrow dictionary buffer, whose values are sliced with cap + // running to the end of the whole buffer, so an over-read returns + // the next location's bytes rather than faulting. + for i := 0; i <= len(encoded); i++ { + require.NotPanicsf(t, func() { decodeLineInfo(encoded[:i:i]) }, + "panicked on the first %d of %d bytes", i, len(encoded)) + } + + // The whole record still decodes, so the bounds checks did not cost + // a field. + got, ok := decodeLineInfo(encoded) + require.True(t, ok) + if len(tc.lines) > 0 { + require.EqualValues(t, 42, got.LineNumber) + } + if len(tc.lines) > 0 && tc.lines[0].FunctionId != 0 { + require.Equal(t, "main.main", got.FunctionName) + require.Equal(t, "/x/main.go", got.FunctionFilename) + require.EqualValues(t, 10, got.FunctionStartLine) + } + }) + } +} + +// A length prefix larger than the record must not panic. +// +// The length is read as a uint64. One above MaxInt converts to a NEGATIVE int, +// so offset+int(length) lands below offset -- which satisfies a naive +// "offset+int(length) > len(data)" check and then panics on a slice whose high +// bound is less than its low one. The check has to be made in the space the +// length was read in. +func TestDecodeLineInfoRejectsOversizedLength(t *testing.T) { + for _, tc := range []struct { + name string + length uint64 + }{ + {"longer than the record", 1 << 20}, + {"larger than MaxInt", ^uint64(0)}, + {"MaxInt64 plus one", 1 << 63}, + } { + t.Run(tc.name, func(t *testing.T) { + var buf []byte + buf = binary.AppendUvarint(buf, 0xdeadbeef) // address + buf = binary.AppendUvarint(buf, 1) // numLines + buf = append(buf, 0x0) // hasMapping = false + buf = binary.AppendUvarint(buf, 42) // line number + buf = binary.AppendUvarint(buf, 0) // column + buf = append(buf, 0x1) // hasFunction = true + buf = binary.AppendUvarint(buf, 10) // startLine + buf = binary.AppendUvarint(buf, tc.length) // function name length + buf = append(buf, []byte("main.main")...) // fewer bytes than claimed + + var got LineInfo + var ok bool + require.NotPanics(t, func() { got, ok = decodeLineInfo(buf) }) + require.False(t, ok, "an oversized length must be reported as malformed") + // What was decoded before the bad length stands; the name does not. + require.EqualValues(t, 42, got.LineNumber) + require.EqualValues(t, 10, got.FunctionStartLine) + require.Empty(t, got.FunctionName) + }) + } +} + +// The two mapping strings are read behind the hasMapping flag, which the +// function-string cases never reach, so they need their own oversized-length +// coverage. +func TestDecodeLineInfoRejectsOversizedMappingLength(t *testing.T) { + for _, tc := range []struct { + name string + which int // 0 = buildID, 1 = mapping filename + }{ + {"build ID", 0}, + {"mapping filename", 1}, + } { + t.Run(tc.name, func(t *testing.T) { + var buf []byte + buf = binary.AppendUvarint(buf, 0xdeadbeef) // address + buf = binary.AppendUvarint(buf, 1) // numLines + buf = append(buf, 0x1) // hasMapping = true + if tc.which == 0 { + buf = binary.AppendUvarint(buf, ^uint64(0)) // buildID length + buf = append(buf, []byte("short")...) + } else { + buf = binary.AppendUvarint(buf, 5) // buildID length + buf = append(buf, []byte("bid01")...) + buf = binary.AppendUvarint(buf, ^uint64(0)) // filename length + buf = append(buf, []byte("short")...) + } + + var ok bool + require.NotPanics(t, func() { _, ok = decodeLineInfo(buf) }) + require.False(t, ok, "an oversized mapping length must be reported as malformed") + }) + } +} diff --git a/pkg/profile/decode.go b/pkg/profile/decode.go index e330a149f2a..034c8b1b75b 100644 --- a/pkg/profile/decode.go +++ b/pkg/profile/decode.go @@ -29,46 +29,105 @@ type SymbolizationInfo struct { Mapping Mapping } +// DecodeSymbolizationInfo decodes the address and mapping of an encoded +// location. +// +// Every read is bounds-checked, and a record that runs out mid-field yields +// whatever had been decoded before it did. It parses bytes handed to it by a +// caller -- on the ClickHouse ingest path, straight out of an Arrow dictionary +// buffer -- and there is no recovery interceptor there, so an unchecked index +// would take the process down rather than fail one request. Reading past the +// end is not even reliably a crash: an Arrow value's cap runs to the end of +// the whole buffer, so an over-read can silently return the bytes of the next +// location instead of panicking. func DecodeSymbolizationInfo(data []byte) (SymbolizationInfo, uint64) { offset := 0 - addr, n := varint.Uvarint(data) // we need to know the address size to read the build ID - offset += n - numberOfLines, n := varint.Uvarint(data[offset:]) - offset += n - - hasMapping := data[offset] == 0x1 - offset++ - - if hasMapping { - buildID, n := decodeString(data[offset:]) + // uvarint reports false when the record has run out or the varint is + // malformed; Uvarint returns n <= 0 for both. + uvarint := func() (uint64, bool) { + if offset >= len(data) { + return 0, false + } + v, n := varint.Uvarint(data[offset:]) + if n <= 0 { + return 0, false + } offset += n + return v, true + } + // str reads a length-prefixed string. The length is unsigned, so one above + // MaxInt converts to a negative int and offset+int(length) lands BELOW + // offset -- which slips past a naive upper-bound check straight into a + // panicking slice. Compare in the space the length was read in. + str := func() ([]byte, bool) { + length, ok := uvarint() + if !ok || length > uint64(len(data)-offset) { + return nil, false + } + v := data[offset : offset+int(length)] + offset += int(length) + return v, true + } - file, n := decodeString(data[offset:]) - offset += n + // We need to know the address size to read the build ID. + addr, ok := uvarint() + if !ok { + return SymbolizationInfo{}, 0 + } - memoryStart, n := varint.Uvarint(data[offset:]) - offset += n + numberOfLines, ok := uvarint() + if !ok { + return SymbolizationInfo{Addr: addr}, 0 + } - memoryLength, n := varint.Uvarint(data[offset:]) - offset += n + if offset >= len(data) { + return SymbolizationInfo{Addr: addr}, numberOfLines + } + hasMapping := data[offset] == 0x1 + offset++ - mappingOffset, _ := varint.Uvarint(data[offset:]) + if !hasMapping { + return SymbolizationInfo{Addr: addr}, numberOfLines + } + buildID, ok := str() + if !ok { + return SymbolizationInfo{Addr: addr}, numberOfLines + } + file, ok := str() + if !ok { + return SymbolizationInfo{Addr: addr, BuildID: buildID}, numberOfLines + } + memoryStart, ok := uvarint() + if !ok { return SymbolizationInfo{ Addr: addr, BuildID: buildID, - Mapping: Mapping{ - StartAddr: memoryStart, - EndAddr: memoryStart + memoryLength, - Offset: mappingOffset, - File: string(file), - }, + Mapping: Mapping{File: string(file)}, + }, numberOfLines + } + memoryLength, ok := uvarint() + if !ok { + return SymbolizationInfo{ + Addr: addr, + BuildID: buildID, + Mapping: Mapping{StartAddr: memoryStart, File: string(file)}, }, numberOfLines } + // The final field is read without a success check on purpose: a zero offset + // is the same answer a missing one would give. + mappingOffset, _ := uvarint() return SymbolizationInfo{ - Addr: addr, + Addr: addr, + BuildID: buildID, + Mapping: Mapping{ + StartAddr: memoryStart, + EndAddr: memoryStart + memoryLength, + Offset: mappingOffset, + File: string(file), + }, }, numberOfLines } diff --git a/pkg/profile/decode_test.go b/pkg/profile/decode_test.go new file mode 100644 index 00000000000..a29960ebb9b --- /dev/null +++ b/pkg/profile/decode_test.go @@ -0,0 +1,103 @@ +// 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 ( + "encoding/binary" + "testing" + + "github.com/stretchr/testify/require" + + pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof" +) + +// A truncated record must not panic. +// +// DecodeSymbolizationInfo is called on the ClickHouse ingest path, on bytes +// taken straight from an Arrow dictionary buffer, and there is no recovery +// interceptor there -- so an unchecked index is a dead server rather than a +// failed request. Before the bounds checks, 19 of the 66 prefixes of one real +// record faulted. +// +// Reading past the end is not even reliably a crash. array.Binary.Value slices +// with cap running to the end of the whole dictionary buffer, so an over-read +// can silently return the next location's bytes and store them as this +// location's mapping -- which is why the sweep caps each prefix with +// encoded[:i:i] rather than trusting a panic to reveal the bug. +func TestDecodeSymbolizationInfoSurvivesTruncation(t *testing.T) { + stringTable := []string{"", "main.main", "main.main", "/x/main.go", "build-id", "/bin/svc"} + funcs := []*pprofpb.Function{{Id: 1, Name: 1, SystemName: 2, Filename: 3, StartLine: 10}} + + for _, tc := range []struct { + name string + mapping *pprofpb.Mapping + lines []*pprofpb.Line + }{ + {"with a mapping", &pprofpb.Mapping{ + Id: 1, BuildId: 4, Filename: 5, + MemoryStart: 0x1000, MemoryLimit: 0x2000, FileOffset: 8, + }, []*pprofpb.Line{{FunctionId: 1, Line: 42}}}, + {"without a mapping", nil, []*pprofpb.Line{{FunctionId: 1, Line: 42}}}, + {"no lines", nil, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + loc := &pprofpb.Location{Id: 1, Address: 0xdeadbeef, Line: tc.lines} + if tc.mapping != nil { + loc.MappingId = tc.mapping.Id + } + encoded := EncodePprofLocation(loc, tc.mapping, funcs, stringTable) + require.NotEmpty(t, encoded) + + for i := 0; i <= len(encoded); i++ { + require.NotPanicsf(t, func() { DecodeSymbolizationInfo(encoded[:i:i]) }, + "panicked on the first %d of %d bytes", i, len(encoded)) + } + + // The whole record still decodes, so the bounds checks did not cost + // a field. + got, numLines := DecodeSymbolizationInfo(encoded) + require.EqualValues(t, 0xdeadbeef, got.Addr) + require.EqualValues(t, len(tc.lines), numLines) + if tc.mapping != nil { + require.Equal(t, "build-id", string(got.BuildID)) + require.Equal(t, "/bin/svc", got.Mapping.File) + require.EqualValues(t, 0x1000, got.Mapping.StartAddr) + require.EqualValues(t, 0x2000, got.Mapping.EndAddr) + require.EqualValues(t, 8, got.Mapping.Offset) + } + }) + } +} + +// A length prefix larger than the record must not panic. The length is +// unsigned: one above MaxInt converts to a negative int, so offset+int(length) +// lands below offset and satisfies a naive upper-bound check before panicking +// on a slice whose high bound is below its low one. +func TestDecodeSymbolizationInfoRejectsOversizedLength(t *testing.T) { + for _, length := range []uint64{1 << 20, 1 << 63, ^uint64(0)} { + var buf []byte + buf = binary.AppendUvarint(buf, 0xdeadbeef) // address + buf = binary.AppendUvarint(buf, 1) // numLines + buf = append(buf, 0x1) // hasMapping = true + buf = binary.AppendUvarint(buf, length) // buildID length + buf = append(buf, []byte("short")...) // fewer bytes than claimed + + var got SymbolizationInfo + require.NotPanics(t, func() { got, _ = DecodeSymbolizationInfo(buf) }) + // The address was decoded before the bad length; the mapping was not. + require.EqualValues(t, 0xdeadbeef, got.Addr) + require.Empty(t, got.BuildID) + require.Empty(t, got.Mapping.File) + } +}