testing: potential issues in runCleanup - infinite recursion on cancelCtx panic and incorrect nil check logic

[dxasu]

Issue 1: Infinite recursion if c.cancelCtx()

go/src/testing/testing.go

Line 1651 in ad91f5d

if c.cancelCtx != nil {

If c.cancelCtx() panics and len(c.cleanups) > 0, the following happens:

1. c.cancelCtx() panics
2. The deferred function checks len(c.cleanups) > 0 → true
3. Recursively calls runCleanup(normalPanic)
4. c.cancelCtx is still not nil, calls c.cancelCtx() again
5. Panics again → infinite recursion → stack overflow

Suggested fix:

if c.cancelCtx != nil {
    cancel := c.cancelCtx
    c.cancelCtx = nil  // clear before calling to prevent repeated calls
    cancel()
}

Issue 2: Incorrect nil check conflates empty slice with nil element

https://github.com/golang/go/blob/ad91f5d241f3b8e85dc866d0087c3f13af96ef33/src/testing/testing.go#L1655C1-L1668C3

The current logic cannot distinguish between:

• Case A: c.cleanups is empty (cleanup stays default nil)
• Case B: c.cleanups contains a nil function pointer

In Case B, returning early would skip remaining cleanup functions.

Suggested fix:

for {
    c.mu.Lock()
    if len(c.cleanups) == 0 {
        c.mu.Unlock()
        return nil
    }
    last := len(c.cleanups) - 1
    cleanup := c.cleanups[last]
    c.cleanups = c.cleanups[:last]
    c.mu.Unlock()
    
    if cleanup != nil {
        cleanup()
    }
    // continue to process remaining cleanups even if this one was nil
}

[gabyhelp]

Related Issues

• proposal: testing: t.Cleanup should run on panic #41355 (closed)
• testing: Don't silently ignore nested t.Cleanup calls #41085 (closed)
• testing: reject calls to Run within Cleanup callbacks #48515 (closed)
• testing: failures in Cleanup functions do not trigger -failfast if the test includes a parallel subtest #61034
• testing: panic swallowed if t.Cleanup calls t.Fatal #51395 (closed)
• testing: parent B.Cleanup() functions do not run if a sub-benchmark panics #60129
• testing: b.Cleanup(nil) panics #38346 (closed)
• testing: misleading error message when a parallel Cleanup function calls runtime.Goexit #48502 (closed)

Related Code Changes

• testing: reject calls to Run within Cleanup callbacks
• testing: allow m.Run return if a test panics

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

can you write code that triggers these issues?

[dxasu]

can you write code that triggers these issues?

Let me clarify with a more concrete scenario:

Issue 1 Clarification: cancelCtx is called multiple times

Even without panicking, if any cleanup() function panics, cancelCtx() will be
called multiple times due to the recursive structure:

func (c *common) runCleanup(ph panicHandling) (panicVal any) {
    defer func() {
        if recur := len(c.cleanups) > 0; recur {
            c.runCleanup(normalPanic)  // recursive call
        }
    }()

    if c.cancelCtx != nil {
        c.cancelCtx()  // called on EVERY recursion!
    }
    
    for {
        cleanup()  // if this panics, recursion happens
    }
}

Execution flow when a cleanup panics:

1. First runCleanup(): cancelCtx() called (1st time)
2. cleanup() panics
3. Defer triggers recursive runCleanup(normalPanic)
4. Second runCleanup(): cancelCtx() called again (2nd time)!

While context.CancelFunc is idempotent, this is still:

• Unnecessary redundant execution
• A logic error in the code structure

Suggested fix:

if c.cancelCtx != nil {
    cancel := c.cancelCtx
    c.cancelCtx = nil  // prevent re-execution
    cancel()
}

This ensures cancelCtx is called exactly once, regardless of cleanup panics.

Issue 2: Incorrect nil check conflates empty slice with nil element

The Cleanup() method wraps user functions in a closure before appending
to cleanups, so nil elements cannot actually be stored.

However, from a defensive programming perspective, the current code in runCleanup:

• Conflates "empty slice" with "nil element" in the same check

If you consider these as "won't fix" since they cannot be triggered in practice,
I completely understand. I raised this issue mainly from a code robustness standpoint.

[randall77]

Issue 1:

We still need a reproducer. A full program that gets into an infinite recursion and overflows its stack.
Or is this not triggerable currently? I'm not sure from your report.

Issue 2:

I believe every entry in cleanups is non-nil, so this issue is not a problem (although a comment to that effect on the cleanups field would be nice).
We do not generally do defensive programming, at least with respect to package-internal invariants we know hold.

[dxasu]

Issue 1: cancelCtx called multiple times

I want to correct my original description. The issue is not about infinite recursion,
but about cancelCtx() being called multiple times when a cleanup function panics.

Reproduction:

package main_test

import (
    "fmt"
    "testing"
)

func TestCleanupPanic(t *testing.T) {
    t.Cleanup(func() {
        fmt.Println("cleanup 1")
    })
    
    t.Cleanup(func() {
        fmt.Println("cleanup 2 - will panic")
        panic("oops")
    })
    
    t.Cleanup(func() {
        fmt.Println("cleanup 3")
    })
}

When cleanup 2 panics:

1. First runCleanup() call: cancelCtx() executed ✓
2. Panic triggers the deferred recursive call
3. Second runCleanup() call: cancelCtx() executed again ✓

While context.CancelFunc is idempotent so this doesn't cause visible bugs,
it's still redundant execution. A simple fix would be:

if c.cancelCtx != nil {
    cancel := c.cancelCtx
    c.cancelCtx = nil
    cancel()
}

If this is considered acceptable behavior (since CancelFunc is idempotent),
I understand and we can close Issue 1.

Issue 2: Redundant nil check logic (not defensive programming)

You're right that cleanups entries are always non-nil. My concern is actually about
redundant/confusing logic, not defensive programming.
The current code:

for {
    var cleanup func()
    c.mu.Lock()
    if len(c.cleanups) > 0 {      // Check 1: slice not empty
        last := len(c.cleanups) - 1
        cleanup = c.cleanups[last]
        c.cleanups = c.cleanups[:last]
    }
    c.mu.Unlock()
    if cleanup == nil {            // Check 2: cleanup is nil
        return nil
    }
    cleanup()
}

The two checks are logically redundant:

• If len(c.cleanups) > 0, then cleanup will be non-nil (given the invariant)
• If len(c.cleanups) == 0, then cleanup stays nil

A cleaner version would be:

for {
    c.mu.Lock()
    if len(c.cleanups) == 0 {
        c.mu.Unlock()
        return nil
    }
    last := len(c.cleanups) - 1
    cleanup := c.cleanups[last]
    c.cleanups = c.cleanups[:last]
    c.mu.Unlock()
    cleanup()
}

This removes the redundant nil check and makes the exit condition clearer.

[randall77]

Is any of this observable by a client of testing?

[dxasu]

Is any of this observable by a client of testing?

Honestly, no, these are not observable by clients of testing:

1. Issue 1: context.CancelFunc is idempotent, so calling it multiple times
   has no observable side effects for users.

2. Issue 2: This is purely an internal implementation detail with no external impact.

I acknowledge these are code quality observations rather than user-facing bugs.
The behavior is correct from a user's perspective.

[seankhliao]

Since this is not observable by user code, closing as there's nothing to do.