Repository: cockroachdb/pebble (go)

Issue:
colblk: bad pointer in Go heap
Apparent regression of #4172

https://github.com/cockroachdb/pebble/actions/runs/12712893491/job/35439536441

```
runtime: pointer 0xc0003f5012 to unallocated span span.base()=0xc0003f4000 span.limit=0xc0003f5fe0 span.state=0
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

goroutine 509 gp=0xc0004bec40 m=8 mp=0xc000091c08 [running]:
runtime.throw({0xc1e895?, 0x44f827?})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/panic.go:1067 +0x48 fp=0xc000589ac0 sp=0xc000589a90 pc=0x476cc8
runtime.badPointer(0x7fa961fd7e68, 0xc0003f5012, 0x0, 0x0)
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/mbitmap.go:1247 +0x165 fp=0xc000589b10 sp=0xc000589ac0 pc=0x415fc5
runtime.findObject(0x589b68?, 0x4800b9?, 0x4800b9?)
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/mbitmap.go:1299 +0xa6 fp=0xc000589b48 sp=0xc000589b10 pc=0x475466
runtime.checkptrBase(0xc0003f5012)
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/checkptr.go:104 +0x3e fp=0xc000589b[70](https://github.com/cockroachdb/pebble/actions/runs/12712893491/job/35439536441#step:4:71) sp=0xc000589b48 pc=0x4707de
runtime.checkptrArithmetic(0xc000589bb8?, {0xc000589c98, 0x1, 0xc000589ba0?})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/checkptr.go:58 +0x38 fp=0xc000589ba0 sp=0xc000589b70 pc=0x40ae98
github.com/cockroachdb/pebble/sstable/colblk.UnsafeUints.At(...)
	/home/runner/work/pebble/pebble/sstable/colblk/unsafe_slice.go:121
github.com/cockroachdb/pebble/sstable/colblk.(*IndexIter).BlockHandleWithProperties(0xc000589ea0)
	/home/runner/work/pebble/pebble/sstable/colblk/index_block.go:314 +0x1d2 fp=0xc000589d10 sp=0xc000589ba0 pc=0x9e2a[72](https://github.com/cockroachdb/pebble/actions/runs/12712893491/job/35439536441#step:4:73)
github.com/cockroachdb/pebble/sstable/colblk.TestIndexIterInitHandle.func1(0xc000589ea0)
	/home/runner/work/pebble/pebble/sstable/colblk/index_block_test.go:136 +0x245 fp=0xc000589e60 sp=0xc000589d10 pc=0xa094e5
github.com/cockroachdb/pebble/sstable/colblk.TestIndexIterInitHandle.func2()
	/home/runner/work/pebble/pebble/sstable/colblk/index_block_test.go:157 +0xfe fp=0xc000589fe0 sp=0xc000589e60 pc=0xa091fe
runtime.goexit({})
	/opt/hostedtoolcache/go/1.23.4/x64/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc000589fe8 sp=0xc000589fe0 pc=0x47ea01
created by github.com/cockroachdb/pebble/sstable/colblk.TestIndexIterInitHandle in goroutine 504
	/home/runner/work/pebble/pebble/sstable/colblk/index_block_test.go:153 +0x58a
```

Jira issue: PEBBLE-320

Pull request:
colblk: fix decoder initialization in TestIndexIterInitHandle
We were initializing the index block decoder using the go data buffer
(resulting in that buffer only being referenced from Cgo memory). Fix
the `Init` call to use the actual buffer.

Fixes #4242

Patch:
diff --git a/internal/cache/clockpro.go b/internal/cache/clockpro.go
index 5307c033..c826b072 100644
--- a/internal/cache/clockpro.go
+++ b/internal/cache/clockpro.go
@@ -135,7 +135,9 @@ func (c *shard) init(maxSize int64) {
 
 // getWithMaybeReadEntry is the internal helper for implementing
 // Cache.{Get,GetWithReadHandle}. When desireReadEntry is true, and the block
-// is not in the cache (nil Value), a non-nil readEntry is returned.
+// is not in the cache (nil Value), a non-nil readEntry is returned (in which
+// case the caller is responsible to dereference the entry, via one of
+// unrefAndTryRemoveFromMap(), setReadValue(), setReadError()).
 func (c *shard) getWithMaybeReadEntry(k key, desireReadEntry bool) (*Value, *readEntry) {
 	c.mu.RLock()
 	var value *Value
@@ -160,7 +162,7 @@ func (c *shard) getWithMaybeReadEntry(k key, desireReadEntry bool) (*Value, *rea
 			}
 		}
 		if value == nil {
-			re = c.readShard.getReadEntryLocked(k)
+			re = c.readShard.acquireReadEntryLocked(k)
 		}
 		c.mu.Unlock()
 	}
@@ -865,6 +867,7 @@ func (c *Cache) GetWithReadHandle(
 	}
 	cv, errorDuration, err = re.waitForReadPermissionOrHandle(ctx)
 	if err != nil || cv != nil {
+		re.unrefAndTryRemoveFromMap()
 		return cv, ReadHandle{}, errorDuration, false, err
 	}
 	return nil, ReadHandle{entry: re}, errorDuration, false, nil
