From 304a4c0fee6d1745b4154883fc37233268f3a2cc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 May 2024 17:43:25 -0700 Subject: [PATCH 1/3] libct: createExecFifo: rm unneeded os.Stat In case file already exists, mknod(2) will return EEXIST. This os.Stat call was (inadvertently?) added by commit 805b8c73. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index ad49244a889..cb68b30a209 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -417,9 +417,6 @@ func (c *Container) createExecFifo() error { } fifoName := filepath.Join(c.stateDir, execFifoFilename) - if _, err := os.Stat(fifoName); err == nil { - return fmt.Errorf("exec fifo %s already exists", fifoName) - } if err := unix.Mkfifo(fifoName, 0o622); err != nil { return &os.PathError{Op: "mkfifo", Path: fifoName, Err: err} } From e3e107257581e676208a0493a78e78ddfdbc1d59 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 May 2024 17:56:19 -0700 Subject: [PATCH 2/3] libct: fix locking in Start/Run/Exec 1. The code to call c.exec from c.Run was initially added by commit 3aacff695. At the time, there was a lock in c.Run. That lock was removed by commit bd3c4f84, which resulted in part of c.Run executing without the lock. 2. All the Start/Run/Exec calls were a mere wrappers for start/run/exec adding a lock, but some more code crept into Start at some point, e.g. by commits 805b8c73 and 108ee85b8. Since the reason mentioned in commit 805b8c73 is no longer true after refactoring, we can fix this. Fix both issues by moving code out of wrappers, and adding locking into c.Run. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 34 +++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index cb68b30a209..7566afd25b3 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -204,28 +204,16 @@ func (c *Container) Set(config configs.Config) error { func (c *Container) Start(process *Process) error { c.m.Lock() defer c.m.Unlock() - if c.config.Cgroups.Resources.SkipDevices { - return errors.New("can't start container with SkipDevices set") - } - if process.Init { - if err := c.createExecFifo(); err != nil { - return err - } - } - if err := c.start(process); err != nil { - if process.Init { - c.deleteExecFifo() - } - return err - } - return nil + return c.start(process) } // Run immediately starts the process inside the container. Returns an error if // the process fails to start. It does not block waiting for the exec fifo // after start returns but opens the fifo after start returns. func (c *Container) Run(process *Process) error { - if err := c.Start(process); err != nil { + c.m.Lock() + defer c.m.Unlock() + if err := c.start(process); err != nil { return err } if process.Init { @@ -314,6 +302,20 @@ type openResult struct { } func (c *Container) start(process *Process) (retErr error) { + if c.config.Cgroups.Resources.SkipDevices { + return errors.New("can't start container with SkipDevices set") + } + if process.Init { + if err := c.createExecFifo(); err != nil { + return err + } + defer func() { + if retErr != nil { + c.deleteExecFifo() + } + }() + } + parent, err := c.newParentProcess(process) if err != nil { return fmt.Errorf("unable to create new parent process: %w", err) From 42cea2ecb43d7cf0637ac683d8edbc2b12013ae5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 6 May 2024 18:18:36 -0700 Subject: [PATCH 3/3] libct: don't allow to start second init process By definition, every container has only 1 init (i.e. PID 1) process. Apparently, libcontainer API supported running more than 1 init, and at least one tests mistakenly used it. Let's not allow that, erroring out if we already have init. Doing otherwise _probably_ results in some confusion inside the library. Fix two cases in libct/int which ran two inits inside a container. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 3 +++ libcontainer/integration/execin_test.go | 2 -- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 7566afd25b3..3af5da7dcd7 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -306,6 +306,9 @@ func (c *Container) start(process *Process) (retErr error) { return errors.New("can't start container with SkipDevices set") } if process.Init { + if c.initProcessStartTime != 0 { + return errors.New("container already has init process") + } if err := c.createExecFifo(); err != nil { return err } diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index c5c324130c6..b9683b76442 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -115,7 +115,6 @@ func testExecInRlimit(t *testing.T, userns bool) { // increase process rlimit higher than container rlimit to test per-process limit {Type: unix.RLIMIT_NOFILE, Hard: 1026, Soft: 1026}, }, - Init: true, } err = container.Run(ps) ok(t, err) @@ -359,7 +358,6 @@ func TestExecInEnvironment(t *testing.T) { Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, - Init: true, } err = container.Run(process2) ok(t, err)