libct.Start: fix locking, do not allow a second container init#4271
Merged
lifubang merged 3 commits intoopencontainers:mainfrom Jun 11, 2024
Merged
libct.Start: fix locking, do not allow a second container init#4271lifubang merged 3 commits intoopencontainers:mainfrom
lifubang merged 3 commits intoopencontainers:mainfrom
Conversation
kolyshkin
force-pushed
the
two-inits
branch
2 times, most recently
from
dfe38a9 to
2575cf4
Compare
Contributor
Author
|
@opencontainers/runc-maintainers PTAL |
AkihiroSuda
approved these changes
May 31, 2024
Contributor
Author
|
@lifubang ptal |
Member
SGTM, but could you give some details for this issue in commit 3b5376f? I think it's a refactor, but not a fix? |
lifubang
reviewed
Jun 10, 2024
| if err := c.createExecFifo(); err != nil { | ||
| return err | ||
| } | ||
| defer func() { |
Member
Contributor
Author
Maybe the commit message is too long and vague. Here's an except about the main issue: "part of c.Run executing without the lock". I did not get too deep inside why this locking is needed, but if it is needed, it is a bug to execute Run in parallel with, say, Start. This is what's being fixed. |
In case file already exists, mknod(2) will return EEXIST. This os.Stat call was (inadvertently?) added by commit 805b8c7. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. The code to call c.exec from c.Run was initially added by commit 3aacff6. At the time, there was a lock in c.Run. That lock was removed by commit bd3c4f8, which resulted in part of c.Run executing without the lock. 2. All the Start/Run/Exec calls were a mere wrappers for start/run/exec adding a lock, but some more code crept into Start at some point, e.g. by commits 805b8c7 and 108ee85. Since the reason mentioned in commit 805b8c7 is no longer true after refactoring, we can fix this. Fix both issues by moving code out of wrappers, and adding locking into c.Run. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
By definition, every container has only 1 init (i.e. PID 1) process. Apparently, libcontainer API supported running more than 1 init, and at least one tests mistakenly used it. Let's not allow that, erroring out if we already have init. Doing otherwise _probably_ results in some confusion inside the library. Fix two cases in libct/int which ran two inits inside a container. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Member
I understand now, thanks your explanation. |
lifubang
approved these changes
Jun 11, 2024
Merged
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.
By definition, every container has only 1 init (i.e. PID 1) process.
Apparently, libcontainer API supported running more than 1 "init", and
two tests mistakenly used it. Of course, the second "init" was not
PID 1, but it was started like init and, I guess, some of the Container fields
were set to wrong values.
Let's not allow that, erroring out if we already have init running.
Fix two cases in libct/int which ran two inits inside a container.
Also, fix a locking issue and remove some code that's not needed.