From bd3c4f844abed063a0d0a8575eb596159f33732c Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Fri, 1 Jun 2018 12:56:13 -0700 Subject: [PATCH] Fix race in runc exec There is a race in runc exec when the init process stops just before the check for the container status. It is then wrongly assumed that we are trying to start an init process instead of an exec process. This commit add an Init field to libcontainer Process to distinguish between init and exec processes to prevent this race. Signed-off-by: Mrunal Patel --- exec.go | 1 + libcontainer/container_linux.go | 29 +++++++-------------- libcontainer/integration/checkpoint_test.go | 2 ++ libcontainer/integration/exec_test.go | 19 ++++++++++++++ libcontainer/integration/execin_test.go | 11 ++++++++ libcontainer/integration/seccomp_test.go | 3 +++ libcontainer/integration/utils_test.go | 1 + libcontainer/process.go | 3 +++ utils_linux.go | 7 +++-- 9 files changed, 54 insertions(+), 22 deletions(-) diff --git a/exec.go b/exec.go index 5696ce6e2..ae8e60e42 100644 --- a/exec.go +++ b/exec.go @@ -140,6 +140,7 @@ func execProcess(context *cli.Context) (int, error) { detach: detach, pidFile: context.String("pid-file"), action: CT_ACT_RUN, + init: false, } return r.run(p) } diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 3160f9699..6bd1dd154 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -224,17 +224,13 @@ func (c *linuxContainer) Set(config configs.Config) error { func (c *linuxContainer) Start(process *Process) error { c.m.Lock() defer c.m.Unlock() - status, err := c.currentStatus() - if err != nil { - return err - } - if status == Stopped { + if process.Init { if err := c.createExecFifo(); err != nil { return err } } - if err := c.start(process, status == Stopped); err != nil { - if status == Stopped { + if err := c.start(process); err != nil { + if process.Init { c.deleteExecFifo() } return err @@ -243,17 +239,10 @@ func (c *linuxContainer) Start(process *Process) error { } func (c *linuxContainer) Run(process *Process) error { - c.m.Lock() - status, err := c.currentStatus() - if err != nil { - c.m.Unlock() - return err - } - c.m.Unlock() if err := c.Start(process); err != nil { return err } - if status == Stopped { + if process.Init { return c.exec() } return nil @@ -334,8 +323,8 @@ type openResult struct { err error } -func (c *linuxContainer) start(process *Process, isInit bool) error { - parent, err := c.newParentProcess(process, isInit) +func (c *linuxContainer) start(process *Process) error { + parent, err := c.newParentProcess(process) if err != nil { return newSystemErrorWithCause(err, "creating new parent process") } @@ -348,7 +337,7 @@ func (c *linuxContainer) start(process *Process, isInit bool) error { } // generate a timestamp indicating when the container was started c.created = time.Now().UTC() - if isInit { + if process.Init { c.state = &createdState{ c: c, } @@ -438,7 +427,7 @@ func (c *linuxContainer) includeExecFifo(cmd *exec.Cmd) error { return nil } -func (c *linuxContainer) newParentProcess(p *Process, doInit bool) (parentProcess, error) { +func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) { parentPipe, childPipe, err := utils.NewSockPair("init") if err != nil { return nil, newSystemErrorWithCause(err, "creating new init pipe") @@ -447,7 +436,7 @@ func (c *linuxContainer) newParentProcess(p *Process, doInit bool) (parentProces if err != nil { return nil, newSystemErrorWithCause(err, "creating new command template") } - if !doInit { + if !p.Init { return c.newSetnsProcess(p, cmd, parentPipe, childPipe) } diff --git a/libcontainer/integration/checkpoint_test.go b/libcontainer/integration/checkpoint_test.go index 9645999dd..63fa14f0f 100644 --- a/libcontainer/integration/checkpoint_test.go +++ b/libcontainer/integration/checkpoint_test.go @@ -110,6 +110,7 @@ func testCheckpoint(t *testing.T, userns bool) { Env: standardEnvironment, Stdin: stdinR, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) @@ -205,6 +206,7 @@ func testCheckpoint(t *testing.T, userns bool) { Cwd: "/", Stdin: restoreStdinR, Stdout: &stdout, + Init: true, } err = container.Restore(restoreProcessConfig, checkpointOpts) diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 0ef425be9..932024d2f 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -231,6 +231,7 @@ func TestEnter(t *testing.T) { Env: standardEnvironment, Stdin: stdinR, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) stdinR.Close() @@ -320,6 +321,7 @@ func TestProcessEnv(t *testing.T) { }, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -366,6 +368,7 @@ func TestProcessEmptyCaps(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -417,6 +420,7 @@ func TestProcessCaps(t *testing.T) { Stdin: nil, Stdout: &stdout, Capabilities: &configs.Capabilities{}, + Init: true, } pconfig.Capabilities.Bounding = append(config.Capabilities.Bounding, "CAP_NET_ADMIN") pconfig.Capabilities.Permitted = append(config.Capabilities.Permitted, "CAP_NET_ADMIN") @@ -491,6 +495,7 @@ func TestAdditionalGroups(t *testing.T) { Stdin: nil, Stdout: &stdout, AdditionalGroups: []string{"plugdev", "audio"}, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -551,6 +556,7 @@ func testFreeze(t *testing.T, systemd bool) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(pconfig) stdinR.Close() @@ -762,6 +768,7 @@ func TestContainerState(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(p) if err != nil { @@ -821,6 +828,7 @@ func TestPassExtraFiles(t *testing.T) { ExtraFiles: []*os.File{pipein1, pipein2}, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&process) if err != nil { @@ -902,6 +910,7 @@ func TestMountCmds(t *testing.T) { Cwd: "/", Args: []string{"sh", "-c", "env"}, Env: standardEnvironment, + Init: true, } err = container.Run(&pconfig) if err != nil { @@ -951,6 +960,7 @@ func TestSysctl(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -1091,6 +1101,7 @@ func TestOomScoreAdj(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -1196,6 +1207,7 @@ func TestHook(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) @@ -1312,6 +1324,7 @@ func TestRootfsPropagationSlaveMount(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(pconfig) @@ -1429,6 +1442,7 @@ func TestRootfsPropagationSharedMount(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(pconfig) @@ -1537,6 +1551,7 @@ func TestInitJoinPID(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR1, + Init: true, } err = container1.Run(init1) stdinR1.Close() @@ -1563,6 +1578,7 @@ func TestInitJoinPID(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR2, + Init: true, } err = container2.Run(init2) stdinR2.Close() @@ -1642,6 +1658,7 @@ func TestInitJoinNetworkAndUser(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR1, + Init: true, } err = container1.Run(init1) stdinR1.Close() @@ -1676,6 +1693,7 @@ func TestInitJoinNetworkAndUser(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR2, + Init: true, } err = container2.Run(init2) stdinR2.Close() @@ -1743,6 +1761,7 @@ func TestTmpfsCopyUp(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, + Init: true, } err = container.Run(&pconfig) ok(t, err) diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index 56fa3c75c..14f8a5964 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -38,6 +38,7 @@ func TestExecIn(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -108,6 +109,7 @@ func testExecInRlimit(t *testing.T, userns bool) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -126,6 +128,7 @@ 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) @@ -162,6 +165,7 @@ func TestExecInAdditionalGroups(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -218,6 +222,7 @@ func TestExecInError(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -270,6 +275,7 @@ func TestExecInTTY(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -366,6 +372,7 @@ func TestExecInEnvironment(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -385,6 +392,7 @@ func TestExecInEnvironment(t *testing.T) { Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, + Init: true, } err = container.Run(process2) ok(t, err) @@ -430,6 +438,7 @@ func TestExecinPassExtraFiles(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -509,6 +518,7 @@ func TestExecInOomScoreAdj(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() @@ -564,6 +574,7 @@ func TestExecInUserns(t *testing.T) { Args: []string{"cat"}, Env: standardEnvironment, Stdin: stdinR, + Init: true, } err = container.Run(process) stdinR.Close() diff --git a/libcontainer/integration/seccomp_test.go b/libcontainer/integration/seccomp_test.go index 3d80032e4..77f1a8d46 100644 --- a/libcontainer/integration/seccomp_test.go +++ b/libcontainer/integration/seccomp_test.go @@ -48,6 +48,7 @@ func TestSeccompDenyGetcwd(t *testing.T) { Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, + Init: true, } err = container.Run(pwd) @@ -123,6 +124,7 @@ func TestSeccompPermitWriteConditional(t *testing.T) { Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, + Init: true, } err = container.Run(dmesg) @@ -184,6 +186,7 @@ func TestSeccompDenyWriteConditional(t *testing.T) { Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, + Init: true, } err = container.Run(dmesg) diff --git a/libcontainer/integration/utils_test.go b/libcontainer/integration/utils_test.go index 25500032c..541d42eb8 100644 --- a/libcontainer/integration/utils_test.go +++ b/libcontainer/integration/utils_test.go @@ -152,6 +152,7 @@ func runContainer(config *configs.Config, console string, args ...string) (buffe Stdin: buffers.Stdin, Stdout: buffers.Stdout, Stderr: buffers.Stderr, + Init: true, } err = container.Run(process) diff --git a/libcontainer/process.go b/libcontainer/process.go index 86bf7387f..9a7c60141 100644 --- a/libcontainer/process.go +++ b/libcontainer/process.go @@ -72,6 +72,9 @@ type Process struct { // ConsoleSocket provides the masterfd console. ConsoleSocket *os.File + // Init specifies whether the process is the first process in the container. + Init bool + ops processOperations } diff --git a/utils_linux.go b/utils_linux.go index 3944eb4b5..c6a348973 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -105,7 +105,7 @@ func getDefaultImagePath(context *cli.Context) string { // 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) { +func newProcess(p specs.Process, init bool) (*libcontainer.Process, error) { lp := &libcontainer.Process{ Args: p.Args, Env: p.Env, @@ -115,6 +115,7 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) { Label: p.SelinuxLabel, NoNewPrivileges: &p.NoNewPrivileges, AppArmorProfile: p.ApparmorProfile, + Init: init, } if p.ConsoleSize != nil { @@ -269,6 +270,7 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (libcont } type runner struct { + init bool enableSubreaper bool shouldDestroy bool detach bool @@ -287,7 +289,7 @@ func (r *runner) run(config *specs.Process) (int, error) { r.destroy() return -1, err } - process, err := newProcess(*config) + process, err := newProcess(*config, r.init) if err != nil { r.destroy() return -1, err @@ -450,6 +452,7 @@ func startContainer(context *cli.Context, spec *specs.Spec, action CtAct, criuOp preserveFDs: context.Int("preserve-fds"), action: action, criuOpts: criuOpts, + init: true, } return r.run(spec.Process) }