Fix race between start and destroy

Before this change, a container starting up could race with
destroy (aka delete) and leave processes behind.

Now, whenever a container is created, Loader.processes gets
a new entry. Start now expects the entry to be there, and if
it's not it means that the container was deleted.

I've also fixed Loader.waitPID to search for the process using
the init process's PID namespace.

We could use a few more tests for signal and wait. I'll send
them in another cl.

PiperOrigin-RevId: 220224290
Change-Id: I15146079f69904dc07d43c3b66cc343a2dab4cc4

3 files changed, 186 insertions, 1 deletions

diff --git a/runsc/container/container.go b/runsc/container/container.go
index 4c542ccb9..11c440f09 100644
--- a/runsc/container/container.go
+++ b/runsc/container/container.go
@@ -321,6 +321,9 @@ func Create(id string, spec *specs.Spec, conf *boot.Config, bundleDir, consoleSo
 			return nil, err
 		}
 		c.Sandbox = sb.Sandbox
+		if err := c.Sandbox.CreateContainer(c.ID); err != nil {
+			return nil, err
+		}
 	}
 	c.changeStatus(Created)
 
@@ -383,7 +386,7 @@ func (c *Container) Start(conf *boot.Config) error {
 		if err != nil {
 			return err
 		}
-		if err := c.Sandbox.Start(c.Spec, conf, c.ID, ioFiles); err != nil {
+		if err := c.Sandbox.StartContainer(c.Spec, conf, c.ID, ioFiles); err != nil {
 			return err
 		}
 		if err := c.Sandbox.AddGoferToCgroup(c.GoferPid); err != nil {
diff --git a/runsc/container/container_test.go b/runsc/container/container_test.go
index 243528d35..64def7eed 100644
--- a/runsc/container/container_test.go
+++ b/runsc/container/container_test.go
@@ -1625,6 +1625,70 @@ func TestWaitOnExitedSandbox(t *testing.T) {
 	}
 }
 
+func TestDestroyNotStarted(t *testing.T) {
+	spec := testutil.NewSpecWithArgs("/bin/sleep", "100")
+	conf := testutil.TestConfig()
+	rootDir, bundleDir, err := testutil.SetupContainer(spec, conf)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer os.RemoveAll(rootDir)
+	defer os.RemoveAll(bundleDir)
+
+	// Create the container and check that it can be destroyed.
+	c, err := Create(testutil.UniqueContainerID(), spec, conf, bundleDir, "", "", "")
+	if err != nil {
+		t.Fatalf("error creating container: %v", err)
+	}
+	if err := c.Destroy(); err != nil {
+		t.Fatalf("deleting non-started container failed: %v", err)
+	}
+}
+
+// TestDestroyStarting attempts to force a race between start and destroy.
+func TestDestroyStarting(t *testing.T) {
+	for i := 0; i < 10; i++ {
+		spec := testutil.NewSpecWithArgs("/bin/sleep", "100")
+		conf := testutil.TestConfig()
+		rootDir, bundleDir, err := testutil.SetupContainer(spec, conf)
+		if err != nil {
+			t.Fatalf("error setting up container: %v", err)
+		}
+		defer os.RemoveAll(rootDir)
+		defer os.RemoveAll(bundleDir)
+
+		// Create the container and check that it can be destroyed.
+		id := testutil.UniqueContainerID()
+		c, err := Create(id, spec, conf, bundleDir, "", "", "")
+		if err != nil {
+			t.Fatalf("error creating container: %v", err)
+		}
+
+		// Container is not thread safe, so load another instance to run in
+		// concurrently.
+		startCont, err := Load(rootDir, id)
+		if err != nil {
+			t.Fatalf("error loading container: %v", err)
+		}
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			// Ignore failures, start can fail if destroy runs first.
+			startCont.Start(conf)
+		}()
+
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if err := c.Destroy(); err != nil {
+				t.Errorf("deleting non-started container failed: %v", err)
+			}
+		}()
+		wg.Wait()
+	}
+}
+
 // executeSync synchronously executes a new process.
 func (cont *Container) executeSync(args *control.ExecArgs) (syscall.WaitStatus, error) {
 	pid, err := cont.Execute(args)
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 4548eb106..8af3d535d 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -612,3 +612,121 @@ func TestMultiContainerKillAll(t *testing.T) {
 		}
 	}
 }
