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) + }) + } +}