libcontainer/windows: Fix process not being killed after stdio attach failure

vvoland · vvoland · commit 55b664046c44 · 2023-09-14T11:10:11.000+02:00
Error check in defer block used wrong error variable which is always nil
if the flow reaches the defer. This caused the `newProcess.Kill` to be
never called if the subsequent attemp to attach to the stdio failed.
Although this only happens in Exec (as Start does overwrite the error),
this also adjusts the Start to also use the returned error to avoid this
kind of mistake in future changes.

Signed-off-by: Paweł Gronowski &lt;pawel.gronowski@docker.com&gt;
diff --git a/libcontainerd/local/local_windows.go b/libcontainerd/local/local_windows.go
@@ -389,7 +389,7 @@ func (c *client) extractResourcesFromSpec(spec *specs.Spec, configuration *hcssh
 	}
 }
 
-func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Task, error) {
+func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Task, retErr error) {
 	ctr.mu.Lock()
 	defer ctr.mu.Unlock()
 
@@ -446,7 +446,7 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS
 	}
 
 	defer func() {
-		if err != nil {
+		if retErr != nil {
 			if err := newProcess.Kill(); err != nil {
 				logger.WithError(err).Error("failed to kill process")
 			}
@@ -557,7 +557,7 @@ func newIOFromProcess(newProcess hcsshim.Process, terminal bool) (*cio.DirectIO,
 // The processID argument is entirely informational. As there is no mechanism
 // (exposed through the libcontainerd interfaces) to enumerate or reference an
 // exec'd process by ID, uniqueness is not currently enforced.
-func (t *task) Exec(ctx context.Context, processID string, spec *specs.Process, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Process, error) {
+func (t *task) Exec(ctx context.Context, processID string, spec *specs.Process, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Process, retErr error) {
 	hcsContainer, err := t.getHCSContainer()
 	if err != nil {
 		return nil, err
@@ -610,7 +610,7 @@ func (t *task) Exec(ctx context.Context, processID string, spec *specs.Process,
 	}
 	pid := newProcess.Pid()
 	defer func() {
-		if err != nil {
+		if retErr != nil {
 			if err := newProcess.Kill(); err != nil {
 				logger.WithError(err).Error("failed to kill process")
 			}

Original file line number	Diff line number	Diff line change
`@@ -389,7 +389,7 @@ func (c client) extractResourcesFromSpec(spec specs.Spec, configuration *hcssh`
`389`	`389`	`}`
`390`	`390`	`}`
`391`	`391`
`392`		`-func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Task, error) {`
	`392`	`+func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Task, retErr error) {`
`393`	`393`	`ctr.mu.Lock()`
`394`	`394`	`defer ctr.mu.Unlock()`
`395`	`395`
`@@ -446,7 +446,7 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS`
`446`	`446`	`}`
`447`	`447`
`448`	`448`	`defer func() {`
`449`		`- if err != nil {`
	`449`	`+ if retErr != nil {`
`450`	`450`	`if err := newProcess.Kill(); err != nil {`
`451`	`451`	`logger.WithError(err).Error("failed to kill process")`
`452`	`452`	`}`
`@@ -557,7 +557,7 @@ func newIOFromProcess(newProcess hcsshim.Process, terminal bool) (*cio.DirectIO,`
`557`	`557`	`// The processID argument is entirely informational. As there is no mechanism`
`558`	`558`	`// (exposed through the libcontainerd interfaces) to enumerate or reference an`
`559`	`559`	`// exec'd process by ID, uniqueness is not currently enforced.`
`560`		`-func (t task) Exec(ctx context.Context, processID string, spec specs.Process, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (libcontainerdtypes.Process, error) {`
	`560`	`+func (t task) Exec(ctx context.Context, processID string, spec specs.Process, withStdin bool, attachStdio libcontainerdtypes.StdioCallback) (_ libcontainerdtypes.Process, retErr error) {`
`561`	`561`	`hcsContainer, err := t.getHCSContainer()`
`562`	`562`	`if err != nil {`
`563`	`563`	`return nil, err`
`@@ -610,7 +610,7 @@ func (t task) Exec(ctx context.Context, processID string, spec specs.Process,`
`610`	`610`	`}`
`611`	`611`	`pid := newProcess.Pid()`
`612`	`612`	`defer func() {`
`613`		`- if err != nil {`
	`613`	`+ if retErr != nil {`
`614`	`614`	`if err := newProcess.Kill(); err != nil {`
`615`	`615`	`logger.WithError(err).Error("failed to kill process")`
`616`	`616`	`}`