mirror/runc
1
0
Fork 0
mirror of https://github.com/opencontainers/runc.git synced 2026-07-11 06:03:57 +08:00
Code Issues Packages Projects Releases Wiki Activity

libct: signalAllProcesses: remove child reaping

Browse Source 
There are two very distinct usage scenarios for signalAllProcesses:

* when used from the runc binary ("runc kill" command), the processes
  that it kills are not the children of "runc kill", and so calling
  wait(2) on each process is totally useless, as it will return ECHLD;

* when used from a program that have created the container (such as
  libcontainer/integration test suite), that program can and should call
  wait(2), not the signalling code.

So, the child reaping code is totally useless in the first case, and
should be implemented by the program using libcontainer in the second
case. I was not able to track down how this code was added, my best
guess is it happened when this code was part of dockerd, which did not
have a proper child reaper implemented at that time.

Remove it, and add a proper documentation piece.

Change the integration test accordingly.

PS the first attempt to disable the child reaping code in
signalAllProcesses was made in commit bb912eb00c, which used a
questionable heuristic to figure out whether wait(2) should be called.
This heuristic worked for a particular use case, but is not correct in
general.

While at it:
 - simplify signalAllProcesses to use unix.Kill;
 - document (container).Signal.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit is contained in:
Kir Kolyshkin
2023-05-12 11:04:21 -07:00
parent e0e8d9c886
commit 5b8f8712a4
3 changed files with 22 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files
libcontainer/container_linux.go
+11 -1
View File
@@ -357,6 +357,12 @@ func (c *Container) start(process *Process) (retErr error) {
return nil
}
// Signal sends a specified signal to container's init, or, if all is true,
// true, to all container's processes (as determined by container's cgroup).
//
// Setting all=true is useful when s is SIGKILL and the container does not have
// its own PID namespace. In this scenario, the libcontainer user may be required
// to implement a proper child reaper.
func (c *Container) Signal(s os.Signal, all bool) error {
c.m.Lock()
defer c.m.Unlock()
@@ -365,12 +371,16 @@ func (c *Container) Signal(s os.Signal, all bool) error {
return err
}
if all {
sig, ok := s.(unix.Signal)
if !ok {
return errors.New("unsupported signal type")
}
if status == Stopped && !c.cgroupManager.Exists() {
// Avoid calling signalAllProcesses which may print
// a warning trying to freeze a non-existing cgroup.
return nil
}
return c.ignoreCgroupError(signalAllProcesses(c.cgroupManager, s))
return c.ignoreCgroupError(signalAllProcesses(c.cgroupManager, sig))
}
// to avoid a PID reuse attack
if status == Running || status == Created || status == Paused {
libcontainer/init_linux.go
+4 -76
View File
@@ -11,7 +11,6 @@ import (
"runtime/debug"
"strconv"
"strings"
"unsafe"
"github.com/containerd/console"
"github.com/opencontainers/runtime-spec/specs-go"
@@ -575,39 +574,9 @@ func setupRlimits(limits []configs.Rlimit, pid int) error {
return nil
}
const _P_PID = 1
//nolint:structcheck,unused
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 := unix.Syscall6(unix.SYS_WAITID, _P_PID, uintptr(pid), uintptr(unsafe.Pointer(si)), unix.WEXITED|unix.WNOWAIT|unix.WNOHANG, 0, 0)
if e != 0 {
return false, &os.SyscallError{Syscall: "waitid", Err: e}
}
return si.si_pid != 0, nil
}
// signalAllProcesses freezes then iterates over all the processes inside the
// manager's cgroups sending the signal s to them.
// If s is SIGKILL and subreaper is not enabled 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
func signalAllProcesses(m cgroups.Manager, s unix.Signal) error {
if err := m.Freeze(configs.Frozen); err != nil {
logrus.Warn(err)
}
@@ -619,55 +588,14 @@ func signalAllProcesses(m cgroups.Manager, s os.Signal) error {
return err
}
for _, pid := range pids {
p, err := os.FindProcess(pid)
if err != nil {
logrus.Warn(err)
continue
}
procs = append(procs, p)
if err := p.Signal(s); err != nil {
logrus.Warn(err)
err := unix.Kill(pid, s)
if err != nil && err != unix.ESRCH { //nolint:errorlint // unix errors are bare
logrus.Warnf("kill %d: %v", pid, err)
}
}
if err := m.Freeze(configs.Thawed); err != nil {
logrus.Warn(err)
}
subreaper, err := system.GetSubreaper()
if err != nil {
// The error here means that PR_GET_CHILD_SUBREAPER is not
// supported because this code might run on a kernel older
// than 3.4. We don't want to throw an error in that case,
// and we simplify things, considering there is no subreaper
// set.
subreaper = 0
}
for _, p := range procs {
if s != unix.SIGKILL {
if ok, err := isWaitable(p.Pid); err != nil {
if !errors.Is(err, unix.ECHILD) {
logrus.Warn("signalAllProcesses: ", p.Pid, err)
}
continue
} else if !ok {
// Not ready to report so don't wait
continue
}
}
// In case a subreaper has been setup, this code must not
// wait for the process. Otherwise, we cannot be sure the
// current process will be reaped by the subreaper, while
// the subreaper might be waiting for this process in order
// to retrieve its exit code.
if subreaper == 0 {
if _, err := p.Wait(); err != nil {
if !errors.Is(err, unix.ECHILD) {
logrus.Warn("wait: ", err)
}
}
}
}
return nil
}
libcontainer/integration/exec_test.go
+7 -6
View File
@@ -1373,7 +1373,7 @@ func TestPIDHostInitProcessWait(t *testing.T) {
process1 := &libcontainer.Process{
Cwd: "/",
Args: []string{"sleep", "100"},
Args: []string{"sleep", "1h"},
Env: standardEnvironment,
Init: true,
}
@@ -1382,7 +1382,7 @@ func TestPIDHostInitProcessWait(t *testing.T) {
process2 := &libcontainer.Process{
Cwd: "/",
Args: []string{"sleep", "100"},
Args: []string{"sleep", "1h"},
Env: standardEnvironment,
Init: false,
}
@@ -1397,10 +1397,11 @@ func TestPIDHostInitProcessWait(t *testing.T) {
t.Fatal("expected Wait to indicate failure")
}
// The non-init process must've been killed.
err = process2.Signal(syscall.Signal(0))
if err == nil || err.Error() != "no such process" {
t.Fatalf("expected process to have been killed: %v", err)
// The non-init process must've also been killed. If not,
// the test will time out.
_, err = process2.Wait()
if err == nil {
t.Fatal("expected Wait to indicate failure")
}
}