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

5 files changed, 227 insertions, 25 deletions

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