Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AdaAibaby wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions packages/orchestrator/pkg/sandbox/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func (c *Cleanup) AddNoContext(ctx context.Context, f func() error) {
}

func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
if c.hasRun.Load() == true {
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))
Expand All @@ -47,13 +47,25 @@ func (c *Cleanup) Add(ctx context.Context, f func(ctx context.Context) error) {
}

c.mu.Lock()
// Double-check under lock: run() may have completed between the Load above
// and acquiring mu, silently dropping f into an already-drained slice.
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
}
defer c.mu.Unlock()

c.cleanup = append(c.cleanup, f)
}

func (c *Cleanup) AddPriority(ctx context.Context, f func(ctx context.Context) error) {
if c.hasRun.Load() == true {
if c.hasRun.Load() {
err := f(context.WithoutCancel(ctx))
if err != nil {
logger.L().Error(ctx, "failed to run priority function after cleanup has run", zap.Error(err))
Expand All @@ -63,6 +75,18 @@ func (c *Cleanup) AddPriority(ctx context.Context, f func(ctx context.Context) e
}

c.mu.Lock()
// Double-check under lock: run() may have completed between the Load above
// and acquiring mu, silently dropping f into an already-drained slice.
if c.hasRun.Load() {
c.mu.Unlock()

err := f(context.WithoutCancel(ctx))
if err != nil {
logger.L().Error(ctx, "failed to run priority function after cleanup has run", zap.Error(err))
}

return
}
defer c.mu.Unlock()

c.priorityCleanup = append(c.priorityCleanup, f)
Expand All @@ -77,11 +101,15 @@ func (c *Cleanup) Run(ctx context.Context) error {
}

func (c *Cleanup) run(ctx context.Context) {
c.hasRun.Store(true)

c.mu.Lock()
defer c.mu.Unlock()

// Set hasRun inside the lock so Add/AddPriority's double-check (also under
// mu) sees the updated value before the slice is drained. Without this,
// Add can observe hasRun==false, lose the race to run(), and append f to an
// already-drained slice where it will never be executed.
c.hasRun.Store(true)

var errs []error

for _, cleanup := range slices.Backward(c.priorityCleanup) {
Expand Down