Repository navigation
bug(orchestrator): Cleanup.Add has TOCTOU race — cleanup functions silently dropped after Run() #3557
Description
Summary
Cleanup.Add and Cleanup.AddPriority have a TOCTOU (time-of-check/time-of-use) race against Cleanup.run that silently discards cleanup functions without executing them, causing permanent resource leaks.
Root Cause
In packages/orchestrator/pkg/sandbox/cleanup.go:
run() sets hasRun before acquiring the lock (line 80 vs 82):
func (c *Cleanup) run(ctx context.Context) {
c.hasRun.Store(true) // ← written outside the lock
c.mu.Lock()
defer c.mu.Unlock()
// ... drains c.cleanup and c.priorityCleanup
}Add() reads hasRun before acquiring the lock (line 40 vs 49):
func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
if c.hasRun.Load() == true { // ← read outside the lock
// run f immediately
return
}
c.mu.Lock()
defer c.mu.Unlock()
c.cleanup = append(c.cleanup, f) // ← appended after run() may have drained
}Race Window
Goroutine A (Add): hasRun.Load() == false → [not yet locked]
Goroutine B (run): hasRun.Store(true) → Lock() → drain all cleanup → Unlock()
Goroutine A (Add): Lock() → append(f) ← f is now in a drained slice, never executed
sync.Once in Run() prevents run() from being called a second time, so f is permanently lost — no log, no error, no signal.
Impact
The Cleanup type is the central resource-release mechanism for Firecracker sandbox lifecycle. The package has 28 call sites across sandbox startup (sandbox.go, resume.go, reboot.go, etc.) registering operations such as:
- Closing overlay filesystems
- Releasing network slots
- Removing Firecracker and UFFD socket files
- Stopping the Firecracker process
Any one of these silently dropped during a concurrent error-path teardown leaves a leaked resource on the host until the orchestrator restarts.
Fix
Move hasRun.Store(true) inside the lock in run(), and add a double-check in Add() / AddPriority() after acquiring mu:
func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
// Optimistic fast path.
if c.hasRun.Load() {
err := f(context.WithoutCancel(ctx))
if err != nil {
logger.L().Error(ctx, "failed to run function after cleanup has run", zap.Error(err))
}
return
}
c.mu.Lock()
// Double-check: run() may have completed between the Load above and here.
if c.hasRun.Load() {
c.mu.Unlock()
err := f(context.WithoutCancel(ctx))
if err != nil {
logger.L().Error(ctx, "failed to run function after cleanup has run", zap.Error(err))
}
return
}
c.cleanup = append(c.cleanup, f)
c.mu.Unlock()
}
func (c *Cleanup) run(ctx context.Context) {
c.mu.Lock()
defer c.mu.Unlock()
c.hasRun.Store(true) // now set under the lock — closes the race window
// ... rest unchanged
}The same double-check applies to AddPriority.
Activity
ryan-sana commentedon Oct 12, 2026
Fixed on main by 9cc9ca7: Cleanup.Add and AddPriority now check hasRun and append while holding the same lock that Run uses to copy the lists, so a callback added late runs immediately instead of being dropped. The commit includes race tests.