Merge pull request #30340 from ijrandom/master

Fix #30311: dockerd leaks ExecIds on failed exec -i

api/server/httputils/errors.go

@@ -54,6 +54,7 @@ func GetHTTPErrorStatusCode(err error) int {
 			code    int
 		}{
 			{"not found", http.StatusNotFound},
+			{"cannot find", http.StatusNotFound},
 			{"no such", http.StatusNotFound},
 			{"bad parameter", http.StatusBadRequest},
 			{"no command", http.StatusBadRequest},

daemon/exec.go

@@ -165,19 +165,26 @@ func (d *Daemon) ContainerExecStart(ctx context.Context, name string, stdin io.R
 		return fmt.Errorf("Error: Exec command %s is already running", ec.ID)
 	}
 	ec.Running = true
-	defer func() {
-		if err != nil {
-			ec.Running = false
-			exitCode := 126
-			ec.ExitCode = &exitCode
-		}
-	}()
 	ec.Unlock()
 
 	c := d.containers.Get(ec.ContainerID)
 	logrus.Debugf("starting exec command %s in container %s", ec.ID, c.ID)
 	d.LogContainerEvent(c, "exec_start: "+ec.Entrypoint+" "+strings.Join(ec.Args, " "))
 
+	defer func() {
+		if err != nil {
+			ec.Lock()
+			ec.Running = false
+			exitCode := 126
+			ec.ExitCode = &exitCode
+			if err := ec.CloseStreams(); err != nil {
+				logrus.Errorf("failed to cleanup exec %s streams: %s", c.ID, err)
+			}
+			ec.Unlock()
+			c.ExecCommands.Delete(ec.ID)
+		}
+	}()
+
 	if ec.OpenStdin && stdin != nil {
 		r, w := io.Pipe()
 		go func() {

daemon/monitor.go

@@ -90,7 +90,7 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error {
 			execConfig.Running = false
 			execConfig.StreamConfig.Wait()
 			if err := execConfig.CloseStreams(); err != nil {
-				logrus.Errorf("%s: %s", c.ID, err)
+				logrus.Errorf("failed to cleanup exec %s streams: %s", c.ID, err)
 			}
 
 			// remove the exec command from the container's store only and not the

integration-cli/docker_api_exec_test.go

@@ -120,21 +120,7 @@ func (s *DockerSuite) TestExecAPIStartMultipleTimesError(c *check.C) {
 	runSleepingContainer(c, "-d", "--name", "test")
 	execID := createExec(c, "test")
 	startExec(c, execID, http.StatusOK)
-
-	timeout := time.After(60 * time.Second)
-	var execJSON struct{ Running bool }
-	for {
-		select {
-		case <-timeout:
-			c.Fatal("timeout waiting for exec to start")
-		default:
-		}
-
-		inspectExec(c, execID, &execJSON)
-		if !execJSON.Running {
-			break
-		}
-	}
+	waitForExec(c, execID)
 
 	startExec(c, execID, http.StatusConflict)
 }
@@ -169,8 +155,43 @@ func (s *DockerSuite) TestExecAPIStartWithDetach(c *check.C) {
 	}
 }
 
+// #30311
+func (s *DockerSuite) TestExecAPIStartValidCommand(c *check.C) {
+	name := "exec_test"
+	dockerCmd(c, "run", "-d", "-t", "--name", name, "busybox", "/bin/sh")
+
+	id := createExecCmd(c, name, "true")
+	startExec(c, id, http.StatusOK)
+
+	waitForExec(c, id)
+
+	var inspectJSON struct{ ExecIDs []string }
+	inspectContainer(c, name, &inspectJSON)
+
+	c.Assert(inspectJSON.ExecIDs, checker.IsNil)
+}
+
+// #30311
+func (s *DockerSuite) TestExecAPIStartInvalidCommand(c *check.C) {
+	name := "exec_test"
+	dockerCmd(c, "run", "-d", "-t", "--name", name, "busybox", "/bin/sh")
+
+	id := createExecCmd(c, name, "invalid")
+	startExec(c, id, http.StatusNotFound)
+	waitForExec(c, id)
+
+	var inspectJSON struct{ ExecIDs []string }
+	inspectContainer(c, name, &inspectJSON)
+
+	c.Assert(inspectJSON.ExecIDs, checker.IsNil)
+}
+
 func createExec(c *check.C, name string) string {
-	_, b, err := request.SockRequest("POST", fmt.Sprintf("/containers/%s/exec", name), map[string]interface{}{"Cmd": []string{"true"}}, daemonHost())
+	return createExecCmd(c, name, "true")
+}
+
+func createExecCmd(c *check.C, name string, cmd string) string {
+	_, b, err := request.SockRequest("POST", fmt.Sprintf("/containers/%s/exec", name), map[string]interface{}{"Cmd": []string{cmd}}, daemonHost())
 	c.Assert(err, checker.IsNil, check.Commentf(string(b)))
 
 	createResp := struct {
@@ -198,3 +219,29 @@ func inspectExec(c *check.C, id string, out interface{}) {
 	err = json.NewDecoder(body).Decode(out)
 	c.Assert(err, checker.IsNil)
 }
+
+func waitForExec(c *check.C, id string) {
+	timeout := time.After(60 * time.Second)
+	var execJSON struct{ Running bool }
+	for {
+		select {
+		case <-timeout:
+			c.Fatal("timeout waiting for exec to start")
+		default:
+		}
+
+		inspectExec(c, id, &execJSON)
+		if !execJSON.Running {
+			break
+		}
+	}
+}
+
+func inspectContainer(c *check.C, id string, out interface{}) {
+	resp, body, err := request.SockRequestRaw("GET", fmt.Sprintf("/containers/%s/json", id), nil, "", daemonHost())
+	c.Assert(err, checker.IsNil)
+	defer body.Close()
+	c.Assert(resp.StatusCode, checker.Equals, http.StatusOK)
+	err = json.NewDecoder(body).Decode(out)
+	c.Assert(err, checker.IsNil)
+}