+
+func TestMultiContainerDestroyNotStarted(t *testing.T) {
+	specs, ids := createSpecs(
+		[]string{"/bin/sleep", "100"},
+		[]string{"/bin/sleep", "100"})
+	conf := testutil.TestConfig()
+
+	rootDir, err := testutil.SetupRootDir()
+	if err != nil {
+		t.Fatalf("error creating root dir: %v", err)
+	}
+	defer os.RemoveAll(rootDir)
+
+	// Create and start root container.
+	rootBundleDir, err := testutil.SetupContainerInRoot(rootDir, specs[0], conf)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer os.RemoveAll(rootBundleDir)
+
+	root, err := Create(ids[0], specs[0], conf, rootBundleDir, "", "", "")
+	if err != nil {
+		t.Fatalf("error creating root container: %v", err)
+	}
+	defer root.Destroy()
+	if err := root.Start(conf); err != nil {
+		t.Fatalf("error starting root container: %v", err)
+	}
+
+	// Create and destroy sub-container.
+	bundleDir, err := testutil.SetupContainerInRoot(rootDir, specs[1], conf)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer os.RemoveAll(bundleDir)
+
+	cont, err := Create(ids[1], specs[1], conf, bundleDir, "", "", "")
+	if err != nil {
+		t.Fatalf("error creating container: %v", err)
+	}
+
+	// Check that container can be destroyed.
+	if err := cont.Destroy(); err != nil {
+		t.Fatalf("deleting non-started container failed: %v", err)
+	}
+}
+
+// TestMultiContainerDestroyStarting attempts to force a race between start
+// and destroy.
+func TestMultiContainerDestroyStarting(t *testing.T) {
+	cmds := make([][]string, 10)
+	for i := range cmds {
+		cmds[i] = []string{"/bin/sleep", "100"}
+	}
+	specs, ids := createSpecs(cmds...)
+	conf := testutil.TestConfig()
+
+	rootDir, err := testutil.SetupRootDir()
+	if err != nil {
+		t.Fatalf("error creating root dir: %v", err)
+	}
+	defer os.RemoveAll(rootDir)
+
+	// Create and start root container.
+	rootBundleDir, err := testutil.SetupContainerInRoot(rootDir, specs[0], conf)
+	if err != nil {
+		t.Fatalf("error setting up container: %v", err)
+	}
+	defer os.RemoveAll(rootBundleDir)
+
+	root, err := Create(ids[0], specs[0], conf, rootBundleDir, "", "", "")
+	if err != nil {
+		t.Fatalf("error creating root container: %v", err)
+	}
+	defer root.Destroy()
+	if err := root.Start(conf); err != nil {
+		t.Fatalf("error starting root container: %v", err)
+	}
+
+	wg := sync.WaitGroup{}
+	for i := range cmds {
+		if i == 0 {
+			continue // skip root container
+		}
+
+		bundleDir, err := testutil.SetupContainerInRoot(rootDir, specs[i], conf)
+		if err != nil {
+			t.Fatalf("error setting up container: %v", err)
+		}
+		defer os.RemoveAll(bundleDir)
+
+		cont, err := Create(ids[i], specs[i], conf, bundleDir, "", "", "")
+		if err != nil {
+			t.Fatalf("error creating container: %v", err)
+		}
+
+		// Container is not thread safe, so load another instance to run in
+		// concurrently.
+		startCont, err := Load(rootDir, ids[i])
+		if err != nil {
+			t.Fatalf("error loading container: %v", err)
+		}
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			startCont.Start(conf) // ignore failures, start can fail if destroy runs first.
+		}()
+
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if err := cont.Destroy(); err != nil {
+				t.Errorf("deleting non-started container failed: %v", err)
+			}
+		}()
+	}
+	wg.Wait()
+}