Skip to content

fix(orchestrator): close TOCTOU race in Cleanup.Add/AddPriority - #3558

Open
AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:fix/cleanup-toctou-race
Open

fix(orchestrator): close TOCTOU race in Cleanup.Add/AddPriority#3558
AdaAibaby wants to merge 1 commit into
e2b-dev:mainfrom
AdaAibaby:fix/cleanup-toctou-race

Conversation

@AdaAibaby

@AdaAibaby AdaAibaby commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Problem

Closes #3557

Cleanup.Add and Cleanup.AddPriority had a TOCTOU race against Cleanup.run that silently discarded cleanup functions, causing permanent resource leaks on the host.

The race window:

Goroutine A (Add):   hasRun.Load() == false  →  [not yet holding mu]
Goroutine B (run):   hasRun.Store(true)  →  Lock()  →  drain slice  →  Unlock()
Goroutine A (Add):   Lock()  →  append(f)   ← f is in a drained slice, never executed

sync.Once in Run() prevents run() from firing a second time, so f is permanently lost — no log, no error, no signal.

With 28 call sites in the sandbox startup path registering operations like closing overlay filesystems, releasing network slots, removing Firecracker/UFFD sockets, and stopping the Firecracker process, any one of these dropped in a concurrent teardown leaves a leaked resource until the orchestrator restarts.

Fix

Two changes to cleanup.go:

1. Move hasRun.Store(true) inside the lock in run()

// before
func (c *Cleanup) run(ctx context.Context) {
    c.hasRun.Store(true)   // ← outside lock
    c.mu.Lock()
    defer c.mu.Unlock()
    ...
}

// after
func (c *Cleanup) run(ctx context.Context) {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.hasRun.Store(true)   // ← inside lock, closes the window
    ...
}

2. Add a double-check of hasRun after acquiring mu in Add/AddPriority

c.mu.Lock()
if c.hasRun.Load() {   // ← double-check: catches goroutines that lost the race
    c.mu.Unlock()
    err := f(context.WithoutCancel(ctx))
    ...
    return
}
defer c.mu.Unlock()
c.cleanup = append(c.cleanup, f)

The optimistic fast-path Load() before the lock is kept to avoid lock contention in the common post-run case (already-torn-down sandbox), but is no longer the only gate.

Test plan

  • Existing sandbox lifecycle tests pass
  • Manual: concurrent Add and Run calls no longer drop functions (race detector: go test -race ./packages/orchestrator/pkg/sandbox/...)

/cc @jakubno @dobrac @ValentaTomas @arkamar @tvi @tomassrnka Looking forward to your code review.

Cleanup.run() set hasRun before acquiring mu, while Add/AddPriority
read hasRun before acquiring mu. This created a window where a goroutine
could observe hasRun==false, lose the race to run(), and then append a
cleanup function into an already-drained slice — silently discarding it.

Fix:
- Move hasRun.Store(true) inside mu in run(), so the flag is only set
  while the slice is still accessible under the lock.
- Add a double-check of hasRun after acquiring mu in Add/AddPriority,
  so any function that loses the race is executed immediately rather
  than silently dropped.

The optimistic fast-path Load() before the lock is kept for the common
post-run case (avoids lock contention when the sandbox is fully torn
down), but is no longer the only gate.

Fixes e2b-dev#3557
@leonmeijer

Copy link
Copy Markdown

This duplicates #3136 but is the smaller, current-main, issue-linked patch. I recommend keeping this PR as the single implementation and closing #3136 after carrying over any useful concurrency regression case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(orchestrator): Cleanup.Add has TOCTOU race — cleanup functions silently dropped after Run()

3 participants