libct.Start: fix locking, do not allow a second container init #4271
Conversation
kolyshkin
force-pushed
the
two-inits
branch
2 times, most recently
from
dfe38a9
to
2575cf4
Compare
|
@opencontainers/runc-maintainers PTAL |
|
@lifubang ptal |
SGTM, but could you give some details for this issue in commit 3b5376f? I think it's a refactor, but not a fix? |
| if err := c.createExecFifo(); err != nil { | ||
| return err | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
My suggestion change is unneeded Once #4319 merged.
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>
I understand now, thanks your explanation. |
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.