From 7396ca90fa47d0458da4188061b24ca1bff465bf Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Mon, 6 Nov 2023 15:38:11 -0800
Subject: [PATCH] runc delete: do not ignore error from destroy

If container.Destroy() has failed, runc destroy still return 0, which is
wrong and can result in other issues down the line.

Let's always return error from destroy in runc delete.

For runc checkpoint and runc run, we still treat it as a warning.

Co-authored-by: Zhang Tianyang <burning9699@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 checkpoint.go                   |  4 +++-
 delete.go                       |  7 ++-----
 libcontainer/container_linux.go |  5 ++++-
 utils_linux.go                  | 10 +++-------
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/checkpoint.go b/checkpoint.go
index a8a27f248..1f5f5e739 100644
--- a/checkpoint.go
+++ b/checkpoint.go
@@ -70,7 +70,9 @@ checkpointed.`,
 		err = container.Checkpoint(options)
 		if err == nil && !(options.LeaveRunning || options.PreDump) {
 			// Destroy the container unless we tell CRIU to keep it.
-			destroy(container)
+			if err := container.Destroy(); err != nil {
+				logrus.Warn(err)
+			}
 		}
 		return err
 	},
diff --git a/delete.go b/delete.go
index e8b92ae5a..fc8133438 100644
--- a/delete.go
+++ b/delete.go
@@ -18,8 +18,7 @@ func killContainer(container *libcontainer.Container) error {
 	for i := 0; i < 100; i++ {
 		time.Sleep(100 * time.Millisecond)
 		if err := container.Signal(unix.Signal(0)); err != nil {
-			destroy(container)
-			return nil
+			return container.Destroy()
 		}
 	}
 	return errors.New("container init still running")
@@ -80,13 +79,11 @@ status of "ubuntu01" as "stopped" the following will delete resources held for
 		}
 		switch s {
 		case libcontainer.Stopped:
-			destroy(container)
+			return container.Destroy()
 		case libcontainer.Created:
 			return killContainer(container)
 		default:
 			return fmt.Errorf("cannot delete container %s that is not stopped: %s", id, s)
 		}
-
-		return nil
 	},
 }
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 515755276..f36abaf10 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -874,7 +874,10 @@ func (c *Container) newInitConfig(process *Process) *initConfig {
 func (c *Container) Destroy() error {
 	c.m.Lock()
 	defer c.m.Unlock()
-	return c.state.destroy()
+	if err := c.state.destroy(); err != nil {
+		return fmt.Errorf("unable to destroy container: %w", err)
+	}
+	return nil
 }
 
 // Pause pauses the container, if its state is RUNNING or CREATED, changing
diff --git a/utils_linux.go b/utils_linux.go
index 633d6d68c..5de006284 100644
--- a/utils_linux.go
+++ b/utils_linux.go
@@ -88,12 +88,6 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) {
 	return lp, nil
 }
 
-func destroy(container *libcontainer.Container) {
-	if err := container.Destroy(); err != nil {
-		logrus.Error(err)
-	}
-}
-
 // setupIO modifies the given process config according to the options.
 func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, detach bool, sockpath string) (*tty, error) {
 	if createTTY {
@@ -303,7 +297,9 @@ func (r *runner) run(config *specs.Process) (int, error) {
 
 func (r *runner) destroy() {
 	if r.shouldDestroy {
-		destroy(r.container)
+		if err := r.container.Destroy(); err != nil {
+			logrus.Warn(err)
+		}
 	}
 }