From 7616be80949318c282ecec02845dd15cd5229218 Mon Sep 17 00:00:00 2001 From: Alex Dadgar Date: Mon, 18 Apr 2016 17:20:11 -0700 Subject: [PATCH] Put the executor into the cgroup to avoid a fork race --- client/driver/exec.go | 8 +++++-- client/driver/executor/executor.go | 12 ++++++---- client/driver/executor/executor_basic.go | 2 +- client/driver/executor/executor_linux.go | 30 ++++++++++++++++++++---- client/driver/java.go | 8 +++++-- 5 files changed, 46 insertions(+), 14 deletions(-) diff --git a/client/driver/exec.go b/client/driver/exec.go index d6a4cac79..4bdfb7fcd 100644 --- a/client/driver/exec.go +++ b/client/driver/exec.go @@ -219,7 +219,9 @@ func (d *ExecDriver) Open(ctx *ExecContext, handleID string) (DriverHandle, erro merrs.Errors = append(merrs.Errors, fmt.Errorf("error destroying plugin and userpid: %v", e)) } if id.IsolationConfig != nil { - if e := executor.DestroyCgroup(id.IsolationConfig.Cgroup, id.IsolationConfig.CgroupPaths); e != nil { + isoConf := id.IsolationConfig + ePid := pluginConfig.Reattach.Pid + if e := executor.DestroyCgroup(isoConf.Cgroup, isoConf.CgroupPaths, ePid); e != nil { merrs.Errors = append(merrs.Errors, fmt.Errorf("destroying cgroup failed: %v", e)) } } @@ -317,7 +319,9 @@ func (h *execHandle) run() { // user pid might be holding onto. if ps.ExitCode == 0 && err != nil { if h.isolationConfig != nil { - if e := executor.DestroyCgroup(h.isolationConfig.Cgroup, h.isolationConfig.CgroupPaths); e != nil { + isoConf := h.isolationConfig + ePid := h.pluginClient.ReattachConfig().Pid + if e := executor.DestroyCgroup(isoConf.Cgroup, isoConf.CgroupPaths, ePid); e != nil { h.logger.Printf("[ERR] driver.exec: destroying cgroup failed while killing cgroup: %v", e) } } diff --git a/client/driver/executor/executor.go b/client/driver/executor/executor.go index 24dab3081..64fe0157b 100644 --- a/client/driver/executor/executor.go +++ b/client/driver/executor/executor.go @@ -239,11 +239,15 @@ func (e *UniversalExecutor) LaunchCmd(command *ExecCommand, ctx *ExecutorContext e.cmd.Args = append([]string{path}, ctx.TaskEnv.ParseAndReplace(command.Args)...) e.cmd.Env = ctx.TaskEnv.EnvList() - // Start the process - if err := e.cmd.Start(); err != nil { + // Apply ourselves into the cgroup. The executor MUST be in the cgroup + // before the user task is started, otherwise we are subject to a fork + // attack in which a process escapes isolation by immediately forking. + if err := e.applyLimits(os.Getpid()); err != nil { return nil, err } - if err := e.applyLimits(e.cmd.Process.Pid); err != nil { + + // Start the process + if err := e.cmd.Start(); err != nil { return nil, err } go e.wait() @@ -376,7 +380,7 @@ func (e *UniversalExecutor) Exit() error { } if e.command != nil && e.command.ResourceLimits { e.cgLock.Lock() - if err := DestroyCgroup(e.groups, e.cgPaths); err != nil { + if err := DestroyCgroup(e.groups, e.cgPaths, os.Getpid()); err != nil { merr.Errors = append(merr.Errors, err) } e.cgLock.Unlock() diff --git a/client/driver/executor/executor_basic.go b/client/driver/executor/executor_basic.go index 18aef8d54..db2479d82 100644 --- a/client/driver/executor/executor_basic.go +++ b/client/driver/executor/executor_basic.go @@ -8,7 +8,7 @@ func (e *UniversalExecutor) configureChroot() error { return nil } -func DestroyCgroup(groups *cgroupConfig.Cgroup, paths map[string]string) error { +func DestroyCgroup(groups *cgroupConfig.Cgroup, paths map[string]string, executorPid int) error { return nil } diff --git a/client/driver/executor/executor_linux.go b/client/driver/executor/executor_linux.go index 16f3cd40b..416ca29d5 100644 --- a/client/driver/executor/executor_linux.go +++ b/client/driver/executor/executor_linux.go @@ -67,7 +67,7 @@ func (e *UniversalExecutor) applyLimits(pid int) error { cgConfig := cgroupConfig.Config{Cgroups: e.groups} if err := manager.Set(&cgConfig); err != nil { e.logger.Printf("[ERR] executor: error setting cgroup config: %v", err) - if er := DestroyCgroup(e.groups, e.cgPaths); er != nil { + if er := DestroyCgroup(e.groups, e.cgPaths, os.Getpid()); er != nil { e.logger.Printf("[ERR] executor: error destroying cgroup: %v", er) } if er := e.removeChrootMounts(); er != nil { @@ -187,15 +187,21 @@ func (e *UniversalExecutor) removeChrootMounts() error { // destroyCgroup kills all processes in the cgroup and removes the cgroup // configuration from the host. -func DestroyCgroup(groups *cgroupConfig.Cgroup, cgPaths map[string]string) error { +func DestroyCgroup(groups *cgroupConfig.Cgroup, cgPaths map[string]string, executorPid int) error { merrs := new(multierror.Error) if groups == nil { return fmt.Errorf("Can't destroy: cgroup configuration empty") } - manager := getCgroupManager(groups, cgPaths) - if pids, perr := manager.GetPids(); perr == nil { + if pids, perr := manager.GetAllPids(); perr == nil { for _, pid := range pids { + // If the pid is the pid of the executor then we don't kill it, the + // executor is going to be killed by the driver once the Wait + // returns + if pid == executorPid { + continue + } + proc, err := os.FindProcess(pid) if err != nil { merrs.Errors = append(merrs.Errors, fmt.Errorf("error finding process %v: %v", pid, err)) @@ -203,15 +209,29 @@ func DestroyCgroup(groups *cgroupConfig.Cgroup, cgPaths map[string]string) error if e := proc.Kill(); e != nil { merrs.Errors = append(merrs.Errors, fmt.Errorf("error killing process %v: %v", pid, e)) } + + // Don't capture the error because we expect this to fail for + // processes we didn't fork. + proc.Wait() } } } else { merrs.Errors = append(merrs.Errors, fmt.Errorf("error getting pids: %v", perr)) } + // Move the executor into the global cgroup so that the task specific + // cgroup can be destroyed. + nilGroup := &cgroupConfig.Cgroup{} + nilGroup.Path = "/" + nilGroup.Resources = groups.Resources + nilManager := getCgroupManager(nilGroup, nil) + if err := nilManager.Apply(executorPid); err != nil { + multierror.Append(merrs, fmt.Errorf("failed to remove executor pid %d: %v", executorPid, err)) + } + // Remove the cgroup. if err := manager.Destroy(); err != nil { - multierror.Append(merrs, fmt.Errorf("Failed to delete the cgroup directories: %v", err)) + multierror.Append(merrs, fmt.Errorf("failed to delete the cgroup directories: %v", err)) } return merrs.ErrorOrNil() } diff --git a/client/driver/java.go b/client/driver/java.go index 93d85c7b1..acd2289a4 100644 --- a/client/driver/java.go +++ b/client/driver/java.go @@ -286,7 +286,9 @@ func (d *JavaDriver) Open(ctx *ExecContext, handleID string) (DriverHandle, erro merrs.Errors = append(merrs.Errors, fmt.Errorf("error destroying plugin and userpid: %v", e)) } if id.IsolationConfig != nil { - if e := executor.DestroyCgroup(id.IsolationConfig.Cgroup, id.IsolationConfig.CgroupPaths); e != nil { + isoConf := id.IsolationConfig + ePid := pluginConfig.Reattach.Pid + if e := executor.DestroyCgroup(isoConf.Cgroup, isoConf.CgroupPaths, ePid); e != nil { merrs.Errors = append(merrs.Errors, fmt.Errorf("destroying cgroup failed: %v", e)) } } @@ -383,7 +385,9 @@ func (h *javaHandle) run() { close(h.doneCh) if ps.ExitCode == 0 && err != nil { if h.isolationConfig != nil { - if e := executor.DestroyCgroup(h.isolationConfig.Cgroup, h.isolationConfig.CgroupPaths); e != nil { + isoConf := h.isolationConfig + ePid := h.pluginClient.ReattachConfig().Pid + if e := executor.DestroyCgroup(isoConf.Cgroup, isoConf.CgroupPaths, ePid); e != nil { h.logger.Printf("[ERR] driver.java: destroying cgroup failed while killing cgroup: %v", e) } } else {