pkg/profile: bounds-check DecodeInto

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.

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