diff --git a/runsc/boot/controller.go b/runsc/boot/controller.go index cb5d8ea31..5e849cb37 100644 --- a/runsc/boot/controller.go +++ b/runsc/boot/controller.go @@ -547,7 +547,8 @@ type SignalArgs struct { // Signo is the signal to send to the process. Signo int32 - // PID is the process ID in the given container that will be signaled. + // PID is the process ID in the given container that will be signaled, + // relative to the root PID namespace, not the container's. // If 0, the root container will be signalled. PID int32 diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index a02eb2ec5..5afce232d 100644 --- a/runsc/boot/loader.go +++ b/runsc/boot/loader.go @@ -1171,7 +1171,8 @@ func (f *sandboxNetstackCreator) CreateStack() (inet.Stack, error) { // signal sends a signal to one or more processes in a container. If PID is 0, // then the container init process is used. Depending on the SignalDeliveryMode // option, the signal may be sent directly to the indicated process, to all -// processes in the container, or to the foreground process group. +// processes in the container, or to the foreground process group. pid is +// relative to the root PID namespace, not the container's. func (l *Loader) signal(cid string, pid, signo int32, mode SignalDeliveryMode) error { if pid < 0 { return fmt.Errorf("PID (%d) must be positive", pid) @@ -1208,6 +1209,8 @@ func (l *Loader) signal(cid string, pid, signo int32, mode SignalDeliveryMode) e } } +// signalProcess sends signal to process in the given container. tgid is +// relative to the root PID namespace, not the container's. func (l *Loader) signalProcess(cid string, tgid kernel.ThreadID, signo int32) error { execTG, err := l.threadGroupFromID(execID{cid: cid, pid: tgid}) if err == nil { @@ -1216,18 +1219,14 @@ func (l *Loader) signalProcess(cid string, tgid kernel.ThreadID, signo int32) er } // The caller may be signaling a process not started directly via exec. - // In this case, find the process in the container's PID namespace and - // signal it. - initTG, err := l.threadGroupFromID(execID{cid: cid}) - if err != nil { - return fmt.Errorf("no thread group found: %v", err) - } - tg := initTG.PIDNamespace().ThreadGroupWithID(tgid) + // In this case, find the process and check that the process belongs to the + // container in question. + tg := l.k.RootPIDNamespace().ThreadGroupWithID(tgid) if tg == nil { return fmt.Errorf("no such process with PID %d", tgid) } if tg.Leader().ContainerID() != cid { - return fmt.Errorf("process %d is part of a different container: %q", tgid, tg.Leader().ContainerID()) + return fmt.Errorf("process %d belongs to a different container: %q", tgid, tg.Leader().ContainerID()) } return l.k.SendExternalSignalThreadGroup(tg, &arch.SignalInfo{Signo: signo}) } diff --git a/runsc/cmd/kill.go b/runsc/cmd/kill.go index aecf0b7ab..e0df39266 100644 --- a/runsc/cmd/kill.go +++ b/runsc/cmd/kill.go @@ -52,7 +52,7 @@ func (*Kill) Usage() string { // SetFlags implements subcommands.Command.SetFlags. func (k *Kill) SetFlags(f *flag.FlagSet) { f.BoolVar(&k.all, "all", false, "send the specified signal to all processes inside the container") - f.IntVar(&k.pid, "pid", 0, "send the specified signal to a specific process") + f.IntVar(&k.pid, "pid", 0, "send the specified signal to a specific process. pid is relative to the root PID namespace") } // Execute implements subcommands.Command.Execute. diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go index 129478505..862d9444d 100644 --- a/runsc/container/container_test.go +++ b/runsc/container/container_test.go @@ -112,6 +112,39 @@ func blockUntilWaitable(pid int) error { return err } +// execPS executes `ps` inside the container and return the processes. +func execPS(c *Container) ([]*control.Process, error) { + out, err := executeCombinedOutput(c, "/bin/ps", "-e") + if err != nil { + return nil, err + } + lines := strings.Split(string(out), "\n") + if len(lines) < 1 { + return nil, fmt.Errorf("missing header: %q", lines) + } + procs := make([]*control.Process, 0, len(lines)-1) + for _, line := range lines[1:] { + if len(line) == 0 { + continue + } + fields := strings.Fields(line) + if len(fields) != 4 { + return nil, fmt.Errorf("malformed line: %s", line) + } + pid, err := strconv.Atoi(fields[0]) + if err != nil { + return nil, err + } + cmd := fields[3] + // Fill only the fields we need thus far. + procs = append(procs, &control.Process{ + PID: kernel.ThreadID(pid), + Cmd: cmd, + }) + } + return procs, nil +} + // procListsEqual is used to check whether 2 Process lists are equal. Fields // set to -1 in wants are ignored. Timestamp and threads fields are always // ignored. @@ -1568,9 +1601,9 @@ func TestReadonlyRoot(t *testing.T) { } // Read mounts to check that root is readonly. - out, ws, err := executeCombinedOutput(c, "/bin/sh", "-c", "mount | grep ' / '") - if err != nil || ws != 0 { - t.Fatalf("exec failed, ws: %v, err: %v", ws, err) + out, err := executeCombinedOutput(c, "/bin/sh", "-c", "mount | grep ' / '") + if err != nil { + t.Fatalf("exec failed: %v", err) } t.Logf("root mount: %q", out) if !strings.Contains(string(out), "(ro)") { @@ -1578,7 +1611,7 @@ func TestReadonlyRoot(t *testing.T) { } // Check that file cannot be created. - ws, err = execute(c, "/bin/touch", "/foo") + ws, err := execute(c, "/bin/touch", "/foo") if err != nil { t.Fatalf("touch file in ro mount: %v", err) } @@ -1627,9 +1660,9 @@ func TestReadonlyMount(t *testing.T) { // Read mounts to check that volume is readonly. cmd := fmt.Sprintf("mount | grep ' %s '", dir) - out, ws, err := executeCombinedOutput(c, "/bin/sh", "-c", cmd) - if err != nil || ws != 0 { - t.Fatalf("exec failed, ws: %v, err: %v", ws, err) + out, err := executeCombinedOutput(c, "/bin/sh", "-c", cmd) + if err != nil { + t.Fatalf("exec failed, err: %v", err) } t.Logf("mount: %q", out) if !strings.Contains(string(out), "(ro)") { @@ -1637,7 +1670,7 @@ func TestReadonlyMount(t *testing.T) { } // Check that file cannot be created. - ws, err = execute(c, "/bin/touch", path.Join(dir, "file")) + ws, err := execute(c, "/bin/touch", path.Join(dir, "file")) if err != nil { t.Fatalf("touch file in ro mount: %v", err) } @@ -2424,10 +2457,10 @@ func execute(cont *Container, name string, arg ...string) (syscall.WaitStatus, e return cont.executeSync(args) } -func executeCombinedOutput(cont *Container, name string, arg ...string) ([]byte, syscall.WaitStatus, error) { +func executeCombinedOutput(cont *Container, name string, arg ...string) ([]byte, error) { r, w, err := os.Pipe() if err != nil { - return nil, 0, err + return nil, err } defer r.Close() @@ -2439,10 +2472,14 @@ func executeCombinedOutput(cont *Container, name string, arg ...string) ([]byte, ws, err := cont.executeSync(args) w.Close() if err != nil { - return nil, 0, err + return nil, err } + if ws != 0 { + return nil, fmt.Errorf("exec failed, status: %v", ws) + } + out, err := ioutil.ReadAll(r) - return out, ws, err + return out, err } // executeSync synchronously executes a new process. diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go index 173332cc2..17aef2121 100644 --- a/runsc/container/multi_container_test.go +++ b/runsc/container/multi_container_test.go @@ -166,8 +166,8 @@ func TestMultiContainerSanity(t *testing.T) { } } -// TestMultiPIDNS checks that it is possible to run 2 dead-simple -// containers in the same sandbox with different pidns. +// TestMultiPIDNS checks that it is possible to run 2 dead-simple containers in +// the same sandbox with different pidns. func TestMultiPIDNS(t *testing.T) { for name, conf := range configs(t, all...) { t.Run(name, func(t *testing.T) { @@ -208,6 +208,33 @@ func TestMultiPIDNS(t *testing.T) { if err := waitForProcessList(containers[1], expectedPL); err != nil { t.Errorf("failed to wait for sleep to start: %v", err) } + + // Root container runs in the root PID namespace and can see all + // processes. + expectedPL = []*control.Process{ + newProcessBuilder().PID(1).Cmd("sleep").Process(), + newProcessBuilder().PID(2).Cmd("sleep").Process(), + newProcessBuilder().Cmd("ps").Process(), + } + got, err := execPS(containers[0]) + if err != nil { + t.Fatal(err) + } + if !procListsEqual(got, expectedPL) { + t.Errorf("container got process list: %s, want: %s", procListToString(got), procListToString(expectedPL)) + } + + expectedPL = []*control.Process{ + newProcessBuilder().PID(1).Cmd("sleep").Process(), + newProcessBuilder().Cmd("ps").Process(), + } + got, err = execPS(containers[1]) + if err != nil { + t.Fatal(err) + } + if !procListsEqual(got, expectedPL) { + t.Errorf("container got process list: %s, want: %s", procListToString(got), procListToString(expectedPL)) + } }) } } @@ -274,6 +301,144 @@ func TestMultiPIDNSPath(t *testing.T) { if err := waitForProcessList(containers[1], expectedPL); err != nil { t.Errorf("failed to wait for sleep to start: %v", err) } + + // Root container runs in the root PID namespace and can see all + // processes. + expectedPL = []*control.Process{ + newProcessBuilder().PID(1).Cmd("sleep").Process(), + newProcessBuilder().PID(2).Cmd("sleep").Process(), + newProcessBuilder().PID(3).Cmd("sleep").Process(), + newProcessBuilder().Cmd("ps").Process(), + } + got, err := execPS(containers[0]) + if err != nil { + t.Fatal(err) + } + if !procListsEqual(got, expectedPL) { + t.Errorf("container got process list: %s, want: %s", procListToString(got), procListToString(expectedPL)) + } + + // Container 1 runs in the same PID namespace as the root container. + expectedPL = []*control.Process{ + newProcessBuilder().PID(1).Cmd("sleep").Process(), + newProcessBuilder().PID(2).Cmd("sleep").Process(), + newProcessBuilder().PID(3).Cmd("sleep").Process(), + newProcessBuilder().Cmd("ps").Process(), + } + got, err = execPS(containers[1]) + if err != nil { + t.Fatal(err) + } + if !procListsEqual(got, expectedPL) { + t.Errorf("container got process list: %s, want: %s", procListToString(got), procListToString(expectedPL)) + } + + // Container 2 runs on its own namespace. + expectedPL = []*control.Process{ + newProcessBuilder().PID(1).Cmd("sleep").Process(), + newProcessBuilder().Cmd("ps").Process(), + } + got, err = execPS(containers[2]) + if err != nil { + t.Fatal(err) + } + if !procListsEqual(got, expectedPL) { + t.Errorf("container got process list: %s, want: %s", procListToString(got), procListToString(expectedPL)) + } + }) + } +} + +// TestMultiPIDNSKill kills processes using PID when containers are using +// different PID namespaces to ensure PID is taken from the root namespace. +func TestMultiPIDNSKill(t *testing.T) { + app, err := testutil.FindFile("test/cmd/test_app/test_app") + if err != nil { + t.Fatal("error finding test_app:", err) + } + + for name, conf := range configs(t, all...) { + t.Run(name, func(t *testing.T) { + rootDir, cleanup, err := testutil.SetupRootDir() + if err != nil { + t.Fatalf("error creating root dir: %v", err) + } + defer cleanup() + conf.RootDir = rootDir + + // Setup the containers. + cmd := []string{app, "task-tree", "--depth=1", "--width=2", "--pause=true"} + const processes = 3 + testSpecs, ids := createSpecs(cmd, cmd) + + // TODO: Uncomment after https://github.com/google/gvisor/pull/5519. + //testSpecs[1].Linux = &specs.Linux{ + // Namespaces: []specs.LinuxNamespace{ + // { + // Type: "pid", + // }, + // }, + //} + + containers, cleanup, err := startContainers(conf, testSpecs, ids) + if err != nil { + t.Fatalf("error starting containers: %v", err) + } + defer cleanup() + + // Wait until all processes are created. + for _, c := range containers { + if err := waitForProcessCount(c, processes); err != nil { + t.Fatalf("error waitting for processes: %v", err) + } + } + + for i, c := range containers { + // First, kill a process that belongs to the container. + procs, err := c.Processes() + if err != nil { + t.Fatalf("container.Processes(): %v", err) + } + t.Logf("Container %q procs: %s", c.ID, procListToString(procs)) + pidToKill := procs[processes-1].PID + t.Logf("PID to kill: %d", pidToKill) + if err := c.SignalProcess(syscall.SIGKILL, int32(pidToKill)); err != nil { + t.Errorf("container.SignalProcess: %v", err) + } + // Wait for the process to get killed. + if err := waitForProcessCount(c, processes-1); err != nil { + t.Fatalf("error waitting for processes: %v", err) + } + procs, err = c.Processes() + if err != nil { + t.Fatalf("container.Processes(): %v", err) + } + t.Logf("Container %q procs after kill: %s", c.ID, procListToString(procs)) + for _, proc := range procs { + if proc.PID == pidToKill { + t.Errorf("process %d not killed: %+v", pidToKill, proc) + } + } + + // Next, attempt to kill a process from another container and check that + // it fails. + other := containers[(i+1)%len(containers)] + procs, err = other.Processes() + if err != nil { + t.Fatalf("container.Processes(): %v", err) + } + t.Logf("Other container %q procs: %s", other.ID, procListToString(procs)) + + pidToKill = procs[len(procs)-1].PID + t.Logf("PID that should not be killed: %d", pidToKill) + err = c.SignalProcess(syscall.SIGKILL, int32(pidToKill)) + if err == nil { + t.Fatalf("killing another container's process should fail") + } + if !strings.Contains(err.Error(), "belongs to a different container") { + t.Errorf("wrong error message from killing another container's: %v", err) + } + } }) } }