Fix race condition when sync with child and grandchild #1237
Conversation
|
ping @opencontainers/runc-maintainers |
|
fwiw it looks very likely that this PR also fixes #1281. |
libcontainer/nsenter/nsexec.c
Outdated
| @@ -695,6 +723,10 @@ void nsexec(void) | |||
| bail("failed to sync with parent: SYNC_RECVPID_ACK: got %u", s); | |||
| } | |||
|
|
|||
| s = SYNC_CHILD_READY; | |||
| if (write(syncfd, &s, sizeof(s)) != sizeof(s)) | |||
| bail("failed to sync with patent: write(SYNC_CHILD_READY)"); | |||
|
@cyphar can you take a look at this one? |
|
I'm assuming this is in a final state now, if so then would be good to get it in ASAP as its causing pretty much all other PR's to have failed tests :( |
|
/me is looking at it. While it does fix the issue, there's some parts I'd like to get cleaned up before merging. |
|
Out of interest does that include changing the way the children are created and reaping the initial child which would make #1301 unneeded? |
To the branch await-prs-from-138131099 in the fork in cloudfoundry-incubator. We can get back onto a released version of runc when the following PRs are merged: * opencontainers/runc#1237 * opencontainers/runc#1275 dadoo was the main guardian component that had to change here, to accomodate changes in runc for processes that have TTYs. [#138131099]
|
LGTM |
|
ping @cyphar |
| break; | ||
| default: | ||
| bail("unexpected sync value"); | ||
| bail("unexpected sync value: %u", s); |
There was a problem hiding this comment.
While I understand this makes the code nicer, remember that the only debugging information we normally get is the return code. Maybe we don't lose too much information, but something to keep in mind.
There was a problem hiding this comment.
Yeah, I don't think we lose any information here.
| /* Now sync with grandchild. */ | ||
|
|
||
| ready = false; | ||
| while (!ready) { |
There was a problem hiding this comment.
What happens if the grandchild is ready before the child -- or if the child dies above but the grandchild is still around?
There was a problem hiding this comment.
What happens if the grandchild is ready before the child
It won't, grandchild won't do anything before father finishes syncing with child and send SYNC_GRANDCHILD to grandchild.
or if the child dies above but the grandchild is still around
All cases that child dies by error after creating grandchild, child will kill grandchild before exit.
libcontainer/nsenter/nsexec.c
Outdated
| @@ -413,7 +414,7 @@ void nsexec(void) | |||
| { | |||
| int pipenum; | |||
| jmp_buf env; | |||
| int syncpipe[2]; | |||
| int syncChildPipe[2], syncGrandchildPipe[2]; | |||
There was a problem hiding this comment.
Nit: Please use snake_case in C code.
Fixes: opencontainers#1236 Fixes: opencontainers#1281 Signed-off-by: Qiang Huang <[email protected]>
|
Updated to use snake case. @crosbymichael @cyphar PTAL. |
|
Failure is because LGTM |
|
I've made an issue (mvdan/sh#66) to track it. |
|
ping @crosbymichael |
|
LGTM |
Fixes: #1236
Signed-off-by: Qiang Huang [email protected]