From 36786c361a022e039d2679588ea1ef3e5ed72978 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Feb 2022 18:30:46 -0800 Subject: [PATCH 1/6] list, utils: remove redundant code The value of root is already an absolute path since commit ede8a86ec1e35, so it does not make sense to call filepath.Abs() again. Signed-off-by: Kir Kolyshkin --- list.go | 7 +------ utils_linux.go | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/list.go b/list.go index 3fb9917248f..5aa9bebddb1 100644 --- a/list.go +++ b/list.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "syscall" "text/tabwriter" "time" @@ -116,11 +115,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { return nil, err } root := context.GlobalString("root") - absRoot, err := filepath.Abs(root) - if err != nil { - return nil, err - } - list, err := os.ReadDir(absRoot) + list, err := os.ReadDir(root) if err != nil { fatal(err) } diff --git a/utils_linux.go b/utils_linux.go index c2214a23377..e0d1cae7472 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -26,12 +26,7 @@ var errEmptyID = errors.New("container id cannot be empty") // loadFactory returns the configured factory instance for execing containers. func loadFactory(context *cli.Context) (libcontainer.Factory, error) { root := context.GlobalString("root") - abs, err := filepath.Abs(root) - if err != nil { - return nil, err - } - - return libcontainer.New(abs) + return libcontainer.New(root) } // getContainer returns the specified container instance by loading it from state From eb2f08dc4e68d794ec80c8d867d953b3f7660b67 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Feb 2022 18:52:08 -0800 Subject: [PATCH 2/6] checkpoint,restore,list: don't call fatal There is a mix of styles when handling CLI commands. In most cases we return an error, which is handled from app.Run in main.go (it calls fatal if there is an error). In a few cases, though, we call fatal(err) from random places. Let's be consistent and always return an error. The only exception is runc exec, which needs to exit with a particular exit code. Signed-off-by: Kir Kolyshkin --- checkpoint.go | 28 +++++++++++++++++++--------- list.go | 4 ++-- restore.go | 11 +++++++---- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/checkpoint.go b/checkpoint.go index 32a62a8bcb2..7597e552df9 100644 --- a/checkpoint.go +++ b/checkpoint.go @@ -60,16 +60,24 @@ checkpointed.`, return err } if status == libcontainer.Created || status == libcontainer.Stopped { - fatal(fmt.Errorf("Container cannot be checkpointed in %s state", status.String())) + return fmt.Errorf("Container cannot be checkpointed in %s state", status.String()) } - options := criuOptions(context) + options, err := criuOptions(context) + if err != nil { + return err + } + if !(options.LeaveRunning || options.PreDump) { // destroy container unless we tell CRIU to keep it defer destroy(container) } // these are the mandatory criu options for a container - setPageServer(context, options) - setManageCgroupsMode(context, options) + if err := setPageServer(context, options); err != nil { + return err + } + if err := setManageCgroupsMode(context, options); err != nil { + return err + } if err := setEmptyNsMask(context, options); err != nil { return err } @@ -109,27 +117,28 @@ func prepareImagePaths(context *cli.Context) (string, string, error) { return imagePath, parentPath, nil } -func setPageServer(context *cli.Context, options *libcontainer.CriuOpts) { +func setPageServer(context *cli.Context, options *libcontainer.CriuOpts) error { // xxx following criu opts are optional // The dump image can be sent to a criu page server if psOpt := context.String("page-server"); psOpt != "" { address, port, err := net.SplitHostPort(psOpt) if err != nil || address == "" || port == "" { - fatal(errors.New("Use --page-server ADDRESS:PORT to specify page server")) + return errors.New("Use --page-server ADDRESS:PORT to specify page server") } portInt, err := strconv.Atoi(port) if err != nil { - fatal(errors.New("Invalid port number")) + return errors.New("Invalid port number") } options.PageServer = libcontainer.CriuPageServerInfo{ Address: address, Port: int32(portInt), } } + return nil } -func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts) { +func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts) error { if cgOpt := context.String("manage-cgroups-mode"); cgOpt != "" { switch cgOpt { case "soft": @@ -139,9 +148,10 @@ func setManageCgroupsMode(context *cli.Context, options *libcontainer.CriuOpts) case "strict": options.ManageCgroupsMode = criu.CriuCgMode_STRICT default: - fatal(errors.New("Invalid manage cgroups mode")) + return errors.New("Invalid manage cgroups mode") } } + return nil } var namespaceMapping = map[specs.LinuxNamespaceType]int{ diff --git a/list.go b/list.go index 5aa9bebddb1..2a1b0adc736 100644 --- a/list.go +++ b/list.go @@ -117,7 +117,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { root := context.GlobalString("root") list, err := os.ReadDir(root) if err != nil { - fatal(err) + return nil, err } var s []containerState @@ -131,7 +131,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { // Possible race with runc delete. continue } - fatal(err) + return nil, err } // This cast is safe on Linux. uid := st.Sys().(*syscall.Stat_t).Uid diff --git a/restore.go b/restore.go index 59d2904ec94..ccd1b232bc9 100644 --- a/restore.go +++ b/restore.go @@ -109,7 +109,10 @@ using the runc checkpoint command.`, logrus.Warn("runc checkpoint is untested with rootless containers") } - options := criuOptions(context) + options, err := criuOptions(context) + if err != nil { + return err + } if err := setEmptyNsMask(context, options); err != nil { return err } @@ -124,10 +127,10 @@ using the runc checkpoint command.`, }, } -func criuOptions(context *cli.Context) *libcontainer.CriuOpts { +func criuOptions(context *cli.Context) (*libcontainer.CriuOpts, error) { imagePath, parentPath, err := prepareImagePaths(context) if err != nil { - fatal(err) + return nil, err } return &libcontainer.CriuOpts{ @@ -145,5 +148,5 @@ func criuOptions(context *cli.Context) *libcontainer.CriuOpts { StatusFd: context.Int("status-fd"), LsmProfile: context.String("lsm-profile"), LsmMountContext: context.String("lsm-mount-context"), - } + }, nil } From 899342b5d49434611635d64f64c343e2a1aeee0a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 11 Feb 2022 13:14:01 -0800 Subject: [PATCH 3/6] main: improve XDG_RUNTIME_DIR handling 1. Variable xdgRuntimeDir is only checked to be non-empty. Change it to a boolean. 2. Refactor so that os.Getenv is not called twice. Signed-off-by: Kir Kolyshkin --- main.go | 13 ++++++------- rootless_linux.go | 3 --- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/main.go b/main.go index d6e4bbf81b3..56a6066f8c5 100644 --- a/main.go +++ b/main.go @@ -71,13 +71,12 @@ func main() { } app.Version = strings.Join(v, "\n") - xdgRuntimeDir := "" root := "/run/runc" - if shouldHonorXDGRuntimeDir() { - if runtimeDir := os.Getenv("XDG_RUNTIME_DIR"); runtimeDir != "" { - root = runtimeDir + "/runc" - xdgRuntimeDir = root - } + xdgDirUsed := false + xdgRuntimeDir := os.Getenv("XDG_RUNTIME_DIR") + if xdgRuntimeDir != "" && shouldHonorXDGRuntimeDir() { + root = xdgRuntimeDir + "/runc" + xdgDirUsed = true } app.Flags = []cli.Flag{ @@ -135,7 +134,7 @@ func main() { featuresCommand, } app.Before = func(context *cli.Context) error { - if !context.IsSet("root") && xdgRuntimeDir != "" { + if !context.IsSet("root") && xdgDirUsed { // According to the XDG specification, we need to set anything in // XDG_RUNTIME_DIR to have a sticky bit if we don't want it to get // auto-pruned. diff --git a/rootless_linux.go b/rootless_linux.go index ae01703365d..a1f54858635 100644 --- a/rootless_linux.go +++ b/rootless_linux.go @@ -52,9 +52,6 @@ func shouldUseRootlessCgroupManager(context *cli.Context) (bool, error) { } func shouldHonorXDGRuntimeDir() bool { - if os.Getenv("XDG_RUNTIME_DIR") == "" { - return false - } if os.Geteuid() != 0 { return true } From 2b07e751b5f982ed05629cdcab2d9ab87a68abff Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 11 Feb 2022 13:18:56 -0800 Subject: [PATCH 4/6] reviseRootDir: skip default values, add validation 1. In case --root option is not provided, do nothing. 2. Instead of checking if root value is empty string, check it after filepath.Abs, and reject "/". Improve docstring while at it. Signed-off-by: Kir Kolyshkin --- utils.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/utils.go b/utils.go index 32ab33e5506..75752f18374 100644 --- a/utils.go +++ b/utils.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "os" "path/filepath" @@ -96,17 +97,25 @@ func revisePidFile(context *cli.Context) error { return context.Set("pid-file", pidFile) } -// reviseRootDir convert the root to absolute path +// reviseRootDir ensures that the --root option argument, +// if specified, is converted to an absolute and cleaned path, +// and that this path is sane. func reviseRootDir(context *cli.Context) error { - root := context.GlobalString("root") - if root == "" { + if !context.IsSet("root") { return nil } - - root, err := filepath.Abs(root) + root, err := filepath.Abs(context.GlobalString("root")) if err != nil { return err } + if root == "/" { + // This can happen if --root argument is + // - "" (i.e. empty); + // - "." (and the CWD is /); + // - "../../.." (enough to get to /); + // - "/" (the actual /). + return errors.New("Option --root argument should not be set to /") + } return context.GlobalSet("root", root) } From d1fca8e599837a694ea4e9a6892b018456e6e1ae Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Feb 2022 18:58:08 -0800 Subject: [PATCH 5/6] list: report error when non-existent --root is specified It is questionable whether runc list should return an empty list of containers when non-existent --root is specified or not. The current behavior is the directory is always created and then the empty list of container is shown. To my mind, specifying a non-existent root is an error and should be reported as such. This is what this patch does. For backward compatibility, if --root is not set (i.e. a default is used), ENOENT is not reported. Signed-off-by: Kir Kolyshkin --- list.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/list.go b/list.go index 2a1b0adc736..f63c18d73b9 100644 --- a/list.go +++ b/list.go @@ -110,12 +110,19 @@ To list containers created using a non-default value for "--root": } func getContainers(context *cli.Context) ([]containerState, error) { - factory, err := loadFactory(context) + root := context.GlobalString("root") + list, err := os.ReadDir(root) if err != nil { + if errors.Is(err, os.ErrNotExist) && context.IsSet("root") { + // Ignore non-existing default root directory + // (no containers created yet). + return nil, nil + } + // Report other errors, including non-existent custom --root. return nil, err } - root := context.GlobalString("root") - list, err := os.ReadDir(root) + + factory, err := loadFactory(context) if err != nil { return nil, err } From 40b0088681bf29bf513ca8a2f44dbdc3a2417abf Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 10 Feb 2022 19:10:51 -0800 Subject: [PATCH 6/6] loadFactory: remove Signed-off-by: Kir Kolyshkin --- list.go | 3 +-- utils_linux.go | 12 ++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/list.go b/list.go index f63c18d73b9..80721e70c78 100644 --- a/list.go +++ b/list.go @@ -121,8 +121,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { // Report other errors, including non-existent custom --root. return nil, err } - - factory, err := loadFactory(context) + factory, err := libcontainer.New(root) if err != nil { return nil, err } diff --git a/utils_linux.go b/utils_linux.go index e0d1cae7472..7ec7b8a79ac 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -23,12 +23,6 @@ import ( var errEmptyID = errors.New("container id cannot be empty") -// loadFactory returns the configured factory instance for execing containers. -func loadFactory(context *cli.Context) (libcontainer.Factory, error) { - root := context.GlobalString("root") - return libcontainer.New(root) -} - // getContainer returns the specified container instance by loading it from state // with the default factory. func getContainer(context *cli.Context) (libcontainer.Container, error) { @@ -36,7 +30,8 @@ func getContainer(context *cli.Context) (libcontainer.Container, error) { if id == "" { return nil, errEmptyID } - factory, err := loadFactory(context) + root := context.GlobalString("root") + factory, err := libcontainer.New(root) if err != nil { return nil, err } @@ -189,7 +184,8 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (libcont return nil, err } - factory, err := loadFactory(context) + root := context.GlobalString("root") + factory, err := libcontainer.New(root) if err != nil { return nil, err }