arenaskl,batchskl: add test ensuring node contains no pointer types

Add a test asserting that the node structs used in both skiplist
implementations does not contain any pointer types. We reuse memory backing
both of these types without zeroing it. If we introduce a pointer type, we
could expose ourselves to an issue similar to the one fixed in #5085: garbage
memory prior could be misinterpreted as a pointer value before the pointer
value has been appropriately initialized.

Author: jbowens

internal/arenaskl/skl_test.go

@@ -22,13 +22,15 @@ import (
 	"encoding/binary"
 	"fmt"
 	"math/rand/v2"
+	"reflect"
 	"strconv"
 	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/pebble/internal/base"
+	"github.com/cockroachdb/pebble/internal/testutils"
 	"github.com/stretchr/testify/require"
 )
 
@@ -77,6 +79,15 @@ func lengthRev(s *Skiplist) int {
 	return count
 }
 
+// TestNoPointers tests that the node struct does not contain any pointers. No
+// struct that is backed by the arena may contain pointers (at least without
+// zeroing the backing memory) *before* type casting the pointer. Otherwise, the
+// Go GC may observe the pointer (while a GC write barrier is in effect) and
+// complain that the uninitialized value is a bad pointer. See 273e2665.
+func TestNoPointers(t *testing.T) {
+	require.False(t, testutils.AnyPointers(reflect.TypeOf(node{})))
+}
+
 func TestEmpty(t *testing.T) {
 	key := makeKey("aaa")
 	l := NewSkiplist(newArena(arenaSize), bytes.Compare)

internal/batchskl/skl_test.go

@@ -21,11 +21,13 @@ import (
 	"encoding/binary"
 	"fmt"
 	"math/rand/v2"
+	"reflect"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/internal/base"
+	"github.com/cockroachdb/pebble/internal/testutils"
 	"github.com/stretchr/testify/require"
 )
 
@@ -82,6 +84,15 @@ func newTestSkiplist(storage *testStorage) *Skiplist {
 		base.DefaultComparer.AbbreviatedKey)
 }
 
+// TestNoPointers tests that the node struct does not contain any pointers. No
+// struct that is backed by the arena may contain pointers (at least without
+// zeroing the backing memory) *before* type casting the pointer. Otherwise, the
+// Go GC may observe the pointer (while a GC write barrier is in effect) and
+// complain that the uninitialized value is a bad pointer. See 273e2665.
+func TestNoPointers(t *testing.T) {
+	require.False(t, testutils.AnyPointers(reflect.TypeOf(node{})))
+}
+
 func TestEmpty(t *testing.T) {
 	key := makeKey("aaa")
 	l := newTestSkiplist(&testStorage{})

internal/testutils/reflect.go

@@ -0,0 +1,72 @@
+// Copyright 2024 The LevelDB-Go and Pebble Authors. All rights reserved. Use
+// of this source code is governed by a BSD-style license that can be found in
+// the LICENSE file.
+
+package testutils
+
+import (
+	"fmt"
+	"reflect"
+)
+
+// AnyPointers returns true if the provided type contains any pointers.
+func AnyPointers(typ reflect.Type) bool {
+	kind := typ.Kind()
+	switch kindPointers[kind] {
+	case kindHasPointer:
+		return true
+	case kindNoPointer:
+		return false
+	}
+	switch kind {
+	case reflect.Struct:
+		for i := range typ.NumField() {
+			if AnyPointers(typ.Field(i).Type) {
+				return true
+			}
+		}
+		return false
+	case reflect.Array:
+		return AnyPointers(typ.Elem())
+	default:
+		panic(fmt.Sprintf("unexpected kind: %s", kind))
+	}
+}
+
+type anyPointers int8
+
+const (
+	kindNoPointer anyPointers = iota
+	kindHasPointer
+	kindMaybeHasPointer
+)
+
+var kindPointers = []anyPointers{
+	reflect.Invalid:       kindNoPointer,
+	reflect.Bool:          kindNoPointer,
+	reflect.Int:           kindNoPointer,
+	reflect.Int8:          kindNoPointer,
+	reflect.Int16:         kindNoPointer,
+	reflect.Int32:         kindNoPointer,
+	reflect.Int64:         kindNoPointer,
+	reflect.Uint:          kindNoPointer,
+	reflect.Uint8:         kindNoPointer,
+	reflect.Uint16:        kindNoPointer,
+	reflect.Uint32:        kindNoPointer,
+	reflect.Uint64:        kindNoPointer,
+	reflect.Uintptr:       kindNoPointer,
+	reflect.Float32:       kindNoPointer,
+	reflect.Float64:       kindNoPointer,
+	reflect.Complex64:     kindNoPointer,
+	reflect.Complex128:    kindNoPointer,
+	reflect.Array:         kindMaybeHasPointer,
+	reflect.Chan:          kindHasPointer,
+	reflect.Func:          kindHasPointer,
+	reflect.Interface:     kindHasPointer,
+	reflect.Map:           kindHasPointer,
+	reflect.Pointer:       kindHasPointer,
+	reflect.Slice:         kindHasPointer,
+	reflect.String:        kindHasPointer,
+	reflect.Struct:        kindMaybeHasPointer,
+	reflect.UnsafePointer: kindHasPointer,
+}