From 5d2e24453fe8d0ac0a407b6f903a177ccebfd399 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 15 Jan 2025 19:20:41 -0800 Subject: [PATCH 1/4] execProcess: move some code to newProcess Let's move some code from execProcess to newProcess, fixing the following few issues: 1. container.State (which does quite a lot) is not needed -- we only need container.Config here. 2. utils.SearchLabels is not needed when "runc exec --process" is used. 3. Context.String("process") is called twice. 4. It is not very clear from the code why checking for len(context.Args()) is performed. Move the check to just before Args is used, to make it clear why. Signed-off-by: Kir Kolyshkin --- exec.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/exec.go b/exec.go index 16bbeebfb..08e4bbc46 100644 --- a/exec.go +++ b/exec.go @@ -158,19 +158,7 @@ func execProcess(context *cli.Context) (int, error) { if status == libcontainer.Paused && !context.Bool("ignore-paused") { return -1, errors.New("cannot exec in a paused container (use --ignore-paused to override)") } - path := context.String("process") - if path == "" && len(context.Args()) == 1 { - return -1, errors.New("process args cannot be empty") - } - state, err := container.State() - if err != nil { - return -1, err - } - bundle, ok := utils.SearchLabels(state.Config.Labels, "bundle") - if !ok { - return -1, errors.New("bundle not found in labels") - } - p, err := getProcess(context, bundle) + p, err := getProcess(context, container) if err != nil { return -1, err } @@ -196,7 +184,7 @@ func execProcess(context *cli.Context) (int, error) { return r.run(p) } -func getProcess(context *cli.Context, bundle string) (*specs.Process, error) { +func getProcess(context *cli.Context, c *libcontainer.Container) (*specs.Process, error) { if path := context.String("process"); path != "" { f, err := os.Open(path) if err != nil { @@ -209,7 +197,11 @@ func getProcess(context *cli.Context, bundle string) (*specs.Process, error) { } return &p, validateProcessSpec(&p) } - // process via cli flags + // Process from config.json and CLI flags. + bundle, ok := utils.SearchLabels(c.Config().Labels, "bundle") + if !ok { + return nil, errors.New("bundle not found in labels") + } if err := os.Chdir(bundle); err != nil { return nil, err } @@ -218,7 +210,11 @@ func getProcess(context *cli.Context, bundle string) (*specs.Process, error) { return nil, err } p := spec.Process - p.Args = context.Args()[1:] + args := context.Args() + if len(args) < 2 { + return nil, errors.New("exec args cannot be empty") + } + p.Args = args[1:] // Override the cwd, if passed. if cwd := context.String("cwd"); cwd != "" { p.Cwd = cwd From c4eb0c61e12fc0b47e4e0f0e1fb5fda0e7c0a91d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 15 Jan 2025 19:41:51 -0800 Subject: [PATCH 2/4] libct: createExecFifo: optimize Every time we call container.Config(), a new copy of struct Config is created and returned, and we do it twice here. Accessing container.config directly fixes this. Fixes: 805b8c73d ("Do not create exec fifo in factory.Create") Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 26f523a90..a411d4081 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -432,11 +432,11 @@ func (c *Container) signal(s os.Signal) error { } func (c *Container) createExecFifo() (retErr error) { - rootuid, err := c.Config().HostRootUID() + rootuid, err := c.config.HostRootUID() if err != nil { return err } - rootgid, err := c.Config().HostRootGID() + rootgid, err := c.config.HostRootGID() if err != nil { return err } From 8fbdb7e78e2db50f6c1f03b73683a04c2347a6f0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 15 Jan 2025 19:58:13 -0800 Subject: [PATCH 3/4] setupIO: optimize The rootuid and rootgid are only needed when detach and createTTY are both false. We also call c.Config() twice, every time creating a copy of struct Config. Solve both issues by passing container pointer to setupIO, and get rootuid/rootgid only when we need those. Signed-off-by: Kir Kolyshkin --- utils_linux.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index 95ba88af0..724d6bb70 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -89,7 +89,7 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) { } // setupIO modifies the given process config according to the options. -func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, detach bool, sockpath string) (*tty, error) { +func setupIO(process *libcontainer.Process, container *libcontainer.Container, createTTY, detach bool, sockpath string) (*tty, error) { if createTTY { process.Stdin = nil process.Stdout = nil @@ -135,6 +135,17 @@ func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, det inheritStdio(process) return &tty{}, nil } + + config := container.Config() + rootuid, err := config.HostRootUID() + if err != nil { + return nil, err + } + rootgid, err := config.HostRootGID() + if err != nil { + return nil, err + } + return setupProcessPipes(process, rootuid, rootgid) } @@ -232,20 +243,12 @@ func (r *runner) run(config *specs.Process) (int, error) { } process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i))) } - rootuid, err := r.container.Config().HostRootUID() - if err != nil { - return -1, err - } - rootgid, err := r.container.Config().HostRootGID() - if err != nil { - return -1, err - } detach := r.detach || (r.action == CT_ACT_CREATE) // Setting up IO is a two stage process. We need to modify process to deal // with detaching containers, and then we get a tty after the container has // started. handler := newSignalHandler(r.enableSubreaper, r.notifySocket) - tty, err := setupIO(process, rootuid, rootgid, config.Terminal, detach, r.consoleSocket) + tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket) if err != nil { return -1, err } From 4b87c7d4fd7d8f1de1a89f79491504781b0077d8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 15 Jan 2025 18:37:09 -0800 Subject: [PATCH 4/4] Fixups for newProcess 1. Pass an argument as a pointer rather than copying the whole structure. It was a pointer initially, but this has changed in commit b2d9d996 without giving a reason why. 2. The newProcess description was added by commit 9fac18329 (yes, the very first one) and hasn't changed since. As of commit 29b139f7, the part of it which says "and stdio from the current process" is no longer valid. Remove it, and while at it, rewrite the description entirely. Signed-off-by: Kir Kolyshkin --- utils_linux.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index 724d6bb70..c13ed585b 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -43,9 +43,8 @@ func getDefaultImagePath() string { return filepath.Join(cwd, "checkpoint") } -// newProcess returns a new libcontainer Process with the arguments from the -// spec and stdio from the current process. -func newProcess(p specs.Process) (*libcontainer.Process, error) { +// newProcess converts [specs.Process] to [libcontainer.Process]. +func newProcess(p *specs.Process) (*libcontainer.Process, error) { lp := &libcontainer.Process{ Args: p.Args, Env: p.Env, @@ -221,7 +220,7 @@ func (r *runner) run(config *specs.Process) (int, error) { if err = r.checkTerminal(config); err != nil { return -1, err } - process, err := newProcess(*config) + process, err := newProcess(config) if err != nil { return -1, err }