From 55b664046c446792191ef7b0be2a42e63a336be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 14 Sep 2023 11:06:46 +0200 Subject: [PATCH 1/3] libcontainer/windows: Fix process not being killed after stdio attach failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- libcontainerd/local/local_windows.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcontainerd/local/local_windows.go b/libcontainerd/local/local_windows.go index 6cb577f4ba..0e0bb4f293 100644 --- 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") } From b805599ef6e73d889d165fe95798214ae9846325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 14 Sep 2023 11:10:40 +0200 Subject: [PATCH 2/3] libcontainer/windows: Remove unneeded var declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup defer uses an `outErr` now, so we don't need to worry about shadowing. Signed-off-by: Paweł Gronowski --- libcontainerd/local/local_windows.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libcontainerd/local/local_windows.go b/libcontainerd/local/local_windows.go index 0e0bb4f293..0bcd93ecea 100644 --- a/libcontainerd/local/local_windows.go +++ b/libcontainerd/local/local_windows.go @@ -474,9 +474,7 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS // exit event is not sent out-of-order. defer func() { go t.reap() }() - // Don't shadow err here due to our deferred clean-up. - var dio *cio.DirectIO - dio, err = newIOFromProcess(newProcess, ctr.ociSpec.Process.Terminal) + dio, err := newIOFromProcess(newProcess, ctr.ociSpec.Process.Terminal) if err != nil { logger.WithError(err).Error("failed to get stdio pipes") return nil, err From 0937aef261ed401a77f8337b0b54d929a72b289d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 14 Sep 2023 11:07:02 +0200 Subject: [PATCH 3/3] libcontainerd/windows: Don't reap on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Synchronize the code to do the same thing as Exec. reap doesn't need to be called before the start event was sent. There's already a defer block which cleans up the process in case where an error occurs. Signed-off-by: Paweł Gronowski --- libcontainerd/local/local_windows.go | 30 +++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/libcontainerd/local/local_windows.go b/libcontainerd/local/local_windows.go index 0bcd93ecea..67053a4084 100644 --- a/libcontainerd/local/local_windows.go +++ b/libcontainerd/local/local_windows.go @@ -460,19 +460,9 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS }() } }() - t := &task{process: process{ - id: ctr.id, - ctr: ctr, - hcsProcess: newProcess, - waitCh: make(chan struct{}), - }} - pid := t.Pid() - logger.WithField("pid", pid).Debug("init process started") - // Spin up a goroutine to notify the backend and clean up resources when - // the task exits. Defer until after the start event is sent so that the - // exit event is not sent out-of-order. - defer func() { go t.reap() }() + pid := newProcess.Pid() + logger.WithField("pid", pid).Debug("init process started") dio, err := newIOFromProcess(newProcess, ctr.ociSpec.Process.Terminal) if err != nil { @@ -485,16 +475,28 @@ func (ctr *container) Start(_ context.Context, _ string, withStdin bool, attachS return nil, err } + t := &task{process{ + id: ctr.id, + ctr: ctr, + hcsProcess: newProcess, + waitCh: make(chan struct{}), + }} + // All fallible operations have succeeded so it is now safe to set the // container's current task. ctr.task = t + // Spin up a goroutine to notify the backend and clean up resources when + // the task exits. Defer until after the start event is sent so that the + // exit event is not sent out-of-order. + defer func() { go t.reap() }() + // Generate the associated event ctr.client.eventQ.Append(ctr.id, func() { ei := libcontainerdtypes.EventInfo{ ContainerID: ctr.id, ProcessID: t.id, - Pid: pid, + Pid: uint32(pid), } ctr.client.logger.WithFields(log.Fields{ "container": ctr.id, @@ -606,7 +608,6 @@ func (t *task) Exec(ctx context.Context, processID string, spec *specs.Process, logger.WithError(err).Errorf("exec's CreateProcess() failed") return nil, err } - pid := newProcess.Pid() defer func() { if retErr != nil { if err := newProcess.Kill(); err != nil { @@ -646,6 +647,7 @@ func (t *task) Exec(ctx context.Context, processID string, spec *specs.Process, // the exit event is not sent out-of-order. defer func() { go p.reap() }() + pid := newProcess.Pid() t.ctr.client.eventQ.Append(t.ctr.id, func() { ei := libcontainerdtypes.EventInfo{ ContainerID: t.ctr.id,