go
/
gvisor
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Fix `runsc kill --pid` 

Previously, loader.signalProcess was inconsitently using both root and
container's PID namespace to find the process. It used root namespace
for the exec'd process and container's PID namespace for other processes.
This fixes the code to use the root PID namespace across the board, which
is the same PID reported in `runsc ps` (or soon will after
https://github.com/google/gvisor/pull/5519).

PiperOrigin-RevId: 358836297
This commit is contained in:
Fabricio Voznika 2021-02-22 09:31:32 -08:00 committed by gVisor bot
parent 93fc09248a
commit 19fe3a2bfb
5 changed files with 227 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
runsc/boot/controller.go
View File

@ -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

17
runsc/boot/loader.go
View File

@ -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})
}

2
runsc/cmd/kill.go
View File

@ -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.

61
runsc/container/container_test.go
View File

@ -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.

169
runsc/container/multi_container_test.go
View File

@ -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)
}
}
})
}
}