Skip to content

Fix mem leak in newHcsTask() #2354

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
kiashok wants to merge 1 commit into from
Closed

Fix mem leak in newHcsTask() #2354

Show file tree
Hide file tree
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
16 changes: 9 additions & 7 deletions cmd/containerd-shim-runhcs-v1/task_hcs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ func newHcsTask(
if parent != nil {
// We have a parent UVM. Listen for its exit and forcibly close this
// task. This is not expected but in the event of a UVM crash we need to
// handle this case.
go ht.waitForHostExit()
// ensure the resources are cleaned up as expected.
go ht.waitForHostOrContainerExit()
}

go ht.waitInitExit()
Expand Down Expand Up @@ -616,19 +616,21 @@ func (ht *hcsTask) waitInitExit() {
ht.close(ctx)
}

// waitForHostExit waits for the host virtual machine to exit. Once exited
// forcibly exits all additional exec's in this task.
// waitForHostOrContainerExit waits for the host virtual machine to exit. Once exited,
// it forcibly exits all additional execs in this task. Make sure to check
// for container exit as well since the container could exit before
// the UVM and leak this goroutine started for its task.
//
// This MUST be called via a goroutine to wait on a background thread.
//
// Note: For Windows process isolated containers there is no host virtual
// machine so this should not be called.
func (ht *hcsTask) waitForHostExit() {
ctx, span := oc.StartSpan(context.Background(), "hcsTask::waitForHostExit")
func (ht *hcsTask) waitForHostOrContainerExit() {
ctx, span := oc.StartSpan(context.Background(), "hcsTask::waitForHostOrContainerExit")
defer span.End()
span.AddAttributes(trace.StringAttribute("tid", ht.id))

err := ht.host.WaitCtx(ctx)
err := ht.host.WaitForUvmOrContainerExit(ctx, ht.c)
if err != nil {
log.G(ctx).WithError(err).Error("failed to wait for host virtual machine exit")
} else {
Expand Down
24 changes: 24 additions & 0 deletions internal/uvm/wait.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,33 @@ import (

"github.com/sirupsen/logrus"

"github.com/Microsoft/hcsshim/internal/cow"
"github.com/Microsoft/hcsshim/internal/logfields"
)

// WaitForUvmOrContainerExit waits for the container `c` or its UVM
// to exit. This is used to clean up hcs task and exec resources by
// the caller.
func (uvm *UtilityVM) WaitForUvmOrContainerExit(ctx context.Context, c cow.Container) (err error) {
select {
case <-c.WaitChannel():
return c.WaitError()
case <-uvm.hcsSystem.WaitChannel():
logrus.WithField(logfields.UVMID, uvm.id).Debug("uvm exited, waiting for output processing to complete")
var outputErr error
if uvm.outputProcessingDone != nil {
select {
case <-uvm.outputProcessingDone:
case <-ctx.Done():
outputErr = fmt.Errorf("failed to wait on uvm output processing: %w", ctx.Err())
}
}
return errors.Join(uvm.hcsSystem.WaitError(), outputErr)
case <-ctx.Done():
return ctx.Err()
}
}
Comment on lines +16 to +37
Copy link
Contributor

@helsaawy helsaawy Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if feels weird to couple the two together, since this duplicates a lot of logic thats in WaitCtx, and this is a very specialized case of waiting on two different channels with a context timeout.

maybe you can add Done() and WaitError functions to the uVM (similar to what we do for containers) that abstracts away the wait logic.

That way waitForHostOrContainerExit can select on them, and we can avoid the current differentiation in the logic if the timeout happens during the wait on the compute system or the output processing

//
// internal\uvm\types.go
//

type UtilityVM struct {
    // ...

	waitErr error
	waitCh  chan struct{}

	exitErr error
	exitCh  chan struct{}

    // ...
}

//
// internal\uvm\start.go
//

func (uvm *UtilityVM) Start(ctx context.Context) (err error) {
    // ...

	// Start waiting on the utility VM.
	go func() {
		// the original context may have timeout or propagate a cancellation
		// copy the original to prevent it affecting the background wait go routine
		cCtx := context.WithoutCancel(pCtx)
		uvm.waitErr := uvm.hcsSystem.WaitCtx(cCtx)

		if uvm.waitErr == nil {
			uvm.exitErr = uvm.hcsSystem.ExitError()
		} else {
			uvm.exitErr = uvm.waitErr
		}
		close(uvm.exitCh)

		if uvm.outputProcessingDone != nil {
			logrus.WithField(logfields.UVMID, uvm.id).Debug("uvm exited, waiting for output processing to complete")
			<-uvm.outputProcessingDone:
		}
		close(uvm.waitCh)
	}()

    // ...
}

//
// internal\uvm\wait.go
//

func (uvm *UtilityVM) Done() <-chan struct{} { return uvm.waitCh }

func (uvm *UtilityVM) WaitError() { return uvm.waitErr }

func (uvm *UtilityVM) WaitCtx(ctx context.Context) (err error) {
	select {
		case <-uvm.waitCh:
			err = uvm.waitErr
		case <-ctx.Done():
			err = fmt.Errorf("failed to wait on uvm: %w", ctx.Err())
		}
	}

	return err
}

//
// cmd/containerd-shim-runhcs-v1/task_hcs.go
//

func (ht *hcsTask) waitForHostOrContainerExit() {
	// ...

	var err error
	select {
	case <- ht.host.Done():
		err = ht.host.WaitError()
	case <- ht.c.WaitChannel()
		err = ht.c.WaitError()
	}

	// ...
}

Copy link
Contributor Author

@kiashok kiashok Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the long term fix in mind. There are callers to uvm.Wait() and uvm.WaitCtx() that might be expecting outputProcessDone (that is IO to complete along with hcsSystem termination too) and we need to account for them as well before removing from uvm.WaitCtx(). Additionally, hcsshim lifecycle management could be looked into and optimized to make things easier as well. This needs additional validation and careful consideration. Since there is an urgency to fix this leak, wanted to lean more towards a quicker least risky fix to ensure no additional regressions and revisit on a cleaner long term fix.


// Wait waits synchronously for a utility VM to terminate.
func (uvm *UtilityVM) Wait() error { return uvm.WaitCtx(context.Background()) }

Expand Down
Loading