Skip to content

Move poststart hook from runc create to runc start #5186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rata merged 3 commits into from
Apr 8, 2026
Merged

Move poststart hook from runc create to runc start #5186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b0762c7
libct: add lock-less c.signal
kolyshkin Mar 19, 2026
2253475
libct: factor handleFifo out of c.exec
kolyshkin Mar 19, 2026
3cdda46
Move poststart hook from runc create to runc start
kolyshkin Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `EXTRA_BUILDTAGS` make variable is deprecated in favor of `RUNC_BUILDTAGS`
and will be removed in runc 1.6. (#5171)

### Fixed ###
- The poststart hooks are now executed after starting the user-specified
process, fixing a runtime-spec conformance issue. (#4347, #5186)

## [1.5.0-rc.1] - 2026-03-12

> 憎しみを束ねてもそれは脆い！
Expand Down
54 changes: 37 additions & 17 deletions libcontainer/container_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,14 @@ func (c *Container) Exec() error {

func (c *Container) exec() error {
path := filepath.Join(c.stateDir, execFifoFilename)
pid := c.initProcess.pid()
if err := handleFifo(path, c.initProcess.pid()); err != nil {
return err
}

return c.postStart()
}

func handleFifo(path string, pid int) error {
blockingFifoOpenCh := awaitFifoOpen(path)
for {
select {
Expand All @@ -253,6 +260,29 @@ func (c *Container) exec() error {
}
}

func (c *Container) postStart() (retErr error) {
if !c.config.HasHook(configs.Poststart) {
return nil
}

defer func() {
if retErr != nil {
// A poststart hook failed; kill the container.
if err := c.signal(unix.SIGKILL); err != nil && !errors.Is(err, ErrNotRunning) {
Copy link
Copy Markdown
Member

@rata rata Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err := c.signal(unix.SIGKILL); err != nil && !errors.Is(err, ErrNotRunning) {
if err := ignoreTerminateErrors(parent.terminate()); err != nil {

This line is causing the race that @lifubang detected. With the suggestion here, I can't trigger it anymore. It is what the old code was doing (I didn't realize at first that this was changed! 😱 ). The key is that parent.terminate() does a kill AND a wait.

Doing this change alone is enough to fix the race. But there is another issue, that I think we should also fix.

The call to currentOCIState() has the side effect of changing the state to running. So the following happens:

func (r *runner) run(config *specs.Process) from util_linux.go
 --> r.container.Run(process)
       --> func (c *Container) Run(process *Process) error
             ---> return c.exec()  // This returns the post-start error with PR

But to get there, we called currentOCIState() that ends up calling refreshState(). Because we already deleted the fifo (we call handle fifo just before the post-start hook, that calls the handleFifoResult that deletes the fifo), that can end up setting the state to running.

That is a complicated (or detailed, you choose :D) way to say that calling currentOCIState() ends up setting the state to running. And that causes issues with Destroy().

The thing is, the outer most run function aforementioned (the one that takes configs as param), has the r.destroy() defer in case of errors (that we do hit an error here). And destroy when we are in the running state returns an error if we still have an init.

Sooo, I think we should:
a) Keep the code as it was using p.terminate(), so we kill and wait for the process
b) Don't call currentOCIState() that causes the state machine to be inconsistent, probably just set the status to running manually (having a quick look, this seems to be what crun also does for these hooks).

So something like this:

diff --git libcontainer/container_linux.go libcontainer/container_linux.go
index adddebbf..2f7186e2 100644
--- libcontainer/container_linux.go
+++ libcontainer/container_linux.go
@@ -268,15 +268,24 @@ func (c *Container) postStart() (retErr error) {
        defer func() {
                if retErr != nil {
                        // A poststart hook failed; kill the container.
-                       if err := c.signal(unix.SIGKILL); err != nil && !errors.Is(err, ErrNotRunning) {
+                       if err := ignoreTerminateErrors(c.initProcess.terminate()); err != nil {
                                logrus.WithError(err).Warn("kill after failed poststart")
                        }
                }
        }()
 
-       s, err := c.currentOCIState()
-       if err != nil {
-               return err
+       // Build the OCI state directly rather than calling currentOCIState(),
+       // which would trigger refreshState() and transition c.state to
+       // "running". That transition causes Destroy() to fail if the init
+       // process is still alive when the poststart hook fails.
+       bundle, annotations := utils.Annotations(c.config.Labels)
+       s := &specs.State{
+               Version:     specs.Version,
+               ID:          c.ID(),
+               Bundle:      bundle,
+               Annotations: annotations,
+               Status:      specs.StateRunning,
+               Pid:         c.initProcess.pid(),
        }
        return c.config.Hooks.Run(configs.Poststart, s)
 }

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the c.initProcess.terminate() (killing init) won't work when a container don't have own pidns (the case described and handled in (*container).signal.

OTOH I see that missing wait is the issue in the current implementation because we're still init's parent / child reaper. Wonder if adding c.initProcess.wait() will fix both issues (one with the state as well, because after wait the init is gone) without introducing some other bug.

Copy link
Copy Markdown
Contributor Author

@kolyshkin kolyshkin Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the updated commit; @rata @lifubang PTAL

Copy link
Copy Markdown
Member

@lifubang lifubang Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test results seem stable now.

logrus.WithError(err).Warn("kill after failed poststart")
}
// We're still init's parent so wait is required.
_, _ = c.initProcess.wait()
}
}()

s, err := c.currentOCIState()
if err != nil {
return err
}
return c.config.Hooks.Run(configs.Poststart, s)
}

func readFromExecFifo(execFifo io.Reader) error {
data, err := io.ReadAll(execFifo)
if err != nil {
Expand Down Expand Up @@ -371,19 +401,6 @@ func (c *Container) start(process *Process) (retErr error) {

if process.Init {
c.fifo.Close()
if c.config.HasHook(configs.Poststart) {
s, err := c.currentOCIState()
if err != nil {
return err
}

if err := c.config.Hooks.Run(configs.Poststart, s); err != nil {
if err := ignoreTerminateErrors(parent.terminate()); err != nil {
logrus.Warn(fmt.Errorf("error running poststart hook: %w", err))
}
return err
}
}
}
return nil
}
Expand All @@ -396,7 +413,10 @@ func (c *Container) start(process *Process) (retErr error) {
func (c *Container) Signal(s os.Signal) error {
c.m.Lock()
defer c.m.Unlock()
return c.signal(s)
}

func (c *Container) signal(s os.Signal) error {
// When a container has its own PID namespace, inside it the init PID
// is 1, and thus it is handled specially by the kernel. In particular,
// killing init with SIGKILL from an ancestor namespace will also kill
Expand All @@ -410,7 +430,7 @@ func (c *Container) Signal(s os.Signal) error {
logrus.WithError(err).Warn("failed to kill all processes, possibly due to lack of cgroup (Hint: enable cgroup v2 delegation)")
// Some processes may leak when cgroup is not delegated
// https://github.com/opencontainers/runc/pull/4395#pullrequestreview-2291179652
return c.signal(s)
return c.signalInit(s)
}
// For not rootless container, if there is no init process and no cgroup,
// it means that the container is not running.
Expand All @@ -422,10 +442,10 @@ func (c *Container) Signal(s os.Signal) error {
return nil
}

return c.signal(s)
return c.signalInit(s)
}

func (c *Container) signal(s os.Signal) error {
func (c *Container) signalInit(s os.Signal) error {
// To avoid a PID reuse attack, don't kill non-running container.
if !c.hasInit() {
return ErrNotRunning
Expand Down
6 changes: 5 additions & 1 deletion tests/integration/hooks.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ function teardown() {
echo "testing hook $hook"
update_config '.hooks |= {"'$hook'": [{"path": "/bin/true"}, {"path": "/bin/false"}]}'
runc run "test_hook-$hook"
[[ "$output" != "Hello World" ]]
# Failed poststart hooks results in container being killed,
# but only after it has started, so output may or may not appear.
if [ "$hook" != "poststart" ]; then
[[ "$output" != "Hello World" ]]
fi
[ "$status" -ne 0 ]
[[ "$output" == *"error running $hook hook #1:"* ]]
done
Expand Down
Loading