diff --git a/internal/cache/read_shard.go b/internal/cache/read_shard.go
index ee8bd90a..5a1328c9 100644
--- a/internal/cache/read_shard.go
+++ b/internal/cache/read_shard.go
@@ -86,9 +86,9 @@ func (rs *readShard) Init(shard *shard) *readShard {
 	return rs
 }
 
-// getReadEntryLocked gets a *readEntry for (id, fileNum, offset). shard.mu is
+// acquireReadEntryLocked gets a *readEntry for (id, fileNum, offset). shard.mu is
 // already write locked.
-func (rs *readShard) getReadEntryLocked(k key) *readEntry {
+func (rs *readShard) acquireReadEntryLocked(k key) *readEntry {
 	e, ok := rs.shardMu.readMap.Get(k)
 	if !ok {
 		e = newReadEntry(rs, k)
@@ -207,23 +207,14 @@ func (e *readEntry) waitForReadPermissionOrHandle(
 		}
 		e.mu.readStart = time.Now()
 	}
-	unlockAndUnrefAndTryRemoveFromMap := func(readLock bool) (errorDuration time.Duration) {
-		errorDuration = e.mu.errorDuration
-		if readLock {
-			e.mu.RUnlock()
-		} else {
-			e.mu.Unlock()
-		}
-		e.unrefAndTryRemoveFromMap()
-		return errorDuration
-	}
 
 	for {
 		e.mu.Lock()
 		if e.mu.v != nil {
 			// Value has already been read.
-			cv := constructValueLocked()
-			errorDuration = unlockAndUnrefAndTryRemoveFromMap(false)
+			cv = constructValueLocked()
+			errorDuration = e.mu.errorDuration
+			e.mu.Unlock()
 			return cv, errorDuration, nil
 		}
 		// Not already read. Wait for turn to do the read or for someone else to do
@@ -245,7 +236,8 @@ func (e *readEntry) waitForReadPermissionOrHandle(
 		select {
 		case <-ctx.Done():
 			e.mu.RLock()
-			errorDuration = unlockAndUnrefAndTryRemoveFromMap(true)
+			errorDuration = e.mu.errorDuration
+			e.mu.RUnlock()
 			return nil, errorDuration, ctx.Err()
 		case _, ok := <-ch:
 			if !ok {
@@ -255,7 +247,8 @@ func (e *readEntry) waitForReadPermissionOrHandle(
 					panic("value is nil")
 				}
 				h := constructValueLocked()
-				errorDuration = unlockAndUnrefAndTryRemoveFromMap(true)
+				errorDuration = e.mu.errorDuration
+				e.mu.RUnlock()
 				return h, errorDuration, nil
 			}
 			// Else, probably granted permission to do the read. NB: since isReading


Test patch:
diff --git a/internal/cache/read_shard_test.go b/internal/cache/read_shard_test.go
index be06b5e8..3816a491 100644
--- a/internal/cache/read_shard_test.go
+++ b/internal/cache/read_shard_test.go
@@ -56,20 +56,26 @@ func (r *testReader) getAsync(shard *shard) *string {
 	r.re = re
 	go func() {
 		v, _, err := re.waitForReadPermissionOrHandle(r.ctx)
+
 		r.mu.Lock()
 		defer r.mu.Unlock()
-		r.mu.finishedWait = true
-		r.mu.cond.Signal()
-		if v != nil {
+
+		switch {
+		case v != nil:
+			re.unrefAndTryRemoveFromMap()
 			r.mu.waitedValue = string(v.RawBuffer())
 			v.Release()
-			return
-		}
-		if err != nil {
+
+		case err != nil:
+			re.unrefAndTryRemoveFromMap()
 			r.mu.waitedErr = err
-			return
+
+		default:
+			r.mu.waitedNeedToRead = true
 		}
-		r.mu.waitedNeedToRead = true
+
+		r.mu.finishedWait = true
+		r.mu.cond.Signal()
 	}()
 	return nil
 }
diff --git a/sstable/colblk/index_block_test.go b/sstable/colblk/index_block_test.go
index 12a9224a..c6fc7224 100644
--- a/sstable/colblk/index_block_test.go
+++ b/sstable/colblk/index_block_test.go
@@ -120,12 +120,13 @@ func TestIndexIterInitHandle(t *testing.T) {
 	c := cache.New(10 << 10)
 	defer c.Unref()
 
-	v := block.Alloc(block.MetadataSize+len(blockData), nil)
-	copy(v.BlockData(), blockData)
-	d := (*IndexBlockDecoder)(unsafe.Pointer(v.BlockMetadata()))
-	d.Init(blockData)
-
-	v.SetInCacheForTesting(c, cache.ID(1), base.DiskFileNum(1), 0)
+	{
+		v := block.Alloc(len(blockData), nil)
+		copy(v.BlockData(), blockData)
+		d := (*IndexBlockDecoder)(unsafe.Pointer(v.BlockMetadata()))
+		d.Init(v.BlockData())
+		v.SetInCacheForTesting(c, cache.ID(1), base.DiskFileNum(1), 0)
+	}
 
 	getBlockAndIterate := func(it *IndexIter) {
 		cv := c.Get(cache.ID(1), base.DiskFileNum(1), 0)