From 1e376a3373883775f61979e9d9ebe1b770ac7260 Mon Sep 17 00:00:00 2001 From: Thomas Guettler Date: Sat, 3 Oct 2026 14:17:59 +0200 Subject: [PATCH] pkg/profile: bounds-check DecodeInto DecodeInto indexes and slices caller-supplied bytes with no length checks. Feeding it every prefix of one real encoded location, 55 of the 66 truncations panic. There is no recover() anywhere in pkg or cmd outside pkg/symbol/addr2line, and no gRPC recovery interceptor, so that is not a failed query -- it is a dead server. This one is on the default path. DecodeInto is what pkg/parcacol/querier.go uses to read locations back, so it runs for every query against the FrostDB backend, on bytes that have been to storage and back. Corruption in a stored block is a more plausible way to reach a malformed record than an in-process round trip is. Three distinct ways it could fault: - data[offset] for the two flag bytes, past the end of a short record. - data[offset:] for every varint, likewise. - decodeString's data[n : n+int(length)]. The length is unsigned: one above MaxInt converts to a NEGATIVE int, so n+int(length) lands BELOW n. That satisfies a naive "n+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 is not even the worst outcome. These bytes usually arrive as an Arrow value, and array.Binary.Value slices with cap running to the end of the whole buffer -- so a read past len but inside cap does not fault, it returns the bytes of the NEXT location, which then decode as this location's own mapping or function. Plausible-looking, and nothing would flag it. Each read now reports whether it succeeded and a record that runs out returns a descriptive error naming the field and offset. The signature already returns an error, so there is no reason to guess: a malformed location fails the query that touched it rather than degrading quietly or killing the process. decodeString itself is left alone. It has thirteen other call sites across pkg/profile and pkg/symbolizer, several on decoders with their own self-consistent formats, so changing its contract is a wider change than this one; the checks live in DecodeInto, matching the shape already used by DecodeSymbolizationInfo. Behaviour on well-formed input is unchanged, which the existing round-trip tests pin. The truncation sweep runs over five record shapes because each reaches a different set of reads, and slices each prefix as encoded[:i: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. --- pkg/profile/decode.go | 164 +++++++++++++++++++++++++++---------- pkg/profile/decode_test.go | 123 ++++++++++++++++++++++++++++ 2 files changed, 246 insertions(+), 41 deletions(-) create mode 100644 pkg/profile/decode_test.go diff --git a/pkg/profile/decode.go b/pkg/profile/decode.go index e330a149f2a..3ad6a66780e 100644 --- a/pkg/profile/decode.go +++ b/pkg/profile/decode.go @@ -79,49 +79,117 @@ type DecodeResult struct { Mapping Mapping } +// DecodeInto decodes an encoded location into lw. +// +// Every read is bounds-checked and a record that runs out returns an error +// rather than panicking. This parses bytes read back from storage on the query +// path, and there is no recover() anywhere in the server, so an unchecked index +// here would take the process down on a corrupt or truncated location instead +// of failing the query that touched it. +// +// Reading past the end is not even reliably a crash: these bytes usually arrive +// as an Arrow value, and array.Binary.Value slices with cap running to the end +// of the whole buffer, so an over-read can silently return the next location's +// bytes and decode them as this location's own. func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeResult, error) { var ( - n int buildID []byte memoryStart uint64 memoryLength uint64 mappingOffset uint64 ) - addr, offset := varint.Uvarint(data) + offset := 0 - lineNumber, n := varint.Uvarint(data[offset:]) - offset += n + // 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 + } + flag := func() (bool, bool) { + if offset >= len(data) { + return false, false + } + v := data[offset] == 0x1 + offset++ + return v, true + } + truncated := func(field string) error { + return fmt.Errorf("malformed location: ran out of bytes reading %s at offset %d of %d", field, offset, len(data)) + } - hasMapping := data[offset] == 0x1 - offset++ + addr, ok := uvarint() + if !ok { + return DecodeResult{}, truncated("address") + } + + lineNumber, ok := uvarint() + if !ok { + return DecodeResult{}, truncated("number of lines") + } + + hasMapping, ok := flag() + if !ok { + return DecodeResult{}, truncated("has-mapping flag") + } if hasMapping { - buildID, n = decodeString(data[offset:]) - offset += n + buildID, ok = str() + if !ok { + return DecodeResult{}, truncated("mapping build ID") + } if err := lw.MappingBuildID.Append(buildID); err != nil { return DecodeResult{}, fmt.Errorf("append mapping build id: %w", err) } - filename, n := decodeString(data[offset:]) - offset += n + filename, ok := str() + if !ok { + return DecodeResult{}, truncated("mapping filename") + } if err := lw.MappingFile.Append(filename); err != nil { return DecodeResult{}, fmt.Errorf("append mapping filename: %w", err) } - memoryStart, n = varint.Uvarint(data[offset:]) - offset += n + memoryStart, ok = uvarint() + if !ok { + return DecodeResult{}, truncated("mapping memory start") + } lw.MappingStart.Append(memoryStart) - memoryLength, n = varint.Uvarint(data[offset:]) - offset += n + memoryLength, ok = uvarint() + if !ok { + return DecodeResult{}, truncated("mapping memory length") + } lw.MappingLimit.Append(memoryStart + memoryLength) - mappingOffset, n = varint.Uvarint(data[offset:]) - offset += n + mappingOffset, ok = uvarint() + if !ok { + return DecodeResult{}, truncated("mapping offset") + } lw.MappingOffset.Append(mappingOffset) } else { @@ -138,29 +206,41 @@ func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeRes for i := uint64(0); i < lineNumber; i++ { lw.Line.Append(true) - line, n := varint.Uvarint(data[offset:]) - offset += n + line, ok := uvarint() + if !ok { + return DecodeResult{}, truncated("line number") + } lw.LineNumber.Append(int64(line)) - column, n := varint.Uvarint(data[offset:]) - offset += n + column, ok := uvarint() + if !ok { + return DecodeResult{}, truncated("column") + } lw.ColumnNumber.Append(column) - hasFunction := data[offset] == 0x1 - offset++ + hasFunction, ok := flag() + if !ok { + return DecodeResult{}, truncated("has-function flag") + } if hasFunction { - startLine, n := varint.Uvarint(data[offset:]) - offset += n + startLine, ok := uvarint() + if !ok { + return DecodeResult{}, truncated("function start line") + } lw.FunctionStartLine.Append(int64(startLine)) - name, n := decodeString(data[offset:]) - offset += n + name, ok := str() + if !ok { + return DecodeResult{}, truncated("function name") + } - systemName, n := decodeString(data[offset:]) - offset += n + systemName, ok := str() + if !ok { + return DecodeResult{}, truncated("function system name") + } // Data written by the v2 ingest path before it populated the // name only carries system_name. @@ -180,8 +260,10 @@ func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeRes return DecodeResult{}, fmt.Errorf("append function system name: %w", err) } - filename, n := decodeString(data[offset:]) - offset += n + filename, ok := str() + if !ok { + return DecodeResult{}, truncated("function filename") + } if err := lw.FunctionFilename.Append(filename); err != nil { return DecodeResult{}, fmt.Errorf("append function filename: %w", err) @@ -197,18 +279,18 @@ func DecodeInto(lw LocationsWriter, data []byte, demangler Demangler) (DecodeRes return DecodeResult{ WroteLines: true, }, nil - } else { - return DecodeResult{ - WroteLines: false, - BuildID: buildID, - Addr: addr, - Mapping: Mapping{ - StartAddr: memoryStart, - EndAddr: memoryStart + memoryLength, - Offset: mappingOffset, - }, - }, nil } + + return DecodeResult{ + WroteLines: false, + BuildID: buildID, + Addr: addr, + Mapping: Mapping{ + StartAddr: memoryStart, + EndAddr: memoryStart + memoryLength, + Offset: mappingOffset, + }, + }, nil } // DecodeFunctionName is a fork of DecodeInto that only tries to find a function name and returns it. diff --git a/pkg/profile/decode_test.go b/pkg/profile/decode_test.go new file mode 100644 index 00000000000..2d058bd87ee --- /dev/null +++ b/pkg/profile/decode_test.go @@ -0,0 +1,123 @@ +// 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/apache/arrow-go/v18/arrow/memory" + "github.com/stretchr/testify/require" + + pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof" +) + +// A truncated location must return an error, not panic. +// +// DecodeInto parses bytes read back from storage on the query path +// (pkg/parcacol), and there is no recover() anywhere in the server, so an +// unchecked index here takes the process down on a corrupt or truncated +// location rather than failing the query that touched it. +// +// Every prefix of a real encoded record is fed in, so this 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. Each prefix is sliced encoded[:i:i] +// because a two-index slice drops the original cap, and Go does not panic +// reading past len while still inside cap -- the cheaper spelling silently +// passes on inputs that really do over-read. That is not academic: these bytes +// usually arrive as an Arrow value, and array.Binary.Value slices with cap +// running to the end of the whole buffer, so an over-read returns the next +// location's bytes instead of faulting. +func TestDecodeIntoSurvivesTruncation(t *testing.T) { + t.Parallel() + + 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}} + + 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}, + {"several lines", mapping, []*pprofpb.Line{ + {FunctionId: 1, Line: 42}, {FunctionId: 1, Line: 43}, + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + 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() { + lw := NewLocationsWriter(memory.DefaultAllocator) + _, _ = DecodeInto(lw, encoded[:i:i], nil) + }, "panicked on the first %d of %d bytes", i, len(encoded)) + } + + // A short record is reported, not swallowed. + if len(encoded) > 1 { + lw := NewLocationsWriter(memory.DefaultAllocator) + _, err := DecodeInto(lw, encoded[:len(encoded)-1:len(encoded)-1], nil) + require.Error(t, err, "a truncated record must be reported") + } + + // The whole record still decodes, so the bounds checks did not cost + // a field. + lw := NewLocationsWriter(memory.DefaultAllocator) + res, err := DecodeInto(lw, encoded, nil) + require.NoError(t, err) + require.Equal(t, len(tc.lines) > 0, res.WroteLines) + }) + } +} + +// A length prefix larger than the record must be refused, not panic. The length +// is unsigned: one above MaxInt converts to a negative int, so +// offset+int(length) lands below offset, satisfying a naive upper-bound check +// before panicking on a slice whose high bound is below its low one. +func TestDecodeIntoRejectsOversizedLength(t *testing.T) { + t.Parallel() + + for _, length := range []uint64{1 << 20, 1 << 63, ^uint64(0)} { + // A location with a mapping, whose build ID claims more bytes than the + // record holds. + 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) // build ID length + buf = append(buf, []byte("short")...) // fewer bytes than claimed + + require.NotPanics(t, func() { + lw := NewLocationsWriter(memory.DefaultAllocator) + _, err := DecodeInto(lw, buf, nil) + require.Error(t, err) + }) + } +}