From accc24556741b5530a3c26ee85fb53516274f67f Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Fri, 2 Oct 2026 22:29:36 +0200 Subject: [PATCH 1/2] pkg/clickhouse: read the column, stop dropping function names A Go service's goroutine profile came back from a server running the ClickHouse backend with every frame unsymbolized, while scraping the very same /debug/pprof endpoint directly named all of them. The frame count survived ingestion; the names did not. The location encoders write a column between the line number and the hasFunction flag. decodeLineInfo did not read it, so it read the column's first byte as the flag. For pprof, which carries no column information, that byte is a uvarint zero, so the flag read as false and the whole function block -- name, system name, filename, start line -- was skipped. Nothing failed while this happened. The address, the mapping and the line number all decoded, the row was written, and the profile came back nameless. Measured on a live server before this change: 3,632,819 stored frames, every address and every line number decoded, zero function names. A non-zero column was worse than lossy. OTLP and Arrow locations carry real column numbers, and there the misread desynchronised the rest of the record: column 1 decoded a corrupted name ("\tmain.main"), and a larger start line walked the offset off the end of the buffer and panicked. The ClickHouse ingest path has no recover(), so that is a dead server rather than a failed request. Only profiles that arrive already symbolized lose names -- anything scraped from a Go /debug/pprof endpoint, and any other pre-symbolized upload. Those are also the ones the symbolizer cannot rescue afterwards, because they usually carry no build ID to look up, so the result reads like a debuginfo coverage gap rather than a decoder that could not parse what it was handed. profile.DecodeInto, the canonical decoder, reads line then column then the flag; every encoder in pkg/profile writes them in that order. decodeLineInfo was the only parser in the tree that disagreed. The tests drive the real encoders rather than hand-built blobs, because a blob written by hand would be written from whatever the decoder happened to do and so would agree with the bug it existed to catch. They cover the mapping and no-mapping branches, a line with no function, and non-zero columns through EncodeOtelLocation -- including multi-byte ones, since a zero column cannot tell a uvarint read apart from a bare offset++. --- pkg/clickhouse/ingester.go | 9 +++ pkg/clickhouse/ingester_test.go | 131 ++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 pkg/clickhouse/ingester_test.go diff --git a/pkg/clickhouse/ingester.go b/pkg/clickhouse/ingester.go index 128a1a29fc9..4df702a8490 100644 --- a/pkg/clickhouse/ingester.go +++ b/pkg/clickhouse/ingester.go @@ -211,6 +211,15 @@ func decodeLineInfo(data []byte) LineInfo { offset += n info.LineNumber = int64(lineNum) + // Read 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. + _, n = varint.Uvarint(data[offset:]) + offset += n + hasFunction := data[offset] == 0x1 offset++ diff --git a/pkg/clickhouse/ingester_test.go b/pkg/clickhouse/ingester_test.go new file mode 100644 index 00000000000..2aa89119b6f --- /dev/null +++ b/pkg/clickhouse/ingester_test.go @@ -0,0 +1,131 @@ +// 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 ( + "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 := decodeLineInfo(profile.EncodePprofLocation(loc, tc.mapping, funcs, stringTable)) + + 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 := decodeLineInfo(profile.EncodeOtelLocation(nil, loc, nil, funcs, stringTable)) + + 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 := decodeLineInfo(profile.EncodePprofLocation(loc, nil, nil, []string{""})) + + 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) + }) +} From a3e95263283bfd48c5b8ba1c477dca69284341ca Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Sat, 3 Oct 2026 00:57:25 +0200 Subject: [PATCH 2/2] pkg/clickhouse, pkg/profile: bounds-check the location decoders The ingest path decodes each encoded location twice -- DecodeSymbolizationInfo for the address and mapping, decodeLineInfo for the line and function -- and neither had a single length check. Feeding every prefix of one real encoded location to them, 19 of 66 truncations panic in the first and 55 of 66 in the second. There is no recovery interceptor on this path, so that is not a failed request, it is a dead server. Both are now checked, and both are 0. The bytes are produced by this server's own encoders, so they are self-consistent in normal operation. That is what makes the failure mode worth closing rather than dismissing: the realistic way to reach a malformed record is encoder/decoder drift, which is exactly the state in which a decoder is already walking the record wrongly, and one field read at the wrong offset carries the next read further into territory that was indexed unchecked. Three distinct ways they could fault: - data[offset] for the flag bytes, past the end of a short record. - data[offset:] for every varint, likewise. - data[offset : offset+int(length)] for the strings. The length is unsigned: one above MaxInt converts to a NEGATIVE int, so offset+int(length) lands BELOW offset. That satisfies a naive "offset+int(length) > len(data)" guard and then panics on a slice whose high bound is below its low one, so the check has to be made in the space the length was read in. Panicking was not even the worst outcome. In production these bytes come from an Arrow dictionary buffer, and array.Binary.Value slices with cap running to the end of that whole buffer -- so a read past len but inside cap does not fault, it silently returns the bytes of the NEXT location. A truncated entry could therefore store a neighbouring location's bytes as this location's function filename, as a plausible-looking value, in a column nothing would flag. A failed read now yields "", never a prefix and never a neighbour. Each read reports whether it succeeded, and a record that runs out yields whatever had been decoded before it did. Partial information is the honest answer for a truncated record and keeps one bad location from discarding the rest of the batch -- but it must not be silent, or a systematic drift degrades every profile to unnamed frames while looking exactly like legitimately unsymbolized ones. decodeLineInfo now reports malformed records, extractStacktraceData counts them, and Ingest logs one warning per batch with the count. numLines == 0 and a line with no function are valid shapes and are not counted. Behaviour on well-formed input is unchanged; the round-trip tests pin that. The truncation sweep runs over six record shapes, because each reaches a different set of reads -- the unguarded decoder faulted on 13 of 14 prefixes of a no-function record and 34 of 43 of a no-mapping one, paths a single full-record sweep never visits. The sweeps slice with encoded[:i:i] rather than encoded[:i]: a two-index slice drops 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. It reports 45 where the capped slice reports 55 -- and given the Arrow cap behaviour above, the capped number is the one that describes production. Left for follow-ups, both pre-existing: profile.decodeString and profile.DecodeInto are unguarded in the same style, with the same unsigned length hazard, reached from pkg/parcacol and pkg/symbolizer; and EncodeArrowLocation writes the hasFunction flag unconditionally while serializedArrowLocationSize budgets the function block only for a valid function name, so the encoder itself can overrun its buffer. --- pkg/clickhouse/ingester.go | 232 +++++++++++++++++++++----------- pkg/clickhouse/ingester_test.go | 158 +++++++++++++++++++++- pkg/profile/decode.go | 107 +++++++++++---- pkg/profile/decode_test.go | 103 ++++++++++++++ 4 files changed, 492 insertions(+), 108 deletions(-) create mode 100644 pkg/profile/decode_test.go diff --git a/pkg/clickhouse/ingester.go b/pkg/clickhouse/ingester.go index 4df702a8490..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,93 +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) - - // Read 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. - _, n = varint.Uvarint(data[offset:]) - offset += n + // 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{}, @@ -269,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) @@ -287,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++ { @@ -314,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) @@ -322,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 index 2aa89119b6f..6c59fc1fecf 100644 --- a/pkg/clickhouse/ingester_test.go +++ b/pkg/clickhouse/ingester_test.go @@ -14,6 +14,7 @@ package clickhouse import ( + "encoding/binary" "testing" "github.com/stretchr/testify/require" @@ -72,7 +73,8 @@ func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { loc.MappingId = tc.mapping.Id } - got := decodeLineInfo(profile.EncodePprofLocation(loc, tc.mapping, funcs, stringTable)) + 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) @@ -100,7 +102,8 @@ func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { Lines: []*pprofextended.Line{{FunctionIndex: 1, Line: wantLine, Column: column}}, } - got := decodeLineInfo(profile.EncodeOtelLocation(nil, loc, nil, funcs, stringTable)) + 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) @@ -120,7 +123,8 @@ func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { Line: []*pprofpb.Line{{FunctionId: 0, Line: wantLine}}, } - got := decodeLineInfo(profile.EncodePprofLocation(loc, nil, nil, []string{""})) + 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) @@ -129,3 +133,151 @@ func TestDecodeLineInfoRoundTripsTheEncodedLocation(t *testing.T) { 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) + } +}