fix(ipc): Handle unsuccessful setup for monitor environment

In case there was an error during the monitor execution environment
setup (e.g. vmm not found, rootfs issue) then the urunc start command
was keep waiting for a message from reexec that everything was ok. This
was leading to the urunc start process hanging there forever and of
course the container was staying like that.

THis commit fixes this issue and moreover it slightly refactors the
whole IPC between the urunc instances and reexec.

PR: #357
Signed-off-by: Charalampos Mainas <cmainas@nubificus.co.uk>
Reviewed-by: Anastassios Nanos <ananos@nubificus.co.uk>
Approved-by: Anastassios Nanos <ananos@nubificus.co.uk>

Author: cmainas

cmd/urunc/create.go

@@ -23,7 +23,6 @@ import (
 	"net"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strconv"
 
 	"github.com/creack/pty"
@@ -268,16 +267,14 @@ func createUnikontainer(cmd *cli.Command, uruncCfg *unikontainers.UruncConfig) (
 	}
 	metrics.Capture(m.TS07)
 
-	// send ACK to reexec process
-	err = unikontainer.SendAckReexec()
+	_, err = initSockParent.Write([]byte(unikontainers.AckReexec))
 	if err != nil {
 		err = fmt.Errorf("failed to send ACK to reexec process: %w", err)
 		return err
-
 	}
+
 	metrics.Capture(m.TS08)
 
-	err = nil
 	return err
 }
 
@@ -367,22 +364,21 @@ func reexecUnikontainer(cmd *cli.Command) error {
 		return fmt.Errorf("unable to convert _LIBCONTAINER_INITPIPE: %w", err)
 	}
 	initPipe := os.NewFile(uintptr(initFd), "initpipe")
-	err = initPipe.Close()
-	if err != nil {
-		return fmt.Errorf("close init pipe: %w", err)
-	}
-
-	// We have already made sure in main.go that root is not nil
-	rootDir := cmd.String("root")
-	baseDir := filepath.Join(rootDir, containerID)
-
 	metrics.Capture(m.TS05)
 
-	// wait AckReexec message on urunc.sock from parent process
-	socketPath := unikontainers.GetUruncSockAddr(baseDir)
-	err = unikontainers.ListenAndAwaitMsg(socketPath, unikontainers.AckReexec)
+	// wait AckReexec message on init socket from parent process
+	buf := make([]byte, len(unikontainers.AckReexec))
+	n, err := initPipe.Read(buf)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to read from init socket: %w", err)
+	}
+	msg := string(buf[0:n])
+	if msg != string(unikontainers.AckReexec) {
+		return fmt.Errorf("received unexpected message: %s", msg)
+	}
+	err = initPipe.Close()
+	if err != nil {
+		return fmt.Errorf("close init pipe: %w", err)
 	}
 	metrics.Capture(m.TS09)
 
@@ -406,14 +402,63 @@ func reexecUnikontainer(cmd *cli.Command) error {
 	metrics.Capture(m.TS10)
 
 	// wait StartExecve message on urunc.sock from urunc start process
-	err = unikontainers.ListenAndAwaitMsg(socketPath, unikontainers.StartExecve)
+	err = unikontainer.CreateListener(unikontainers.FromReexec)
 	if err != nil {
+		return fmt.Errorf("error creating listener on reexec socket: %w", err)
+	}
+	awaitErr := unikontainer.AwaitMsg(unikontainers.StartExecve)
+	// Before checking for any errors, make sure to clean up the socket
+	cleanErr := unikontainer.DestroyListener(unikontainers.FromReexec)
+	if cleanErr != nil {
+		logrus.WithError(cleanErr).Error("failed to destroy listener on reexec socket")
+		cleanErr = fmt.Errorf("error destroying listener on reexec socket: %w", cleanErr)
+	}
+	// NOTE: Currently, we do not fail in case the socket cleanup fails
+	// because urunc start has told reexec to start preparing the execution
+	// environment for the monitor. Therefore, if the socket cleanup affects the
+	// preparation the error will get manifested later. However, if environment
+	// setup goes well and the socket was not cleaned up correctly,
+	// we execve the monitor and we rely on Go's close-on-exec feature in all file
+	// descriptors. THerefore, we might want to rethink this in future and not rely
+	// on Go, but this requires quite a a lot of changes.
+	if awaitErr != nil {
+		awaitErr = fmt.Errorf("error waiting START message: %w", awaitErr)
+		err = errors.Join(awaitErr, cleanErr)
 		return err
 	}
 	metrics.Capture(m.TS14)
 
+	err = unikontainer.CreateConn(unikontainers.FromReexec)
+	if err != nil {
+		return fmt.Errorf("error creating connection to urunc socket: %w", err)
+	}
+	// NOTE: More changes are required for a proper handling of this cleanup.
+	// Currently, the cleanup will happen if something fails in the container
+	// setup and unikontainers.Exec returns with an error. In that case, we also
+	// ignore any errors, since it is a simple socket cleanup and the process exits.
+	// If unikontainers.Exec succeeds though, then we will never execute this
+	// cleanup, since we execve to the monitor process. In that case, we rely
+	// once more in Go's close on exec handling of all file descriptors.
+	// In the future, we might want to revisit this and rely less in Go.
+	defer func() {
+		tmpErr := unikontainer.DestroyConn(unikontainers.FromReexec)
+		if tmpErr != nil {
+			logrus.WithError(tmpErr).Error("failed to destroy connection on reexec socket")
+		}
+	}()
+
 	// execve
 	// we need to pass metrics to Exec() function, as the unikontainer
 	// struct does not have the part of urunc config that configures metrics
-	return unikontainer.Exec(metrics)
+	var sockErr error
+	err = unikontainer.Exec(metrics)
+	if err != nil {
+		logrus.WithError(err).Error("Setting up execution environment for monitor")
+		sockErr = unikontainer.SendMessage(unikontainers.StartErr)
+		if sockErr != nil {
+			logrus.WithError(sockErr).Error("failed to send error message to urunc socket")
+		}
+	}
+
+	return errors.Join(err, sockErr)
 }

