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

2 files changed, 216 insertions, 14 deletions

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