Fix race condition in legacy process stdio code#737
Merged
kevpar merged 1 commit intomicrosoft:masterfrom Nov 15, 2019
Merged
Conversation
jstarks
reviewed
Nov 14, 2019
jstarks
reviewed
Nov 14, 2019
kevpar
force-pushed
the
fix-legacy-stdio
branch
3 times, most recently
from
cd42497 to
b432e59
Compare
jstarks
reviewed
Nov 14, 2019
jstarks
approved these changes
Nov 14, 2019
Member
jstarks
left a comment
jstarks
left a comment
There was a problem hiding this comment.
Looks good with some suggestions.
kevpar
force-pushed
the
fix-legacy-stdio
branch
from
b432e59 to
2371af8
Compare
HcsCreateProcess returns a set of stdio handles for the newly created process. A while ago, we used to cache these handles and return them the first time stdio handles were requested for the process, and then get new handles via HcsGetProcessInfo for each subsequent request. At some point, this code was cleaned up to instead always return the original set of handles as non-closable (for new callers) and always get new handles via HcsGetProcessInfo (for legacy callers, who required closable handles). However, this change introduced a race condition for legacy callers, where in the case of a short lived container process, the container could have terminated between when it was started and when the orchestrator requested stdio handles. This led to ERROR_NOT_FOUND being returned from HcsGetProcessInfo. This change addresses this by returning the original handles the first time stdio handles are requested, and then calling HcsGetProcessInfo for every subsequent request (just as it used to work a while ago). Signed-off-by: Kevin Parsons <[email protected]>
kevpar
force-pushed
the
fix-legacy-stdio
branch
from
2371af8 to
b3f49c0
Compare
1 task
vikramhh
pushed a commit
to vikramhh/moby
that referenced
this pull request
Nov 25, 2019
Among other things, this is required to pull in microsoft/hcsshim#718 which should take care of multiple issues reported to us. Also fixes microsoft/hcsshim#737 which was caught by checks while attempting to bump up hcsshim version. Modifies TestRunAttachFailedNoLeak to do case-insensitive comparison to fix a failure on RS1. Signed-off-by: Vikram bir Singh <[email protected]>
vikramhh
pushed a commit
to vikramhh/moby
that referenced
this pull request
Nov 25, 2019
Among other things, this is required to pull in microsoft/hcsshim#718 Also fixes microsoft/hcsshim#737 which was caught by checks while attempting to bump up hcsshim version. Signed-off-by: Vikram bir Singh <[email protected]>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Nov 26, 2019
Among other things, this is required to pull in microsoft/hcsshim#718 Also fixes microsoft/hcsshim#737 which was caught by checks while attempting to bump up hcsshim version. Signed-off-by: Vikram bir Singh <[email protected]> Upstream-commit: a7b6c3f0bf5d10c6227a29bac7dd46b9a7a779bc Component: engine
thaJeztah
pushed a commit
to thaJeztah/docker
that referenced
this pull request
Dec 3, 2019
Among other things, this is required to pull in microsoft/hcsshim#718 Also fixes microsoft/hcsshim#737 which was caught by checks while attempting to bump up hcsshim version. Signed-off-by: Vikram bir Singh <[email protected]> (cherry picked from commit a7b6c3f) Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
I'm not sure if this PR is responsible for a new error I'm seeing or not -- but something in between nightly docker 2019-09-25 and nightly docker 2019-12-04 is causing errors on container exit under LCOW with a This is the last change to roll in that modified |
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jan 23, 2020
Among other things, this is required to pull in microsoft/hcsshim#718 Also fixes microsoft/hcsshim#737 which was caught by checks while attempting to bump up hcsshim version. Signed-off-by: Vikram bir Singh <[email protected]> (cherry picked from commit a7b6c3f0bf5d10c6227a29bac7dd46b9a7a779bc) Signed-off-by: Sebastiaan van Stijn <[email protected]> Upstream-commit: e2f226b5b41c958fa518f677eb213eb1462f90a8 Component: engine
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HcsCreateProcess returns a set of stdio handles for the newly created
process. A while ago, we used to cache these handles and return them
the first time stdio handles were requested for the process, and then
get new handles via HcsGetProcessInfo for each subsequent request. At
some point, this code was cleaned up to instead always return the
original set of handles as non-closable (for new callers) and always get
new handles via HcsGetProcessInfo (for legacy callers, who required
closable handles).
However, this change introduced a race condition for legacy callers,
where in the case of a short lived container process, the container
could have terminated between when it was started and when the
orchestrator requested stdio handles. This led to ERROR_NOT_FOUND
being returned from HcsGetProcessInfo.
This change addresses this by returning the original handles the first
time stdio handles are requested, and then calling HcsGetProcessInfo for
every subsequent request (just as it used to work a while ago).
Signed-off-by: Kevin Parsons [email protected]