cmd/urunc/start.go

@@ -16,6 +16,8 @@ package main
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"os"
 
 	"github.com/sirupsen/logrus"
@@ -56,27 +58,56 @@ func startUnikontainer(cmd *cli.Command) error {
 	}
 	metrics.Capture(m.TS12)
 
-	sockAddr := unikontainers.GetStartSockAddr(unikontainer.BaseDir)
-	listener, cleaner, err := unikontainers.CreateListener(sockAddr, true)
+	err = unikontainer.CreateListener(!unikontainers.FromReexec)
 	if err != nil {
 		return err
 	}
-	defer cleaner()
+	// NOTE: We ignore any errors from the DestroyListener here, because
+	// the reexec process has already started the monitor execution and hence
+	// returning an error would confuse the shim. Furthermore, this process
+	// exits. However, we might want to revisit this in the future and
+	// handle it better.
+	defer func() {
+		tmpErr := unikontainer.DestroyListener(!unikontainers.FromReexec)
+		if tmpErr != nil {
+			logrus.WithError(tmpErr).Error("failed to destroy listener on reexec socket")
+		}
+	}()
 
-	err = unikontainer.SendStartExecve()
+	// Send message to reexec to start the monitor
+	err = unikontainer.CreateConn(!unikontainers.FromReexec)
+	if err != nil {
+		err = fmt.Errorf("failed to create connection with reexec socket: %w", err)
+		return err
+	}
+	sendErr := unikontainer.SendMessage(unikontainers.StartExecve)
+	if sendErr != nil {
+		logrus.WithError(sendErr).Error("failed to send START message to reexec")
+		sendErr = fmt.Errorf("error sending START message: %w", sendErr)
+	}
+	// Regardless of the SendMessage status, make sure to clean up the socket,
+	// since it is not required anymore
+	cleanErr := unikontainer.DestroyConn(!unikontainers.FromReexec)
+	if cleanErr != nil {
+		logrus.WithError(cleanErr).Error("failed to destroy connection to reexec socket")
+		cleanErr = fmt.Errorf("error destroying connection to reexec socket: %w", cleanErr)
+	}
+	err = errors.Join(sendErr, cleanErr)
 	if err != nil {
 		return err
 	}
 	metrics.Capture(m.TS13)
 
 	// wait ContainerStarted message on start.sock from reexec process
-	err = unikontainers.AwaitMessage(listener, unikontainers.ContainerStarted)
+	err = unikontainer.AwaitMsg(unikontainers.StartSuccess)
 	if err != nil {
+		err = fmt.Errorf("failed to get message from successful start from reexec: %w", err)
 		return err
 	}
 
 	err = unikontainer.SetRunningState()
 	if err != nil {
+		err = fmt.Errorf("failed to set the state as running for container: %w", err)
 		return err
 	}
 

pkg/unikontainers/ipc.go

