libcontainer: Don't close already closed fds and bail on close(2) failures #3058
Conversation
This file contains 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
This was closed in the child[1], before calling clone_parent (so runc INIT will have this fd closed too), there is no point closing it again. This was not causing issues because we ignore the return code of close(2) and no one was opening a new fd between both calls to close. However, with the new patches that I'm working on (PR opencontainers#2576), this problem is no longer inocuos: we do open a new fd in that PR, sometimes that fd is allocated between the two close(2) calls and, as the lowest fd is allocated to the new fd, sometimes the second close ends up incorrectly closing this new fd. Before it was not a problem in practice, but it was incorrect nevertheless. This seems to be long standing bug, present since at least 2018 (a54316b), when SYNC_GRANDCHILD was introduced. [1]: https://github.com/opencontainers/runc/blob/5547b5774f71f75a088e7432fa961778750a0fbd/libcontainer/nsenter/nsexec.c#L888 Co-authored-by: Alban Crequy <[email protected]> Signed-off-by: Rodrigo Campos <[email protected]>
3 tasks
cyphar
previously requested changes
Jul 2, 2021
rata
force-pushed
the
rata/nsexec-close-bug
branch
from
57eaa87
to
6289bab
Compare
|
@cyphar fixed, PTAL |
rata
force-pushed
the
rata/nsexec-close-bug
branch
from
6289bab
to
42c9054
Compare
Don't ignore close(2) return code, rather bail if there is any unexpected failures. By checking the close return code we make sure we don't introduce the same bug (closing an already closed fd) I've fixed in the previous patch. As a side note, we are not handling in this patch when close(2) returns EINTR and the go runtime, since go 1.14, sends SIGURG to preempt goroutines. This should not happen here though, as nsenter is guaranteed to be executed before the go runtime starts. Signed-off-by: Rodrigo Campos <[email protected]>
rata
force-pushed
the
rata/nsexec-close-bug
branch
from
42c9054
to
83776dd
Compare
AkihiroSuda
approved these changes
Jul 2, 2021
|
As this is not a bug (without #2576) I think we don't need to backport it to 1.0. |
|
@kolyshkin agree, it was there for 3 years and doesn't seem to hurt anyone. It should be safe to backport just the removal of the extra close "libcontainer: Don't close fds already closed" (7d479e6). But as you think it is best, backporting it won't fix any visible regressions AFAIK. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hi!
We've found these bugs while rebasing #2576. The bug is that we are closing an fd that is already closed, and as we ignore close(2) failures we never realized.
It was not a big deal in runc master, it seems, as that close fails and is otherwise ignored. However, when working on PR #2576 we are opening a new fd, sometimes between the two close(2) calls, the fd number is reused (allocates the lower available fd) and the second close(2) was closing this new open fd that reused the number.
I've create two patches: one to remove the bogus close, and another one to check failures on close(2) so this bug does not happen again (as long as we continue to check for close failures :)). I've also verified that if we check for close failures on the close I remove, it indeed always returns an error on master.
You can find a more in detail explanation in the commit messages :)
cc @alban