New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Feb 14, 2017

Conversation

@ijrandom
Contributor

ijrandom commented Jan 21, 2017

If libcontainerd failed to start process there will be no callback about "exec done" so cleanup code should be added to ContainerExecStart
Signed-off-by: Dmitry Shyshkin dmitry@shyshkin.org.ua

- What I did
Close streams and removed ExecConfig from container in case of process failed to start
- How I did it
Copy part of cleanup code from containerd exec callback
- How to verify it

  1. execute docker exec -i anycontainer invalidcommand
  2. check for ExecIDs. docker inspect anycontainer | grep -A 10 ExecIDs

- Description for the changelog
Fix #30311: dockerd leaks ExecIds on failed exec -i

@AkihiroSuda

Can you please add an automated test?

Show outdated Hide outdated daemon/exec.go Outdated
@ijrandom

This comment has been minimized.

Show comment
Hide comment
@ijrandom

ijrandom Jan 22, 2017

Contributor

added tests

Contributor

ijrandom commented Jan 22, 2017

added tests

@ijrandom

This comment has been minimized.

Show comment
Hide comment
@ijrandom

ijrandom Jan 22, 2017

Contributor

On windows docker exec invalidcommand produce 500 and on linux 404. Which one need to be corrected?

Contributor

ijrandom commented Jan 22, 2017

On windows docker exec invalidcommand produce 500 and on linux 404. Which one need to be corrected?

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Jan 22, 2017

Contributor
Contributor

justincormack commented Jan 22, 2017

@LK4D4

LK4D4 approved these changes Jan 30, 2017

LGTM

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

ping @docker/core-engine-maintainers

Contributor

LK4D4 commented Jan 30, 2017

ping @docker/core-engine-maintainers

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

This comment has been minimized.

@dnephin

dnephin Jan 30, 2017

Member

Can we avoid expanding this "guess the status code" system further and return a specific http error instead?

@dnephin

dnephin Jan 30, 2017

Member

Can we avoid expanding this "guess the status code" system further and return a specific http error instead?

This comment has been minimized.

@LK4D4

LK4D4 Jan 31, 2017

Contributor

@dnephin can you explain a little bit more for @ijrandom ?

@LK4D4

LK4D4 Jan 31, 2017

Contributor

@dnephin can you explain a little bit more for @ijrandom ?

This comment has been minimized.

@dnephin

dnephin Jan 31, 2017

Member

Sure, I'd be happy to.

This code that you've added to is trying to guess which http status code to return based on an error message string. Instead of guessing based on string matching, we'd like to be explicit about the error.

The explicit error is in api/errors/errors.go NewRequestNotFoundError(). Could you return that directly from the function that causes this error, instead of adding a new string to this list?

@dnephin

dnephin Jan 31, 2017

Member

Sure, I'd be happy to.

This code that you've added to is trying to guess which http status code to return based on an error message string. Instead of guessing based on string matching, we'd like to be explicit about the error.

The explicit error is in api/errors/errors.go NewRequestNotFoundError(). Could you return that directly from the function that causes this error, instead of adding a new string to this list?

This comment has been minimized.

@ijrandom

ijrandom Feb 9, 2017

Contributor

Just to be clear. You want to return NewRequestNotFoundError from platform specific wrapper to execv/CreateProcess?

@ijrandom

ijrandom Feb 9, 2017

Contributor

Just to be clear. You want to return NewRequestNotFoundError from platform specific wrapper to execv/CreateProcess?

This comment has been minimized.

@dnephin

dnephin Feb 9, 2017

Member

Where is the error which contains the text "cannot find"?

@dnephin

dnephin Feb 9, 2017

Member

Where is the error which contains the text "cannot find"?

This comment has been minimized.

@ijrandom

ijrandom Feb 10, 2017

Contributor

It come from the windows os when CreateProcess failed

@ijrandom

ijrandom Feb 10, 2017

Contributor

It come from the windows os when CreateProcess failed

This comment has been minimized.

@dnephin

dnephin Feb 10, 2017

Member

Ok, is there an issue about this? I don't think we return a 404 on other platforms when the path to a binary is incorrect.

@dnephin

dnephin Feb 10, 2017

Member

Ok, is there an issue about this? I don't think we return a 404 on other platforms when the path to a binary is incorrect.

This comment has been minimized.

@ijrandom

ijrandom Feb 10, 2017

Contributor

on linux message contains "not found" those producing 404

@ijrandom

ijrandom Feb 10, 2017

Contributor

on linux message contains "not found" those producing 404

Show outdated Hide outdated daemon/exec.go Outdated
Show outdated Hide outdated daemon/exec.go Outdated
@mlaventure

LGTM

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Feb 3, 2017

Contributor

@ijrandom would you mind to fix @dnephin suggestion? Thanks!

Contributor

LK4D4 commented Feb 3, 2017

@ijrandom would you mind to fix @dnephin suggestion? Thanks!

}
}
func inspectContainer(c *check.C, id string, out interface{}) {

This comment has been minimized.

@coolljt0725

coolljt0725 Feb 4, 2017

Contributor

you can use inspectFieldJSON (https://github.com/docker/docker/blob/master/integration-cli/docker_utils_test.go#L554) instead.

The fix LGTM.

It's better to squash your commits :)

@coolljt0725

coolljt0725 Feb 4, 2017

Contributor

you can use inspectFieldJSON (https://github.com/docker/docker/blob/master/integration-cli/docker_utils_test.go#L554) instead.

The fix LGTM.

It's better to squash your commits :)

@cpuguy83

Let's makes sure to squash to 1 commit

Fix #303111: dockerd leaks ExecIds on failed exec -i
Signed-off-by: Dmitry Shyshkin <dmitry@shyshkin.org.ua>
@ijrandom

This comment has been minimized.

Show comment
Hide comment
@ijrandom

ijrandom Feb 11, 2017

Contributor

rebased and squashed

Contributor

ijrandom commented Feb 11, 2017

rebased and squashed

@vdemeester

LGTM 🐸

@AkihiroSuda AkihiroSuda merged commit eac68db into moby:master Feb 14, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30649 has succeeded
Details
janky Jenkins build Docker-PRs 39263 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10322 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 14, 2017

@thaJeztah thaJeztah added this to PRs in 17.03.2-maybe Feb 14, 2017

@thaJeztah thaJeztah removed this from PRs in 17.03.2-maybe Feb 18, 2017

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Fix docker leaks ExecIds on failed exec -i
upstream issue: moby#30311
Cherry-pick from moby#30340
fix moby#161

Signed-off-by: Dmitry Shyshkin <dmitry@shyshkin.org.ua>
Signed-off-by: Lei Jitang <leijitang@huawei.com>

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

Merge branch 'fix_exec_ict' into 'IT-1.11.2'
Fix docker leaks ExecIds on failed exec -i

upstream issue: moby#30311
Cherry-pick from moby#30340
fix moby#161

Signed-off-by: Dmitry Shyshkin <dmitry@shyshkin.org.ua>
Signed-off-by: Lei Jitang <leijitang@huawei.com>


See merge request docker/docker!275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment