Fix nil mb.cache dereference in secure removeMsgFromBlock

Project: nats-io/nats-server | Language: Go | Concepts: concurrency, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/nats-io/nats-server/pull/8548

The problem

The server could crash with a nil pointer dereference when erasing (securely deleting) a message from a message block if the block's cache expired while a lock (mb.mu) was temporarily released. This happened because the code assumed the cache was still present after reacquiring the lock, but in reality, a background process could have set mb.cache = nil in the meantime.

Why it's non-trivial

A naive fix might simply check the cache once at the start of the function, but this doesn't account for the fact that the lock is released and reacquired during the operation. During this window, other goroutines can expire or clear the cache. To be safe, the code must re-check and reload the cache after reacquiring the lock, not just at the beginning.

The approach

Guarding against cache expiry after reacquiring the lock:

After releasing and then reacquiring mb.mu, the code now checks if the cache is still loaded and reloads it if necessary:

if secure {
+   // The cache may have been expired or recycled while we dropped mb.mu above.
+   if mb.cacheNotLoaded() {
+       if err := mb.loadMsgsWithLock(); err != nil {
+           finishedWithCache()
+           mb.mu.Unlock()
+           return false, err
+       }
+   }
    // Grab record info, but use the pre-computed record length.
    ri, _, _, err := mb.slotInfo(int(seq - mb.cache.fseq))
    if err != nil {
        ...

What this does:

• After reacquiring the lock, it checks mb.cacheNotLoaded().
• If the cache is missing, it reloads it with mb.loadMsgsWithLock().
• If reloading fails, it cleans up and returns an error.
• This prevents dereferencing a nil cache pointer, which was the source of the crash.

Testing the race condition:

A new test simulates the exact race by rapidly expiring the cache in a goroutine while erasing messages:

// Expire the cache as fast as we can, like the expiration timer or a linear scan ending on this block.
stop := make(chan struct{})
var wg sync.WaitGroup
wg.Add(1)
go func() {
    defer wg.Done()
    for {
        select {
        case <-stop:
            return
        default:
            if mb.mu.TryLock() {
                mb.tryForceExpireCacheLocked()
                mb.mu.Unlock()
            }
        }
    }
}()
defer func() {
    close(stop)
    wg.Wait()
}()

What this does:

• Spawns a goroutine that repeatedly tries to expire the cache while the test runs.
• Uses TryLock to avoid deadlocks if the main thread panics while holding the lock.
• Ensures the race is exercised reliably.

The test then erases all but one message from the block, which would previously panic, but now passes.

From the review thread

The diagnosis is correct. removeMsgFromBlock drops mb.mu at line 6228 to write the delete tombstone into the last block. During that window, tryForceExpireCacheLocked (or the timer-driven tryExpireCacheLocked) can reach clearCache / finishedWithCache, setting mb.cache = nil. The secure branch then reads mb.cache.fseq (in slotInfo) and mb.cache.buf/mb.cache.wp (in eraseMsg) with no guard — hence the nil dereference at offset 0x38.

The fix is correct, minimal, and mirrors the existing pattern.

[review] neilalexander: You should be able to use defer require_NoPanic(t) at the top of the test instead.

[review] TzachiSh: Done, switched to defer require_NoPanic(t). The thrasher goroutine now uses TryLock and fs.Stop moved to the end of the test, otherwise a panicking erase still holding mb.mu would hang wg.Wait and Stop until the package timeout instead of failing cleanly. With the fix reverted it still fails 3/3, with it 20/20 pass under -race.

Takeaways

• Always re-check shared state after reacquiring a lock: If you release and reacquire a lock, other goroutines may have changed the state you depend on. Don't assume it is unchanged.
• Simulate races in tests to catch rare bugs: Intentionally triggering race conditions in tests (e.g., with goroutines rapidly mutating state) can reliably reproduce and verify fixes for concurrency bugs.
• Guard against nil dereferences in concurrent code: When shared pointers or resources may be cleared by other threads, always check for nil or reload as needed before dereferencing.