From 8dcd42b4146837e7075c2a0c2af5a993b35720a9 Mon Sep 17 00:00:00 2001
From: lifubang <lifubang@acmcoder.com>
Date: Sat, 20 Jul 2024 08:51:13 +0800
Subject: [PATCH] Ensure we can always terminate the parent process on error

As we all know, we should terminate the parent process if there is an
error when starting the container process, but these terminate function
are called in many places, for example: `initProcess`, `setnsProcess`,
and `Container`, if we forget this terminate call for some errors, it
will let the container in unknown state, so we should change to call it
in some final places.

Signed-off-by: lifubang <lifubang@acmcoder.com>
---
 libcontainer/container_linux.go | 53 +++++++++++++++++++++++++++------
 libcontainer/process.go         |  1 +
 libcontainer/process_linux.go   | 14 ---------
 3 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 6a91df01db0..613a0eb272e 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -201,18 +201,30 @@ func (c *Container) Set(config configs.Config) error {
 
 // Start starts a process inside the container. Returns error if process fails
 // to start. You can track process lifecycle with passed Process structure.
-func (c *Container) Start(process *Process) error {
+func (c *Container) Start(process *Process) (retErr error) {
 	c.m.Lock()
-	defer c.m.Unlock()
+	defer func() {
+		if retErr != nil {
+			c.terminate(process)
+		}
+		c.m.Unlock()
+	}()
 	return c.start(process)
 }
 
 // Run immediately starts the process inside the container. Returns an error if
 // the process fails to start. It does not block waiting for the exec fifo
 // after start returns but opens the fifo after start returns.
-func (c *Container) Run(process *Process) error {
+func (c *Container) Run(process *Process) (retErr error) {
 	c.m.Lock()
 	defer c.m.Unlock()
+
+	defer func() {
+		if retErr != nil {
+			c.terminate(process)
+		}
+	}()
+
 	if err := c.start(process); err != nil {
 		return err
 	}
@@ -229,6 +241,34 @@ func (c *Container) Exec() error {
 	return c.exec()
 }
 
+// terminate is to kill the container's init/exec process when got failure.
+func (c *Container) terminate(process *Process) {
+	if process.ops == nil {
+		return
+	}
+	if process.Init {
+		if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
+			logrus.WithError(err).Warn("unable to terminate initProcess")
+		}
+		// If we haven't saved container's state yet, we need to destroy the
+		// cgroup & intelRdt manager manually.
+		if _, err := os.Stat(filepath.Join(c.stateDir, stateFilename)); os.IsNotExist(err) {
+			if err := c.cgroupManager.Destroy(); err != nil {
+				logrus.WithError(err).Warn("unable to destroy cgroupManager")
+			}
+			if c.intelRdtManager != nil {
+				if err := c.intelRdtManager.Destroy(); err != nil {
+					logrus.WithError(err).Warn("unable to destroy intelRdtManager")
+				}
+			}
+		}
+		return
+	}
+	if err := ignoreTerminateErrors(process.ops.terminate()); err != nil {
+		logrus.WithError(err).Warn("unable to terminate setnsProcess")
+	}
+}
+
 func (c *Container) exec() error {
 	path := filepath.Join(c.stateDir, execFifoFilename)
 	pid := c.initProcess.pid()
@@ -359,12 +399,7 @@ func (c *Container) start(process *Process) (retErr error) {
 				return err
 			}
 
-			if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
-				if err := ignoreTerminateErrors(parent.terminate()); err != nil {
-					logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
-				}
-				return err
-			}
+			return c.config.Hooks.Run(configs.Poststart, s)
 		}
 	}
 	return nil
diff --git a/libcontainer/process.go b/libcontainer/process.go
index 3663c7e0dd2..9b25699b8f4 100644
--- a/libcontainer/process.go
+++ b/libcontainer/process.go
@@ -12,6 +12,7 @@ import (
 var errInvalidProcess = errors.New("invalid process")
 
 type processOperations interface {
+	terminate() error
 	wait() (*os.ProcessState, error)
 	signal(sig os.Signal) error
 	pid() int
diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 9a2473f716e..55d904f42c7 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -151,10 +151,6 @@ func (p *setnsProcess) start() (retErr error) {
 			if werr != nil {
 				logrus.WithError(werr).Warn()
 			}
-			err := ignoreTerminateErrors(p.terminate())
-			if err != nil {
-				logrus.WithError(err).Warn("unable to terminate setnsProcess")
-			}
 		}
 	}()
 
@@ -563,16 +559,6 @@ func (p *initProcess) start() (retErr error) {
 			if werr != nil {
 				logrus.WithError(werr).Warn()
 			}
-
-			// Terminate the process to ensure we can remove cgroups.
-			if err := ignoreTerminateErrors(p.terminate()); err != nil {
-				logrus.WithError(err).Warn("unable to terminate initProcess")
-			}
-
-			_ = p.manager.Destroy()
-			if p.intelRdtManager != nil {
-				_ = p.intelRdtManager.Destroy()
-			}
 		}
 	}()