Fix #30311: dockerd leaks ExecIds on failed exec -i by ijrandom · Pull Request #30340 · moby/moby

ijrandom · 2017-01-21T11:44:26Z

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

execute docker exec -i anycontainer invalidcommand
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?

AkihiroSuda · 2017-01-22T10:26:50Z

daemon/exec.go

Probably no need to move these lines?

It's necessary to use c in defer

ijrandom · 2017-01-22T16:06:38Z

added tests

ijrandom · 2017-01-22T19:20:36Z

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

justincormack · 2017-01-22T19:22:22Z

Should be a 4xx error.

…

On 22 Jan 2017 19:20, "ijrandom" ***@***.***> wrote: On windows docker exec invalidcommand produce 500 and on linux 404. Which one need to be corrected? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#30340 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdcPKvRatzY5wPrHW17uiZV0gJ-wZz0ks5rU6wLgaJpZM4LqC8B> .

LK4D4

LGTM

LK4D4 · 2017-01-30T21:29:08Z

ping @docker/core-engine-maintainers

dnephin · 2017-01-30T21:33:59Z

api/server/httputils/errors.go

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

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

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?

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

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

It come from the windows os when CreateProcess failed

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.

on linux message contains "not found" those producing 404

mlaventure · 2017-01-30T21:36:16Z

daemon/exec.go

Doesn't this needs to be protected by the lock?

Only if changes to ec.Running and ec.ExitCode need to be atomic. There were no lock protection in containerd callback - so I added no lock here. Should I add the lock? Add the lock in containerd callback as well?

Sorry. There was a lock. Added lock/unlock and changed message

mlaventure · 2017-01-30T21:36:57Z

daemon/exec.go

maybe add some context? e.g. "failed to cleanup exec %s streams: %s"?

done, also changed message in monitor.go

mlaventure

LGTM

LK4D4 · 2017-02-03T19:37:31Z

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

coolljt0725 · 2017-02-04T07:14:55Z

integration-cli/docker_api_exec_test.go

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

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

ijrandom · 2017-02-11T06:43:57Z

rebased and squashed

vdemeester

LGTM 🐸

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>

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

GordonTheTurtle added the status/0-triage label Jan 21, 2017

AkihiroSuda reviewed Jan 22, 2017

View reviewed changes

AkihiroSuda added status/2-code-review and removed status/0-triage labels Jan 22, 2017

LK4D4 approved these changes Jan 30, 2017

View reviewed changes

dnephin reviewed Jan 30, 2017

View reviewed changes

mlaventure suggested changes Jan 30, 2017

View reviewed changes

mlaventure approved these changes Jan 31, 2017

View reviewed changes

coolljt0725 reviewed Feb 4, 2017

View reviewed changes

GordonTheTurtle assigned cpuguy83 Feb 10, 2017

cpuguy83 requested changes Feb 10, 2017

View reviewed changes

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

3cc0d6b

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

ijrandom force-pushed the master branch from 6202edb to 3cc0d6b Compare February 10, 2017 19:16

vdemeester approved these changes Feb 13, 2017

View reviewed changes

vdemeester added status/4-merge and removed status/2-code-review labels Feb 13, 2017

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

GordonTheTurtle added this to the 1.14.0 milestone Feb 14, 2017

thaJeztah added the impact/changelog label Feb 20, 2017

Conversation

ijrandom commented Jan 21, 2017

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijrandom commented Jan 22, 2017

Uh oh!

ijrandom commented Jan 22, 2017

Uh oh!

justincormack commented Jan 22, 2017 via email

Uh oh!

LK4D4 left a comment

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

ijrandom commented Feb 11, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants