From fdb100d24717209f72737d1f9a433f3ae6ea6182 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 14 Mar 2016 15:46:08 -0700 Subject: [PATCH] Destroy container along with processes before stdio We need to make sure the container is destroyed before closing the stdio for the container. This becomes a big issues when running in the host's pid namespace because the other processes could have inherited the stdio of the initial process. The call to close will just block as they still have the io open. Calling destroy before closing io, especially in the host pid namespace will cause all additional processes to be killed in the container's cgroup. This will allow the io to be closed successfuly. This change makes sure the order for destroy and close is correct as well as ensuring that if any errors encoutered during start or exec will be handled by terminating the process and destroying the container. We cannot use defers here because we need to enforce the correct ordering on destroy. This also sets the subreaper setting for runc so that when running in pid host, runc can wait on the addiontal processes launched by the container, useful on destroy, but also good for reaping the additional processes that were launched. Signed-off-by: Michael Crosby --- exec.go | 14 ++++++- libcontainer/init_linux.go | 13 ++++--- libcontainer/system/linux.go | 18 +++++++++ restore.go | 7 +++- signals.go | 16 ++++---- start.go | 26 ++++++------- utils.go | 73 +++++++++++++++++++++++++----------- 7 files changed, 116 insertions(+), 51 deletions(-) diff --git a/exec.go b/exec.go index 0f7fe8819..85ba352f3 100644 --- a/exec.go +++ b/exec.go @@ -76,6 +76,10 @@ following will output a list of processes running in the container: Value: &cli.StringSlice{}, Usage: "add a capability to the bounding set for the process", }, + cli.BoolFlag{ + Name: "no-subreaper", + Usage: "disable the use of the subreaper used to reap reparented processes", + }, }, Action: func(context *cli.Context) { if os.Geteuid() != 0 { @@ -104,7 +108,15 @@ func execProcess(context *cli.Context) (int, error) { if err != nil { return -1, err } - return runProcess(container, p, nil, context.String("console"), context.String("pid-file"), detach) + r := &runner{ + enableSubreaper: !context.Bool("no-subreaper"), + shouldDestroy: false, + container: container, + console: context.String("console"), + detach: detach, + pidFile: context.String("pid-file"), + } + return r.run(p) } func getProcess(context *cli.Context, bundle string) (*specs.Process, error) { diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 24e8f7146..eb8ad83ae 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -346,11 +346,14 @@ func killCgroupProcesses(m cgroups.Manager) error { return err } for _, pid := range pids { - if p, err := os.FindProcess(pid); err == nil { - procs = append(procs, p) - if err := p.Kill(); err != nil { - logrus.Warn(err) - } + p, err := os.FindProcess(pid) + if err != nil { + logrus.Warn(err) + continue + } + procs = append(procs, p) + if err := p.Kill(); err != nil { + logrus.Warn(err) } } if err := m.Freeze(configs.Thawed); err != nil { diff --git a/libcontainer/system/linux.go b/libcontainer/system/linux.go index babf55048..d109d7f88 100644 --- a/libcontainer/system/linux.go +++ b/libcontainer/system/linux.go @@ -11,6 +11,19 @@ import ( "unsafe" ) +// If arg2 is nonzero, set the "child subreaper" attribute of the +// calling process; if arg2 is zero, unset the attribute. When a +// process is marked as a child subreaper, all of the children +// that it creates, and their descendants, will be marked as +// having a subreaper. In effect, a subreaper fulfills the role +// of init(1) for its descendant processes. Upon termination of +// a process that is orphaned (i.e., its immediate parent has +// already terminated) and marked as having a subreaper, the +// nearest still living ancestor subreaper will receive a SIGCHLD +// signal and be able to wait(2) on the process to discover its +// termination status. +const PR_SET_CHILD_SUBREAPER = 36 + type ParentDeathSignal int func (p ParentDeathSignal) Restore() error { @@ -113,6 +126,11 @@ func RunningInUserNS() bool { return true } +// SetSubreaper sets the value i as the subreaper setting for the calling process +func SetSubreaper(i int) error { + return Prctl(PR_SET_CHILD_SUBREAPER, uintptr(i), 0, 0, 0) +} + func Prctl(option int, arg2, arg3, arg4, arg5 uintptr) (err error) { _, _, e1 := syscall.Syscall6(syscall.SYS_PRCTL, uintptr(option), arg2, arg3, arg4, arg5, 0) if e1 != 0 { diff --git a/restore.go b/restore.go index 6883d9177..873540d2e 100644 --- a/restore.go +++ b/restore.go @@ -68,6 +68,10 @@ using the runc checkpoint command.`, Value: "", Usage: "specify the file to write the process id to", }, + cli.BoolFlag{ + Name: "no-subreaper", + Usage: "disable the use of the subreaper used to reap reparented processes", + }, }, Action: func(context *cli.Context) { imagePath := context.String("image-path") @@ -140,8 +144,7 @@ func restoreContainer(context *cli.Context, spec *specs.Spec, config *configs.Co return -1, err } defer tty.Close() - handler := newSignalHandler(tty) - defer handler.Close() + handler := newSignalHandler(tty, !context.Bool("no-subreaper")) if err := container.Restore(process, options); err != nil { return -1, err } diff --git a/signals.go b/signals.go index f4ea61e88..5eee44e76 100644 --- a/signals.go +++ b/signals.go @@ -9,6 +9,7 @@ import ( "github.com/Sirupsen/logrus" "github.com/opencontainers/runc/libcontainer" + "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/runc/libcontainer/utils" ) @@ -16,7 +17,13 @@ const signalBufferSize = 2048 // newSignalHandler returns a signal handler for processing SIGCHLD and SIGWINCH signals // while still forwarding all other signals to the process. -func newSignalHandler(tty *tty) *signalHandler { +func newSignalHandler(tty *tty, enableSubreaper bool) *signalHandler { + if enableSubreaper { + // set us as the subreaper before registering the signal handler for the container + if err := system.SetSubreaper(1); err != nil { + logrus.Warn(err) + } + } // ensure that we have a large buffer size so that we do not miss any signals // incase we are not processing them fast enough. s := make(chan os.Signal, signalBufferSize) @@ -107,10 +114,3 @@ func (h *signalHandler) reap() (exits []exit, err error) { }) } } - -func (h *signalHandler) Close() error { - if h.tty != nil { - return h.tty.Close() - } - return nil -} diff --git a/start.go b/start.go index e8b4f5cb5..17a595fc9 100644 --- a/start.go +++ b/start.go @@ -43,6 +43,10 @@ is a directory with a specification file and a root filesystem.`, Value: "", Usage: "specify the file to write the process id to", }, + cli.BoolFlag{ + Name: "no-subreaper", + Usage: "disable the use of the subreaper used to reap reparented processes", + }, }, Action: func(context *cli.Context) { bundle := context.String("bundle") @@ -100,24 +104,20 @@ func startContainer(context *cli.Context, spec *specs.Spec) (int, error) { if err != nil { return -1, err } - - // ensure that the container is always removed if we were the process - // that created it. detach := context.Bool("detach") - if !detach { - defer destroy(container) - } - // Support on-demand socket activation by passing file descriptors into the container init process. listenFDs := []*os.File{} if os.Getenv("LISTEN_FDS") != "" { listenFDs = activation.Files(false) } - - status, err := runProcess(container, &spec.Process, listenFDs, context.String("console"), context.String("pid-file"), detach) - if err != nil { - destroy(container) - return -1, err + r := &runner{ + enableSubreaper: !context.Bool("no-subreaper"), + shouldDestroy: true, + container: container, + listenFDs: listenFDs, + console: context.String("console"), + detach: detach, + pidFile: context.String("pid-file"), } - return status, nil + return r.run(&spec.Process) } diff --git a/utils.go b/utils.go index 3b43e8536..45600ef18 100644 --- a/utils.go +++ b/utils.go @@ -321,47 +321,76 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (libcont return factory.Create(id, config) } -// runProcess will create a new process in the specified container -// by executing the process specified in the 'config'. -func runProcess(container libcontainer.Container, config *specs.Process, listenFDs []*os.File, console string, pidFile string, detach bool) (int, error) { +type runner struct { + enableSubreaper bool + shouldDestroy bool + detach bool + listenFDs []*os.File + pidFile string + console string + container libcontainer.Container +} + +func (r *runner) run(config *specs.Process) (int, error) { process, err := newProcess(*config) if err != nil { + r.destroy() return -1, err } - - // Add extra file descriptors if needed - if len(listenFDs) > 0 { - process.Env = append(process.Env, fmt.Sprintf("LISTEN_FDS=%d", len(listenFDs)), "LISTEN_PID=1") - process.ExtraFiles = append(process.ExtraFiles, listenFDs...) + if len(r.listenFDs) > 0 { + process.Env = append(process.Env, fmt.Sprintf("LISTEN_FDS=%d", len(r.listenFDs)), "LISTEN_PID=1") + process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...) } - - rootuid, err := container.Config().HostUID() + rootuid, err := r.container.Config().HostUID() if err != nil { + r.destroy() return -1, err } - - tty, err := setupIO(process, rootuid, console, config.Terminal, detach) + tty, err := setupIO(process, rootuid, r.console, config.Terminal, r.detach) if err != nil { + r.destroy() return -1, err } - defer tty.Close() - handler := newSignalHandler(tty) - defer handler.Close() - if err := container.Start(process); err != nil { + handler := newSignalHandler(tty, r.enableSubreaper) + if err := r.container.Start(process); err != nil { + r.destroy() + tty.Close() return -1, err } if err := tty.ClosePostStart(); err != nil { + r.terminate(process) + r.destroy() + tty.Close() return -1, err } - if pidFile != "" { - if err := createPidFile(pidFile, process); err != nil { - process.Signal(syscall.SIGKILL) - process.Wait() + if r.pidFile != "" { + if err := createPidFile(r.pidFile, process); err != nil { + r.terminate(process) + r.destroy() + tty.Close() return -1, err } } - if detach { + if r.detach { + tty.Close() return 0, nil } - return handler.forward(process) + status, err := handler.forward(process) + if err != nil { + r.terminate(process) + } + r.destroy() + tty.Close() + return status, err +} + +func (r *runner) destroy() { + if r.shouldDestroy { + destroy(r.container) + } +} + +func (r *runner) terminate(p *libcontainer.Process) { + p.Signal(syscall.SIGKILL) + p.Wait() }