From 27a5447ea4325ad936e333adbc5b71355ab4aa2b Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Sat, 21 Jan 2017 18:02:01 +0000 Subject: [PATCH 1/2] Only wait for processes after delivering SIGKILL in signalAllProcesses signalAllProcesses was making the assumption that the requested signal was SIGKILL, possibly due to the signal parameter being added at a later date, and hence it was safe to wait for all processes which is not the case. BaseContainer.Signal(s os.Signal, all bool) exposes this functionality to consumers, so an arbitrary signal could be used which is not guaranteed to make the processes exit. Correct the documentation for signalAllProcesses around the signal delivered and update it so that the wait is only performed on SIGKILL hence making it safe to process other signals without risk of blocking forever, while still maintaining compatibility to SIGKILL callers. Signed-off-by: Steven Hartland --- libcontainer/init_linux.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 9d5f06802..cbd828a98 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -394,8 +394,8 @@ func setOomScoreAdj(oomScoreAdj int, pid int) error { } // signalAllProcesses freezes then iterates over all the processes inside the -// manager's cgroups sending a SIGKILL to each process then waiting for them to -// exit. +// manager's cgroups sending the signal s to them. +// If s is SIGKILL then it will also wait for each process to exit. func signalAllProcesses(m cgroups.Manager, s os.Signal) error { var procs []*os.Process if err := m.Freeze(configs.Frozen); err != nil { @@ -420,6 +420,11 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error { if err := m.Freeze(configs.Thawed); err != nil { logrus.Warn(err) } + + if s != syscall.SIGKILL { + return nil + } + for _, p := range procs { if _, err := p.Wait(); err != nil { logrus.Warn(err) From 82d895fbb9a256e1c0147e360b63864eed13db71 Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Thu, 26 Jan 2017 13:10:16 +0000 Subject: [PATCH 2/2] Conditionally wait for children after delivering signal When signaling children and the signal is SIGKILL wait for children otherwise conditionally wait for children which are ready to report. This reaps all children which exited due to the signal sent without blocking indefinitely. Also: * Ignore ignore ECHILD, which means the child has already gone. Signed-off-by: Steven Hartland --- libcontainer/init_linux.go | 64 ++++++++++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 6 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index cbd828a98..4c6bb53fa 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -12,6 +12,7 @@ import ( "strconv" "strings" "syscall" + "unsafe" "github.com/Sirupsen/logrus" "github.com/opencontainers/runc/libcontainer/cgroups" @@ -393,9 +394,50 @@ func setOomScoreAdj(oomScoreAdj int, pid int) error { return ioutil.WriteFile(path, []byte(strconv.Itoa(oomScoreAdj)), 0600) } +const _P_PID = 1 + +type siginfo struct { + si_signo int32 + si_errno int32 + si_code int32 + // below here is a union; si_pid is the only field we use + si_pid int32 + // Pad to 128 bytes as detailed in blockUntilWaitable + pad [96]byte +} + +// isWaitable returns true if the process has exited false otherwise. +// Its based off blockUntilWaitable in src/os/wait_waitid.go +func isWaitable(pid int) (bool, error) { + si := &siginfo{} + _, _, e := syscall.Syscall6(syscall.SYS_WAITID, _P_PID, uintptr(pid), uintptr(unsafe.Pointer(si)), syscall.WEXITED|syscall.WNOWAIT|syscall.WNOHANG, 0, 0) + if e != 0 { + return false, os.NewSyscallError("waitid", e) + } + + return si.si_pid != 0, nil +} + +// isNoChildren returns true if err represents a syscall.ECHILD false otherwise +func isNoChildren(err error) bool { + switch err := err.(type) { + case syscall.Errno: + if err == syscall.ECHILD { + return true + } + case *os.SyscallError: + if err.Err == syscall.ECHILD { + return true + } + } + return false +} + // signalAllProcesses freezes then iterates over all the processes inside the // manager's cgroups sending the signal s to them. -// If s is SIGKILL then it will also wait for each process to exit. +// If s is SIGKILL then it will wait for each process to exit. +// For all other signals it will check if the process is ready to report its +// exit status and only if it is will a wait be performed. func signalAllProcesses(m cgroups.Manager, s os.Signal) error { var procs []*os.Process if err := m.Freeze(configs.Frozen); err != nil { @@ -421,13 +463,23 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error { logrus.Warn(err) } - if s != syscall.SIGKILL { - return nil - } - for _, p := range procs { + if s != syscall.SIGKILL { + if ok, err := isWaitable(p.Pid); err != nil { + if !isNoChildren(err) { + logrus.Warn("signalAllProcesses: ", p.Pid, err) + } + continue + } else if !ok { + // Not ready to report so don't wait + continue + } + } + if _, err := p.Wait(); err != nil { - logrus.Warn(err) + if !isNoChildren(err) { + logrus.Warn("wait: ", err) + } } } return nil