From 72657eac2e6af14ec00512d40872168bb11ac4a5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 11 Apr 2023 16:38:50 -0700 Subject: [PATCH 1/4] libct: move StartInitialization No code change, just moving a function from factory_linux.go to init_linux.go. Signed-off-by: Kir Kolyshkin --- libcontainer/factory_linux.go | 87 ----------------------------------- libcontainer/init_linux.go | 86 ++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 87 deletions(-) diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index bf8904efb..d10a27810 100644 --- a/libcontainer/factory_linux.go +++ b/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 { diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 6a88f1b21..763ff9a15 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -8,6 +8,8 @@ import ( "io" "net" "os" + "runtime/debug" + "strconv" "strings" "unsafe" @@ -71,6 +73,90 @@ type initConfig struct { Cgroup2Path string `json:"cgroup2_path,omitempty"` } +// 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() +} + type initer interface { Init() error } From 4f0a7e78c3881375159ce649e45a7f2074aa94be Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 Dec 2021 14:22:34 -0800 Subject: [PATCH 2/4] libct/init: call Init from containerInit Instead of having newContainerInit return an interface, and let its caller call Init(), it is easier to call Init directly. Do that, and rename newContainerInit to containerInit. I think it makes the code more readable and straightforward. Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 763ff9a15..2a49d40ba 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -148,42 +148,34 @@ func StartInitialization() (err error) { } }() - 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() + // If init succeeds, it will not return, hence none of the defers will be called. + return containerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds) } -type initer interface { - Init() error -} - -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(), @@ -191,9 +183,10 @@ func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, fifoFd, 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 From eba31a7c6c70ab98bb05266f201e64069b91921b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 4 Aug 2022 16:33:57 -0700 Subject: [PATCH 3/4] libct/StartInitialization: rename returned error This is a cosmetic change to improve code readability, making it easier to distinguish between a local error and the error being returned. While at it, rename e to err (it was originally called e to not clash with returned error named err) and ee to err2. Signed-off-by: Kir Kolyshkin --- libcontainer/init_linux.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 2a49d40ba..52cb7d573 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -76,7 +76,7 @@ type initConfig struct { // 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) { +func StartInitialization() (retErr error) { // Get the INITPIPE. envInitPipe := os.Getenv("_LIBCONTAINER_INITPIPE") pipefd, err := strconv.Atoi(envInitPipe) @@ -91,12 +91,12 @@ func StartInitialization() (err error) { 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) + if err := writeSync(pipe, procError); err != nil { + fmt.Fprintln(os.Stderr, retErr) return } - if werr := utils.WriteJSON(pipe, &initError{Message: err.Error()}); werr != nil { - fmt.Fprintln(os.Stderr, err) + if err := utils.WriteJSON(pipe, &initError{Message: retErr.Error()}); err != nil { + fmt.Fprintln(os.Stderr, retErr) return } }() @@ -139,11 +139,11 @@ func StartInitialization() (err error) { 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()) + if err := recover(); err != nil { + if err2, ok := err.(error); ok { + retErr = fmt.Errorf("panic from initialization: %w, %s", err2, debug.Stack()) } else { - err = fmt.Errorf("panic from initialization: %v, %s", e, debug.Stack()) + retErr = fmt.Errorf("panic from initialization: %v, %s", err, debug.Stack()) } } }() From 7e481ee2eb069fba08bcffd04c3f5f723ae96527 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 Dec 2021 12:58:35 -0800 Subject: [PATCH 4/4] libct/int: remove logger from init Currently, TestInit sets up logrus, and init uses it to log an error from StartInitialization(). This is solely used by TestExecInError to check that error returned from StartInitialization is the one it expects. Note that the very same error is communicated to the runc init parent and is ultimately returned by container.Run(), so checking what StartInitialization returned is redundant. Remove logrus setup and use from TestMain/init. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/execin_test.go | 11 +++-------- libcontainer/integration/init_test.go | 17 +++++++++-------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index f8a6a9c69..58519a419 100644 --- a/libcontainer/integration/execin_test.go +++ b/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()) - } } } diff --git a/libcontainer/integration/init_test.go b/libcontainer/integration/init_test.go index 76638ffc7..efcbe72b0 100644 --- a/libcontainer/integration/init_test.go +++ b/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) }