@@ -21,7 +21,6 @@ import (
 	"net"
 	"os"
 	"path/filepath"
-	"syscall"
 	"time"
 
 	"github.com/sirupsen/logrus"
@@ -30,14 +29,18 @@ import (
 type IPCMessage string
 
 const (
-	startSock                   = "start.sock"
-	uruncSock                   = "urunc.sock"
-	ReexecStarted    IPCMessage = "BOOTED"
-	AckReexec        IPCMessage = "ACK"
-	StartExecve      IPCMessage = "START"
-	ContainerStarted IPCMessage = "CNTR_STARTED"
-	maxRetries                  = 50
-	waitTime                    = 5 * time.Millisecond
+	// Socket for messages towards reexec. The reexec process listens in this socket
+	reexecSock = "reexec.sock"
+	// Socket for messages from reexec. The reexec process writes in this socket
+	uruncSock                = "urunc.sock"
+	ReexecStarted IPCMessage = "RX_START"
+	AckReexec     IPCMessage = "UC_ACK"
+	StartExecve   IPCMessage = "UC_START"
+	StartSuccess  IPCMessage = "RX_SUCCESS"
+	StartErr      IPCMessage = "RX_ERROR"
+	maxRetries               = 50
+	waitTime                 = 5 * time.Millisecond
+	FromReexec               = true
 )
 
 func getSockAddr(dir string, name string) string {
@@ -48,8 +51,8 @@ func getUruncSockAddr(containerDir string) string {
 	return getSockAddr(containerDir, uruncSock)
 }
 
-func getStartSockAddr(baseDir string) string {
-	return getSockAddr(baseDir, startSock)
+func getReexecSockAddr(baseDir string) string {
+	return getSockAddr(baseDir, reexecSock)
 }
 
 func ensureValidSockAddr(sockAddr string) error {
@@ -125,32 +128,21 @@ func sendIPCMessageWithRetry(socketAddress string, message IPCMessage, mustBeVal
 	return err
 }
 
-// CreateListener sets up a listener for new connection to socketAddress
-func CreateListener(socketAddress string, mustBeValid bool) (*net.UnixListener, func(), error) {
+// createListener sets up a listener for new connection to socketAddress
+func createListener(socketAddress string, mustBeValid bool) (*net.UnixListener, error) {
 	if mustBeValid {
 		err := ensureValidSockAddr(socketAddress)
 		if err != nil {
-			return nil, nil, err
+			return nil, err
 		}
 	}
 
 	listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: socketAddress, Net: "unix"})
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
-	cleanup := func() {
-		derr := listener.Close()
-		if derr != nil {
-			logrus.WithError(derr).Error("failed to close listener")
-		}
-		derr = syscall.Unlink(socketAddress)
-		if derr != nil {
-			logrus.WithError(derr).Errorf("failed to unlink %s", socketAddress)
-		}
-	}
-
-	return listener, cleanup, nil
+	return listener, nil
 }
 
 // awaitMessage opens a new connection to socketAddress

pkg/unikontainers/ipc_test.go

@@ -141,11 +141,11 @@ func TestSendIPCMessageWithRetry(t *testing.T) {
 func TestCreateListener(t *testing.T) {
 	socketAddress := "/tmp/test_create_listener.sock"
 
-	listener, cleaner, err := CreateListener(socketAddress, true)
+	listener, err := createListener(socketAddress, true)
 	if err != nil {
 		t.Fatalf("Failed to create listener: %v", err)
 	}
-	defer cleaner()
+	defer listener.Close()
 
 	assert.NotNil(t, listener, "Expected listener to be created")
 }
@@ -154,11 +154,11 @@ func TestAwaitMessage(t *testing.T) {
 	socketAddress := "/tmp/test_await_message.sock"
 	expectedMessage := ReexecStarted
 
-	listener, cleaner, err := CreateListener(socketAddress, true)
+	listener, err := createListener(socketAddress, true)
 	if err != nil {
 		t.Fatalf("Failed to create listener: %v", err)
 	}
-	defer cleaner()
+	defer listener.Close()
 
 	go func() {
 		conn, err := net.Dial("unix", socketAddress)

pkg/unikontainers/rootfs.go

@@ -172,7 +172,7 @@ func (rs *rootfsSelector) tryContainerRootfs() (types.RootfsParams, bool) {
 		return result, true
 	}
 
-	uniklog.Error("can not use the container rootfs as block, or through shared-fs")
+	uniklog.Error("can not use the container rootfs as the sandbox's guest rootfs through block or shared-fs")
 	return types.RootfsParams{}, false
 }
 
@@ -225,7 +225,7 @@ func chooseRootfs(bundle string, cntrRootfs string, annot map[string]string,
 	}
 
 	if selector.shouldMountContainerRootfs() {
-		return types.RootfsParams{}, fmt.Errorf("can not mount container's rootfs as block or through shared-fs to guest")
+		return types.RootfsParams{}, fmt.Errorf("can not use the container rootfs as the sandbox's guest rootfs through block or shared-fs")
 	}
 
 	uniklog.Info("no rootfs configured for guest")