Merge pull request #3854 from kolyshkin/refff

Some init code refactoring

Author: mrunalp

libcontainer/factory_linux.go

@@ -5,8 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"runtime/debug"
-	"strconv"
 
 	securejoin "github.com/cyphar/filepath-securejoin"
 	"golang.org/x/sys/unix"
@@ -18,7 +16,6 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs/validate"
 	"github.com/opencontainers/runc/libcontainer/intelrdt"
 	"github.com/opencontainers/runc/libcontainer/utils"
-	"github.com/sirupsen/logrus"
 )
 
 const (
@@ -151,90 +148,6 @@ func Load(root, id string) (*Container, error) {
 	return c, nil
 }
 
-// StartInitialization loads a container by opening the pipe fd from the parent
-// to read the configuration and state. This is a low level implementation
-// detail of the reexec and should not be consumed externally.
-func StartInitialization() (err error) {
-	// Get the INITPIPE.
-	envInitPipe := os.Getenv("_LIBCONTAINER_INITPIPE")
-	pipefd, err := strconv.Atoi(envInitPipe)
-	if err != nil {
-		err = fmt.Errorf("unable to convert _LIBCONTAINER_INITPIPE: %w", err)
-		logrus.Error(err)
-		return err
-	}
-	pipe := os.NewFile(uintptr(pipefd), "pipe")
-	defer pipe.Close()
-
-	defer func() {
-		// We have an error during the initialization of the container's init,
-		// send it back to the parent process in the form of an initError.
-		if werr := writeSync(pipe, procError); werr != nil {
-			fmt.Fprintln(os.Stderr, err)
-			return
-		}
-		if werr := utils.WriteJSON(pipe, &initError{Message: err.Error()}); werr != nil {
-			fmt.Fprintln(os.Stderr, err)
-			return
-		}
-	}()
-
-	// Only init processes have FIFOFD.
-	fifofd := -1
-	envInitType := os.Getenv("_LIBCONTAINER_INITTYPE")
-	it := initType(envInitType)
-	if it == initStandard {
-		envFifoFd := os.Getenv("_LIBCONTAINER_FIFOFD")
-		if fifofd, err = strconv.Atoi(envFifoFd); err != nil {
-			return fmt.Errorf("unable to convert _LIBCONTAINER_FIFOFD: %w", err)
-		}
-	}
-
-	var consoleSocket *os.File
-	if envConsole := os.Getenv("_LIBCONTAINER_CONSOLE"); envConsole != "" {
-		console, err := strconv.Atoi(envConsole)
-		if err != nil {
-			return fmt.Errorf("unable to convert _LIBCONTAINER_CONSOLE: %w", err)
-		}
-		consoleSocket = os.NewFile(uintptr(console), "console-socket")
-		defer consoleSocket.Close()
-	}
-
-	logPipeFdStr := os.Getenv("_LIBCONTAINER_LOGPIPE")
-	logPipeFd, err := strconv.Atoi(logPipeFdStr)
-	if err != nil {
-		return fmt.Errorf("unable to convert _LIBCONTAINER_LOGPIPE: %w", err)
-	}
-
-	// Get mount files (O_PATH).
-	mountFds, err := parseMountFds()
-	if err != nil {
-		return err
-	}
-
-	// clear the current process's environment to clean any libcontainer
-	// specific env vars.
-	os.Clearenv()
-
-	defer func() {
-		if e := recover(); e != nil {
-			if ee, ok := e.(error); ok {
-				err = fmt.Errorf("panic from initialization: %w, %s", ee, debug.Stack())
-			} else {
-				err = fmt.Errorf("panic from initialization: %v, %s", e, debug.Stack())
-			}
-		}
-	}()
-
-	i, err := newContainerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds)
-	if err != nil {
-		return err
-	}
-
-	// If Init succeeds, syscall.Exec will not return, hence none of the defers will be called.
-	return i.Init()
-}
-
 func loadState(root string) (*State, error) {
 	stateFilePath, err := securejoin.SecureJoin(root, stateFilename)
 	if err != nil {

libcontainer/init_linux.go

@@ -8,6 +8,8 @@ import (
 	"io"
 	"net"
 	"os"
+	"runtime/debug"
+	"strconv"
 	"strings"
 	"unsafe"
 
@@ -71,43 +73,120 @@ type initConfig struct {
 	Cgroup2Path      string                `json:"cgroup2_path,omitempty"`
 }
 
-type initer interface {
-	Init() error
+// StartInitialization loads a container by opening the pipe fd from the parent
+// to read the configuration and state. This is a low level implementation
+// detail of the reexec and should not be consumed externally.
+func StartInitialization() (retErr error) {
+	// Get the INITPIPE.
+	envInitPipe := os.Getenv("_LIBCONTAINER_INITPIPE")
+	pipefd, err := strconv.Atoi(envInitPipe)
+	if err != nil {
+		err = fmt.Errorf("unable to convert _LIBCONTAINER_INITPIPE: %w", err)
+		logrus.Error(err)
+		return err
+	}
+	pipe := os.NewFile(uintptr(pipefd), "pipe")
+	defer pipe.Close()
+
+	defer func() {
+		// We have an error during the initialization of the container's init,
+		// send it back to the parent process in the form of an initError.
+		if err := writeSync(pipe, procError); err != nil {
+			fmt.Fprintln(os.Stderr, retErr)
+			return
+		}
+		if err := utils.WriteJSON(pipe, &initError{Message: retErr.Error()}); err != nil {
+			fmt.Fprintln(os.Stderr, retErr)
+			return
+		}
+	}()
+
+	// Only init processes have FIFOFD.
+	fifofd := -1
+	envInitType := os.Getenv("_LIBCONTAINER_INITTYPE")
+	it := initType(envInitType)
+	if it == initStandard {
+		envFifoFd := os.Getenv("_LIBCONTAINER_FIFOFD")
+		if fifofd, err = strconv.Atoi(envFifoFd); err != nil {
+			return fmt.Errorf("unable to convert _LIBCONTAINER_FIFOFD: %w", err)
+		}
+	}
+
+	var consoleSocket *os.File
+	if envConsole := os.Getenv("_LIBCONTAINER_CONSOLE"); envConsole != "" {
+		console, err := strconv.Atoi(envConsole)
+		if err != nil {
+			return fmt.Errorf("unable to convert _LIBCONTAINER_CONSOLE: %w", err)
+		}
+		consoleSocket = os.NewFile(uintptr(console), "console-socket")
+		defer consoleSocket.Close()
+	}
+
+	logPipeFdStr := os.Getenv("_LIBCONTAINER_LOGPIPE")
+	logPipeFd, err := strconv.Atoi(logPipeFdStr)
+	if err != nil {
+		return fmt.Errorf("unable to convert _LIBCONTAINER_LOGPIPE: %w", err)
+	}
+
+	// Get mount files (O_PATH).
+	mountFds, err := parseMountFds()
+	if err != nil {
+		return err
+	}
+
+	// clear the current process's environment to clean any libcontainer
+	// specific env vars.
+	os.Clearenv()
+
+	defer func() {
+		if err := recover(); err != nil {
+			if err2, ok := err.(error); ok {
+				retErr = fmt.Errorf("panic from initialization: %w, %s", err2, debug.Stack())
+			} else {
+				retErr = fmt.Errorf("panic from initialization: %v, %s", err, debug.Stack())
+			}
+		}
+	}()
+
+	// If init succeeds, it will not return, hence none of the defers will be called.
+	return containerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds)
 }
 
-func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, fifoFd, logFd int, mountFds []int) (initer, error) {
+func containerInit(t initType, pipe *os.File, consoleSocket *os.File, fifoFd, logFd int, mountFds []int) error {
 	var config *initConfig
 	if err := json.NewDecoder(pipe).Decode(&config); err != nil {
-		return nil, err
+		return err
 	}
 	if err := populateProcessEnvironment(config.Env); err != nil {
-		return nil, err
+		return err
 	}
 	switch t {
 	case initSetns:
 		// mountFds must be nil in this case. We don't mount while doing runc exec.
 		if mountFds != nil {
-			return nil, errors.New("mountFds must be nil; can't mount from exec")
+			return errors.New("mountFds must be nil; can't mount from exec")
 		}
 
-		return &linuxSetnsInit{
+		i := &linuxSetnsInit{
 			pipe:          pipe,
 			consoleSocket: consoleSocket,
 			config:        config,
 			logFd:         logFd,
-		}, nil
+		}
+		return i.Init()
 	case initStandard:
-		return &linuxStandardInit{
+		i := &linuxStandardInit{
 			pipe:          pipe,
 			consoleSocket: consoleSocket,
 			parentPid:     unix.Getppid(),
 			config:        config,
 			fifoFd:        fifoFd,
 			logFd:         logFd,
 			mountFds:      mountFds,
-		}, nil
+		}
+		return i.Init()
 	}
-	return nil, fmt.Errorf("unknown init type %q", t)
+	return fmt.Errorf("unknown init type %q", t)
 }
 
 // populateProcessEnvironment loads the provided environment variables into the

libcontainer/integration/execin_test.go

@@ -215,12 +215,10 @@ func TestExecInError(t *testing.T) {
 	ok(t, err)
 
 	for i := 0; i < 42; i++ {
-		var out bytes.Buffer
 		unexistent := &libcontainer.Process{
-			Cwd:    "/",
-			Args:   []string{"unexistent"},
-			Env:    standardEnvironment,
-			Stderr: &out,
+			Cwd:  "/",
+			Args: []string{"unexistent"},
+			Env:  standardEnvironment,
 		}
 		err = container.Run(unexistent)
 		if err == nil {
@@ -229,9 +227,6 @@ func TestExecInError(t *testing.T) {
 		if !strings.Contains(err.Error(), "executable file not found") {
 			t.Fatalf("Should be error about not found executable, got %s", err)
 		}
-		if !bytes.Contains(out.Bytes(), []byte("executable file not found")) {
-			t.Fatalf("executable file not found error not delivered to stdio:\n%s", out.String())
-		}
 	}
 }
 

libcontainer/integration/init_test.go

@@ -1,6 +1,7 @@
 package integration
 
 import (
+	"fmt"
 	"os"
 	"runtime"
 	"testing"
@@ -9,27 +10,27 @@ import (
 	//nolint:revive // Enable cgroup manager to manage devices
 	_ "github.com/opencontainers/runc/libcontainer/cgroups/devices"
 	_ "github.com/opencontainers/runc/libcontainer/nsenter"
-
-	"github.com/sirupsen/logrus"
 )
 
-// init runs the libcontainer initialization code because of the busybox style needs
-// to work around the go runtime and the issues with forking
+// Same as ../../init.go but for libcontainer/integration.
 func init() {
 	if len(os.Args) < 2 || os.Args[1] != "init" {
 		return
 	}
+	// This is the golang entry point for runc init, executed
+	// before TestMain() but after libcontainer/nsenter's nsexec().
 	runtime.GOMAXPROCS(1)
 	runtime.LockOSThread()
 	if err := libcontainer.StartInitialization(); err != nil {
-		logrus.Fatal(err)
+		// logrus is not initialized
+		fmt.Fprintln(os.Stderr, err)
 	}
+	// Normally, StartInitialization() never returns, meaning
+	// if we are here, it had failed.
+	os.Exit(1)
 }
 
 func TestMain(m *testing.M) {
-	logrus.SetOutput(os.Stderr)
-	logrus.SetLevel(logrus.InfoLevel)
-
 	ret := m.Run()
 	os.Exit(ret)
 }