sstable: prevent block metadata pointers to garbage

When allocating memory for a new block, we allocate an additional
block.MetadataSize bytes for holding a Go structure containing decoded data
describing the block and maintaining pointers into the block's contents. This
allows us to decode the block structure once when the block is loaded into the
block cache. Readers that find the block in the cache simply cast this 'block
metadata' preamble into the appropriate type.

When a block is not found within the block cache, we: load the block, decode
the block's header and initialize the block's unique metadata. We initialize
the metadata by first casting the allocation's pointer to the appropriate Go
pointer type. Then we decode the block's data, initializing the metadata struct
accordingly. In the interim between the pointer cast and the initialization,
the contents of the metadata are undefined.

The per-block metadata types typically contain pointer fields. Once
initialized, these pointers point into the block's data, all part of the same
CGo manual allocation. If the Go garbage collector examines this Go struct
before it's been fully initialized, it may observe arbitrary garbage within the
pointer fields. If one of these values looks like a pointer to a Go-allocated
part of the address space, the Go garbage collector crashes the process. It's a
little unclear the circumstances in which the Go garbage collector will examine
these metadata structs that exist within CGo memory, but experimentally it
appears to be the case.

This commit fixes the issue by always zeroing block metadata memory before
casting it into the appropriate metadata struct. Additionally, it solves an
analogous problem during de-allocation by nil-ing metadata pointers into blocks
before we release the blocks back to the cache or buffer pool. Together, these
changes ensure that whenever we have a pointer to a block metadata Go
structure, the structure's backing memory is either zeroed or the result of
initialization based on decoding the block.

Informs cockroachdb/cockroach#149955.
Informs cockroachdb/cockroach#150216.

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

Author: jbowens

sstable/blob/blocks.go

