From c6e7b1a8ec55d0d3f3d6e72c7ea1d8bd6bca0f04 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Tue, 15 Aug 2023 18:22:38 -0700
Subject: [PATCH] libct: initProcess.start: fix sync logic

The code in this function became quite complicated and not entirely
correct over time. As a result, if an error is returned from parseSync,
it might end up stuck waiting for the child to finish.

1. Let's not wait() for the child twice. We already do it in the
defer statement (call p.terminate()) when we are returning an error.

2. Remove sentResume and sentRun since we do not want to check if
these were sent or not. Instead, introduce and check seenProcReady, as
procReady is always expected from runc init.

3. Eliminate the possibility to wrap nil as an error.

4. Make sure we always call shutdown on the sync socket, and do not let
   shutdown error shadow the ierr.

This fixes the issue of stuck `runc runc` with the optimization patch
(sending procSeccompDone earlier) applied.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/process_linux.go | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 48861406d..41709a4ea 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -451,11 +451,8 @@ func (p *initProcess) start() (retErr error) {
 	if err := p.sendConfig(); err != nil {
 		return fmt.Errorf("error sending config to init process: %w", err)
 	}
-	var (
-		sentRun    bool
-		sentResume bool
-	)
 
+	var seenProcReady bool
 	ierr := parseSync(p.messageSockPair.parent, func(sync *syncT) error {
 		switch sync.Type {
 		case procSeccomp:
@@ -494,6 +491,7 @@ func (p *initProcess) start() (retErr error) {
 				return err
 			}
 		case procReady:
+			seenProcReady = true
 			// set rlimits, this has to be done here because we lose permissions
 			// to raise the limits once we enter a user-namespace
 			if err := setupRlimits(p.config.Rlimits, p.pid()); err != nil {
@@ -555,7 +553,6 @@ func (p *initProcess) start() (retErr error) {
 			if err := writeSync(p.messageSockPair.parent, procRun); err != nil {
 				return err
 			}
-			sentRun = true
 		case procHooks:
 			// Setup cgroup before prestart hook, so that the prestart hook could apply cgroup permissions.
 			if err := p.manager.Set(p.config.Config.Cgroups.Resources); err != nil {
@@ -587,7 +584,6 @@ func (p *initProcess) start() (retErr error) {
 			if err := writeSync(p.messageSockPair.parent, procResume); err != nil {
 				return err
 			}
-			sentResume = true
 		default:
 			return errors.New("invalid JSON payload from child")
 		}
@@ -595,20 +591,14 @@ func (p *initProcess) start() (retErr error) {
 		return nil
 	})
 
-	if !sentRun {
-		return fmt.Errorf("error during container init: %w", ierr)
-	}
-	if p.config.Config.Namespaces.Contains(configs.NEWNS) && !sentResume {
-		return errors.New("could not synchronise after executing prestart and CreateRuntime hooks with container process")
-	}
-	if err := unix.Shutdown(int(p.messageSockPair.parent.Fd()), unix.SHUT_WR); err != nil {
+	if err := unix.Shutdown(int(p.messageSockPair.parent.Fd()), unix.SHUT_WR); err != nil && ierr == nil {
 		return &os.PathError{Op: "shutdown", Path: "(init pipe)", Err: err}
 	}
-
-	// Must be done after Shutdown so the child will exit and we can wait for it.
+	if !seenProcReady && ierr == nil {
+		ierr = errors.New("procReady not received")
+	}
 	if ierr != nil {
-		_, _ = p.wait()
-		return ierr
+		return fmt.Errorf("error during container init: %w", ierr)
 	}
 	return nil
 }