From 8ff0b71be918766ec848962c6c5e22cbcb99a0cf 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 | 54 +++++++++++++++++++++++++++------
 libcontainer/process.go         |  1 +
 libcontainer/process_linux.go   | 14 ---------
 3 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index c02116177ad..26b2c8d2c36 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -201,20 +201,31 @@ func (c *Container) Set(config configs.Config) error {
 func (c *Container) Start(process *Process) error {
 	c.m.Lock()
 	defer c.m.Unlock()
-	return c.start(process)
+
+	if err := c.start(process); err != nil {
+		c.terminate(process)
+		return err
+	}
+	return nil
 }
 
 // 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()
+
 	if err := c.start(process); err != nil {
+		c.terminate(process)
 		return err
 	}
-	if process.Init {
-		return c.exec()
+	if !process.Init {
+		return nil
+	}
+	if err := c.exec(); err != nil {
+		c.terminate(process)
+		return err
 	}
 	return nil
 }
@@ -226,6 +237,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()
@@ -356,12 +395,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 114b3f2b6cb..a06e1693984 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 fcbb54a3e41..2a623c4bf43 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -146,10 +146,6 @@ func (p *setnsProcess) start() (retErr error) {
 				// Someone in this cgroup was killed, this _might_ be us.
 				retErr = fmt.Errorf("%w (possibly OOM-killed)", retErr)
 			}
-			err := ignoreTerminateErrors(p.terminate())
-			if err != nil {
-				logrus.WithError(err).Warn("unable to terminate setnsProcess")
-			}
 		}
 	}()
 
@@ -548,16 +544,6 @@ func (p *initProcess) start() (retErr error) {
 					retErr = errors.New(oomError)
 				}
 			}
-
-			// 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()
-			}
 		}
 	}()