Merge pull request #320 from tonistiigi/clean-exec-fifos

[17.09] libcontainerd: fix leaking container/exec state

components/engine/integration-cli/docker_api_exec_test.go

@@ -8,6 +8,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"os"
+	"strings"
 	"time"
 
 	"github.com/docker/docker/api/types"
@@ -198,6 +200,45 @@ func (s *DockerSuite) TestExecAPIStartInvalidCommand(c *check.C) {
 	c.Assert(inspectJSON.ExecIDs, checker.IsNil)
 }
 
+func (s *DockerSuite) TestExecStateCleanup(c *check.C) {
+	testRequires(c, DaemonIsLinux, SameHostDaemon)
+
+	// This test checks accidental regressions. Not part of stable API.
+
+	name := "exec_cleanup"
+	cid, _ := dockerCmd(c, "run", "-d", "-t", "--name", name, "busybox", "/bin/sh")
+	cid = strings.TrimSpace(cid)
+
+	stateDir := "/var/run/docker/libcontainerd/" + cid
+
+	checkReadDir := func(c *check.C) (interface{}, check.CommentInterface) {
+		fi, err := ioutil.ReadDir(stateDir)
+		c.Assert(err, checker.IsNil)
+		return len(fi), nil
+	}
+
+	fi, err := ioutil.ReadDir(stateDir)
+	c.Assert(err, checker.IsNil)
+	c.Assert(len(fi), checker.GreaterThan, 1)
+
+	id := createExecCmd(c, name, "ls")
+	startExec(c, id, http.StatusOK)
+	waitForExec(c, id)
+
+	waitAndAssert(c, 5*time.Second, checkReadDir, checker.Equals, len(fi))
+
+	id = createExecCmd(c, name, "invalid")
+	startExec(c, id, http.StatusBadRequest)
+	waitForExec(c, id)
+
+	waitAndAssert(c, 5*time.Second, checkReadDir, checker.Equals, len(fi))
+
+	dockerCmd(c, "stop", name)
+	_, err = os.Stat(stateDir)
+	c.Assert(err, checker.NotNil)
+	c.Assert(os.IsNotExist(err), checker.True)
+}
+
 func createExec(c *check.C, name string) string {
 	return createExecCmd(c, name, "true")
 }

components/engine/libcontainerd/client_linux.go

@@ -117,6 +117,13 @@ func (clnt *client) AddProcess(ctx context.Context, containerID, processFriendly
 		return -1, err
 	}
 
+	// clean up fifos if failed to add process
+	defer func() {
+		if err != nil {
+			p.cleanFifos(processFriendlyName)
+		}
+	}()
+
 	resp, err := clnt.remote.apiClient.AddProcess(ctx, r)
 	if err != nil {
 		p.closeFifos(iopipe)

components/engine/libcontainerd/container_unix.go

@@ -69,11 +69,7 @@ func (ctr *container) clean() error {
 // Caller needs to lock container ID before calling this method.
 func (ctr *container) cleanProcess(id string) {
 	if p, ok := ctr.processes[id]; ok {
-		for _, i := range []int{unix.Stdin, unix.Stdout, unix.Stderr} {
-			if err := os.Remove(p.fifo(i)); err != nil && !os.IsNotExist(err) {
-				logrus.Warnf("libcontainerd: failed to remove %v for process %v: %v", p.fifo(i), id, err)
-			}
-		}
+		p.cleanFifos(id)
 	}
 	delete(ctr.processes, id)
 }

components/engine/libcontainerd/process_unix.go

@@ -9,8 +9,10 @@ import (
 	"path/filepath"
 	goruntime "runtime"
 	"strings"
+	"syscall"
 
 	containerd "github.com/containerd/containerd/api/grpc/types"
+	"github.com/sirupsen/logrus"
 	"github.com/tonistiigi/fifo"
 	"golang.org/x/net/context"
 	"golang.org/x/sys/unix"
@@ -105,3 +107,12 @@ func (r emptyReader) Read(b []byte) (int, error) {
 func (p *process) fifo(index int) string {
 	return filepath.Join(p.dir, p.friendlyName+"-"+fdNames[index])
 }
+
+func (p *process) cleanFifos(id string) {
+	for _, i := range []int{syscall.Stdin, syscall.Stdout, syscall.Stderr} {
+		if err := os.Remove(p.fifo(i)); err != nil && !os.IsNotExist(err) {
+			logrus.Warnf("failed to remove %v for process %v: %v", p.fifo(i), id, err)
+		}
+	}
+
+}