@@ -236,10 +236,7 @@ const _ uint = block.MetadataSize - uint(unsafe.Sizeof(indexBlockDecoder{}))
 
 // initIndexBlockMetadata initializes the index block metadata.
 func initIndexBlockMetadata(md *block.Metadata, data []byte) (err error) {
-	if uintptr(unsafe.Pointer(md))%8 != 0 {
-		return errors.AssertionFailedf("metadata is not 8-byte aligned")
-	}
-	d := (*indexBlockDecoder)(unsafe.Pointer(md))
+	d := block.CastMetadataZero[indexBlockDecoder](md)
 	// Initialization can panic; convert panics to corruption errors (so higher
 	// layers can add file number and offset information).
 	defer func() {
@@ -355,10 +352,7 @@ const _ uint = block.MetadataSize - uint(unsafe.Sizeof(blobValueBlockDecoder{}))
 
 // initBlobValueBlockMetadata initializes the blob value block metadata.
 func initBlobValueBlockMetadata(md *block.Metadata, data []byte) (err error) {
-	if uintptr(unsafe.Pointer(md))%8 != 0 {
-		return errors.AssertionFailedf("metadata is not 8-byte aligned")
-	}
-	d := (*blobValueBlockDecoder)(unsafe.Pointer(md))
+	d := block.CastMetadataZero[blobValueBlockDecoder](md)
 	// Initialization can panic; convert panics to corruption errors (so higher
 	// layers can add file number and offset information).
 	defer func() {

sstable/blob/fetcher.go

@@ -6,7 +6,6 @@ package blob
 
 import (
 	"context"
-	"unsafe"
 
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/internal/base"
@@ -250,7 +249,7 @@ func (cr *cachedReader) GetUnsafeValue(
 			if err != nil {
 				return nil, err
 			}
-			cr.indexBlock.dec = (*indexBlockDecoder)(unsafe.Pointer(cr.indexBlock.buf.BlockMetadata()))
+			cr.indexBlock.dec = block.CastMetadata[indexBlockDecoder](cr.indexBlock.buf.BlockMetadata())
 			cr.indexBlock.loaded = true
 		}
 
@@ -281,14 +280,17 @@ func (cr *cachedReader) GetUnsafeValue(
 		// this case to be rare, and this is a hot path for the more common case
 		// of non-rewritten blob files, so we defer optimizing for now.
 		h := cr.indexBlock.dec.BlockHandle(physicalBlockIndex)
+		// Nil out the decoder before releasing the buffers to ensure the Go GC
+		// doesn't misinterpret the freed memory backing the decoders.
+		cr.currentValueBlock.dec = nil
 		cr.currentValueBlock.buf.Release()
 		cr.currentValueBlock.loaded = false
 		var err error
 		cr.currentValueBlock.buf, err = cr.r.ReadValueBlock(ctx, env, cr.rh, h)
 		if err != nil {
 			return nil, err
 		}
-		cr.currentValueBlock.dec = (*blobValueBlockDecoder)(unsafe.Pointer(cr.currentValueBlock.buf.BlockMetadata()))
+		cr.currentValueBlock.dec = block.CastMetadata[blobValueBlockDecoder](cr.currentValueBlock.buf.BlockMetadata())
 		cr.currentValueBlock.physicalIndex = physicalBlockIndex
 		cr.currentValueBlock.virtualID = vh.BlockID
 		cr.currentValueBlock.valueIDOffset = valueIDOffset
@@ -311,6 +313,10 @@ func (cfr *cachedReader) Close() (err error) {
 	if cfr.rh != nil {
 		err = cfr.rh.Close()
 	}
+	// Nil out the decoders before releasing the buffers to ensure the Go GC
+	// doesn't misinterpret the freed memory backing the decoders.
+	cfr.indexBlock.dec = nil
+	cfr.currentValueBlock.dec = nil
 	cfr.indexBlock.buf.Release()
 	cfr.currentValueBlock.buf.Release()
 	// Release the cfg.Reader. closeFunc is provided by the file cache and

sstable/block/block.go

@@ -12,6 +12,7 @@ import (
 	"runtime"
 	"slices"
 	"time"
+	"unsafe"
 
 	"github.com/cespare/xxhash/v2"
 	"github.com/cockroachdb/crlib/crtime"
@@ -219,12 +220,41 @@ func ValidateChecksum(checksumType ChecksumType, b []byte, bh Handle) error {
 // when the block is read from disk.
 //
 // Portions of this buffer can be cast to the structures we need (through
-// unsafe.Pointer), but note that any pointers in these structures will be
-// invisible to the GC. Pointers to the block's data buffer are ok, since the
-// metadata and the data have the same lifetime (sharing the underlying
-// allocation).
+// CastMetadata[Zero]), but note that any pointers in these structures should be
+// considered invisible to the GC for the purpose of preserving lifetime.
+// Pointers to the block's data buffer are ok, since the metadata and the data
+// have the same lifetime (sharing the underlying allocation).
 type Metadata [MetadataSize]byte
 
+// CastMetadataZero casts the provided metadata to the type parameter T, zeroing
+// the memory backing the metadata first. This zeroing is necessary when first
+// initializing the data structure to ensure that the Go garbage collector
+// doesn't misinterpret any of T's pointer fields, falsely detecting them as
+// invalid pointers.
+func CastMetadataZero[T any](md *Metadata) *T {
+	var z T
+	if invariants.Enabled {
+		if uintptr(unsafe.Pointer(md))%unsafe.Alignof(z) != 0 {
+			panic(errors.AssertionFailedf("incorrect alignment for %T (%p)", z, unsafe.Pointer(md)))
+		}
+	}
+	clear((*md)[:unsafe.Sizeof(z)])
+	return (*T)(unsafe.Pointer(md))
+}
+
+// CastMetadata casts the provided metadata to the type parameter T. If the
+// Metadata has not already been initialized, callers should use
+// CastMetadataZero.
+func CastMetadata[T any](md *Metadata) *T {
+	var z T
+	if invariants.Enabled {
+		if uintptr(unsafe.Pointer(md))%unsafe.Alignof(z) != 0 {
+			panic(fmt.Sprintf("incorrect alignment for %T (%p)", z, unsafe.Pointer(md)))
+		}
+	}
+	return (*T)(unsafe.Pointer(md))
+}
+
 // MetadataSize is the size of the metadata. The value is chosen to fit a
 // colblk.DataBlockDecoder and a CockroachDB colblk.KeySeeker.
 const MetadataSize = 336

sstable/block/buffer_pool.go

@@ -177,8 +177,9 @@ func (p *BufferPool) Release() {
 		if p.pool[i].b != nil {
 			panic(errors.AssertionFailedf("Release called on a BufferPool with in-use buffers"))
 		}
-		cache.Free(p.pool[i].v)
+		v := p.pool[i].v
 		p.pool[i].v = nil
+		cache.Free(v)
 	}
 	p.pool = p.pool[:0]
 }

sstable/colblk/data_block.go

@@ -802,20 +802,22 @@ const _ uint = block.MetadataSize - uint(dataBlockDecoderSize) - KeySeekerMetada
 
 // InitDataBlockMetadata initializes the metadata for a data block.
 func InitDataBlockMetadata(schema *KeySchema, md *block.Metadata, data []byte) (err error) {
-	if uintptr(unsafe.Pointer(md))%8 != 0 {
-		return errors.AssertionFailedf("metadata is not 8-byte aligned")
+	type blockDecoderAndKeySeekerMetadata struct {
+		d DataBlockDecoder
+		// Pad to ensure KeySeekerMetadata is 8-byte aligned.
+		_             [dataBlockDecoderSize - unsafe.Sizeof(DataBlockDecoder{})]byte
+		keySchemaMeta KeySeekerMetadata
 	}
-	d := (*DataBlockDecoder)(unsafe.Pointer(md))
+	metadatas := block.CastMetadataZero[blockDecoderAndKeySeekerMetadata](md)
 	// Initialization can panic; convert panics to corruption errors (so higher
 	// layers can add file number and offset information).
 	defer func() {
 		if r := recover(); r != nil {
 			err = base.CorruptionErrorf("error initializing data block metadata: %v", r)
 		}
 	}()
-	d.Init(schema, data)
-	keySchemaMeta := (*KeySeekerMetadata)(unsafe.Pointer(&md[dataBlockDecoderSize]))
-	schema.InitKeySeekerMetadata(keySchemaMeta, d)
+	metadatas.d.Init(schema, data)
+	schema.InitKeySeekerMetadata(&metadatas.keySchemaMeta, &metadatas.d)
 	return nil
 }
 
@@ -824,10 +826,7 @@ const _ uint = block.MetadataSize - uint(unsafe.Sizeof(IndexBlockDecoder{}))
 
 // InitIndexBlockMetadata initializes the metadata for an index block.
 func InitIndexBlockMetadata(md *block.Metadata, data []byte) (err error) {
-	if uintptr(unsafe.Pointer(md))%8 != 0 {
-		return errors.AssertionFailedf("metadata is not 8-byte aligned")
-	}
-	d := (*IndexBlockDecoder)(unsafe.Pointer(md))
+	d := block.CastMetadataZero[IndexBlockDecoder](md)
 	// Initialization can panic; convert panics to corruption errors (so higher
 	// layers can add file number and offset information).
 	defer func() {
@@ -844,10 +843,7 @@ const _ uint = block.MetadataSize - uint(unsafe.Sizeof(KeyspanDecoder{}))
 
 // InitKeyspanBlockMetadata initializes the metadata for a rangedel or range key block.
 func InitKeyspanBlockMetadata(md *block.Metadata, data []byte) (err error) {
-	if uintptr(unsafe.Pointer(md))%8 != 0 {
-		return errors.AssertionFailedf("metadata is not 8-byte aligned")
-	}
-	d := (*KeyspanDecoder)(unsafe.Pointer(md))
+	d := block.CastMetadataZero[KeyspanDecoder](md)
 	// Initialization can panic; convert panics to corruption errors (so higher
 	// layers can add file number and offset information).
 	defer func() {