Browse code

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

Signed-off-by: Dmitry Shyshkin <dmitry@shyshkin.org.ua>

Dmitry Shyshkin authored on 2017/01/21 20:35:54
Showing 4 changed files
... ...
@@ -54,6 +54,7 @@ func GetHTTPErrorStatusCode(err error) int {
54 54
 			code    int
55 55
 		}{
56 56
 			{"not found", http.StatusNotFound},
57
+			{"cannot find", http.StatusNotFound},
57 58
 			{"no such", http.StatusNotFound},
58 59
 			{"bad parameter", http.StatusBadRequest},
59 60
 			{"no command", http.StatusBadRequest},
... ...
@@ -165,18 +165,25 @@ func (d *Daemon) ContainerExecStart(ctx context.Context, name string, stdin io.R
165 165
 		return fmt.Errorf("Error: Exec command %s is already running", ec.ID)
166 166
 	}
167 167
 	ec.Running = true
168
+	ec.Unlock()
169
+
170
+	c := d.containers.Get(ec.ContainerID)
171
+	logrus.Debugf("starting exec command %s in container %s", ec.ID, c.ID)
172
+	d.LogContainerEvent(c, "exec_start: "+ec.Entrypoint+" "+strings.Join(ec.Args, " "))
173
+
168 174
 	defer func() {
169 175
 		if err != nil {
176
+			ec.Lock()
170 177
 			ec.Running = false
171 178
 			exitCode := 126
172 179
 			ec.ExitCode = &exitCode
180
+			if err := ec.CloseStreams(); err != nil {
181
+				logrus.Errorf("failed to cleanup exec %s streams: %s", c.ID, err)
182
+			}
183
+			ec.Unlock()
184
+			c.ExecCommands.Delete(ec.ID)
173 185
 		}
174 186
 	}()
175
-	ec.Unlock()
176
-
177
-	c := d.containers.Get(ec.ContainerID)
178
-	logrus.Debugf("starting exec command %s in container %s", ec.ID, c.ID)
179
-	d.LogContainerEvent(c, "exec_start: "+ec.Entrypoint+" "+strings.Join(ec.Args, " "))
180 187
 
181 188
 	if ec.OpenStdin && stdin != nil {
182 189
 		r, w := io.Pipe()
... ...
@@ -90,7 +90,7 @@ func (daemon *Daemon) StateChanged(id string, e libcontainerd.StateInfo) error {
90 90
 			execConfig.Running = false
91 91
 			execConfig.StreamConfig.Wait()
92 92
 			if err := execConfig.CloseStreams(); err != nil {
93
-				logrus.Errorf("%s: %s", c.ID, err)
93
+				logrus.Errorf("failed to cleanup exec %s streams: %s", c.ID, err)
94 94
 			}
95 95
 
96 96
 			// remove the exec command from the container's store only and not the
... ...
@@ -120,21 +120,7 @@ func (s *DockerSuite) TestExecAPIStartMultipleTimesError(c *check.C) {
120 120
 	runSleepingContainer(c, "-d", "--name", "test")
121 121
 	execID := createExec(c, "test")
122 122
 	startExec(c, execID, http.StatusOK)
123
-
124
-	timeout := time.After(60 * time.Second)
125
-	var execJSON struct{ Running bool }
126
-	for {
127
-		select {
128
-		case <-timeout:
129
-			c.Fatal("timeout waiting for exec to start")
130
-		default:
131
-		}
132
-
133
-		inspectExec(c, execID, &execJSON)
134
-		if !execJSON.Running {
135
-			break
136
-		}
137
-	}
123
+	waitForExec(c, execID)
138 124
 
139 125
 	startExec(c, execID, http.StatusConflict)
140 126
 }
... ...
@@ -169,8 +155,43 @@ func (s *DockerSuite) TestExecAPIStartWithDetach(c *check.C) {
169 169
 	}
170 170
 }
171 171
 
172
+// #30311
173
+func (s *DockerSuite) TestExecAPIStartValidCommand(c *check.C) {
174
+	name := "exec_test"
175
+	dockerCmd(c, "run", "-d", "-t", "--name", name, "busybox", "/bin/sh")
176
+
177
+	id := createExecCmd(c, name, "true")
178
+	startExec(c, id, http.StatusOK)
179
+
180
+	waitForExec(c, id)
181
+
182
+	var inspectJSON struct{ ExecIDs []string }
183
+	inspectContainer(c, name, &inspectJSON)
184
+
185
+	c.Assert(inspectJSON.ExecIDs, checker.IsNil)
186
+}
187
+
188
+// #30311
189
+func (s *DockerSuite) TestExecAPIStartInvalidCommand(c *check.C) {
190
+	name := "exec_test"
191
+	dockerCmd(c, "run", "-d", "-t", "--name", name, "busybox", "/bin/sh")
192
+
193
+	id := createExecCmd(c, name, "invalid")
194
+	startExec(c, id, http.StatusNotFound)
195
+	waitForExec(c, id)
196
+
197
+	var inspectJSON struct{ ExecIDs []string }
198
+	inspectContainer(c, name, &inspectJSON)
199
+
200
+	c.Assert(inspectJSON.ExecIDs, checker.IsNil)
201
+}
202
+
172 203
 func createExec(c *check.C, name string) string {
173
-	_, b, err := request.SockRequest("POST", fmt.Sprintf("/containers/%s/exec", name), map[string]interface{}{"Cmd": []string{"true"}}, daemonHost())
204
+	return createExecCmd(c, name, "true")
205
+}
206
+
207
+func createExecCmd(c *check.C, name string, cmd string) string {
208
+	_, b, err := request.SockRequest("POST", fmt.Sprintf("/containers/%s/exec", name), map[string]interface{}{"Cmd": []string{cmd}}, daemonHost())
174 209
 	c.Assert(err, checker.IsNil, check.Commentf(string(b)))
175 210
 
176 211
 	createResp := struct {
... ...
@@ -198,3 +219,29 @@ func inspectExec(c *check.C, id string, out interface{}) {
198 198
 	err = json.NewDecoder(body).Decode(out)
199 199
 	c.Assert(err, checker.IsNil)
200 200
 }
201
+
202
+func waitForExec(c *check.C, id string) {
203
+	timeout := time.After(60 * time.Second)
204
+	var execJSON struct{ Running bool }
205
+	for {
206
+		select {
207
+		case <-timeout:
208
+			c.Fatal("timeout waiting for exec to start")
209
+		default:
210
+		}
211
+
212
+		inspectExec(c, id, &execJSON)
213
+		if !execJSON.Running {
214
+			break
215
+		}
216
+	}
217
+}
218
+
219
+func inspectContainer(c *check.C, id string, out interface{}) {
220
+	resp, body, err := request.SockRequestRaw("GET", fmt.Sprintf("/containers/%s/json", id), nil, "", daemonHost())
221
+	c.Assert(err, checker.IsNil)
222
+	defer body.Close()
223
+	c.Assert(resp.StatusCode, checker.Equals, http.StatusOK)
224
+	err = json.NewDecoder(body).Decode(out)
225
+	c.Assert(err, checker.IsNil)
226
+}