From b0762c7af186bdf975f8304659c8dbe31431b472 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 19 Mar 2026 15:37:43 -0700 Subject: [PATCH 1/3] libct: add lock-less c.signal Rename c.signal to c.signalInit, and add c.signal which is a lock-less form of c.Signal. To be used by the next patch. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index b4368547c..a5aff4d7d 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -396,7 +396,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 @@ -410,7 +413,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. @@ -422,10 +425,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 From 2253475660b79ec13612832ecfd65a44e718f0e1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 19 Mar 2026 15:56:59 -0700 Subject: [PATCH 2/3] libct: factor handleFifo out of c.exec No functional change. To be used by the next patch. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index a5aff4d7d..df31dda29 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -232,7 +232,10 @@ func (c *Container) Exec() error { func (c *Container) exec() error { path := filepath.Join(c.stateDir, execFifoFilename) - pid := c.initProcess.pid() + return handleFifo(path, c.initProcess.pid()) +} + +func handleFifo(path string, pid int) error { blockingFifoOpenCh := awaitFifoOpen(path) for { select { From 3cdda464fa1f1eb0d5774acacf73bb446df5f3d8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 19 Mar 2026 13:57:12 -0700 Subject: [PATCH 3/3] Move poststart hook from runc create to runc start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime-spec [1] currently says: > 6. Runtime's start command is invoked with the unique identifier of > the container. > 7. The startContainer hooks MUST be invoked by the runtime. If any > startContainer hook fails, the runtime MUST generate an error, stop > the container, and continue the lifecycle at step 12. > 8. The runtime MUST run the user-specified program, as specified by > process. > 9. The poststart hooks MUST be invoked by the runtime. If any > poststart hook fails, the runtime MUST generate an error, stop the > container, and continue the lifecycle at step 12. > ... > 11. Runtime's delete command is invoked with the unique identifier of > the container. > 12. The container MUST be destroyed by undoing the steps performed > during create phase (step 2). > 13. The poststop hooks MUST be invoked by the runtime. If any poststop > hook fails, the runtime MUST log a warning, but the remaining hooks > and lifecycle continue as if the hook had succeeded. Currently, we do 9 before 8 (heck, even before 6), which is clearly against the spec and results in issues like the one described in [2]. Let's move running poststart hook to after the user-specified process has started. NOTE this patch only fixes the order and does not implement removing the container when the poststart hook failed (as this part of the spec is controversial -- destroy et al and should probably be, and currently are, part of "runc delete"). [1]: https://github.com/opencontainers/runtime-spec/blob/main/runtime.md#lifecycle [2]: https://github.com/opencontainers/runc/issues/5182 Reported-by: ningmingxiao Reported-by: Erik Sjölund Signed-off-by: Kir Kolyshkin --- CHANGELOG.md | 4 ++++ libcontainer/container_linux.go | 42 ++++++++++++++++++++++----------- tests/integration/hooks.bats | 6 ++++- 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48b1eb9ea..9e17abd83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 > 憎しみを束ねてもそれは脆い！ diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index df31dda29..6938ea1be 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -232,7 +232,11 @@ func (c *Container) Exec() error { func (c *Container) exec() error { path := filepath.Join(c.stateDir, execFifoFilename) - return handleFifo(path, c.initProcess.pid()) + if err := handleFifo(path, c.initProcess.pid()); err != nil { + return err + } + + return c.postStart() } func handleFifo(path string, pid int) error { @@ -256,6 +260,29 @@ func handleFifo(path string, pid int) 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) { + 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 { @@ -374,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 } diff --git a/tests/integration/hooks.bats b/tests/integration/hooks.bats index 854fc4b83..3a9e06dcf 100644 --- a/tests/integration/hooks.bats +++ b/tests/integration/hooks.bats @@ -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