Fix crash and resource-leak bugs in the containerd shim

- runsc/debug.go: the SIGUSR2 stack dumper never exited its loop, so one
signal pinned a core for the life of the shim.
- proc/init_state.go: the start-failure path closed p.io unconditionally,
but Init.Create leaves it nil for terminal containers. The nil deref
killed the shim, and with it the pod.
- proc/utils.go: getLastRuntimeError leaked the log file it opened, on
every failed runsc command.
- utils/volumes.go: configureShm wrote into a nil annotation map, which
IsSandbox permits.
- runsc/epoll.go: the cgroups v1 OOM watcher panicked on the first
publish failure, which is normal under CRI-O. Reuse forward's leniency
check; the panic under containerd is kept.
- runsccmd/runsc.go: cmdOutput returned slices aliasing sync.Pool buffers
that putBuf immediately recycled.
This commit is contained in:
Jing Chen
2026-08-26 22:47:36 +00:00
parent b6ec8fed14
commit ff3425ac1f
15 changed files with 496 additions and 27 deletions
+7 -1
View File
@@ -45,12 +45,18 @@ go_library(
go_test(
name = "proc_test",
size = "small",
srcs = ["update_test.go"],
srcs = [
"init_state_test.go",
"update_test.go",
"utils_test.go",
],
library = ":proc",
deps = [
"//pkg/shim/v1/runsccmd",
"@com_github_containerd_console//:go_default_library",
"@com_github_containerd_containerd_v2//pkg/protobuf/types:go_default_library",
"@com_github_containerd_containerd_v2//pkg/stdio:go_default_library",
"@com_github_containerd_errdefs//:go_default_library",
"@com_github_containerd_go_runc//:go_default_library",
],
)
+4 -1
View File
@@ -92,7 +92,10 @@ func (s *createdState) Start(ctx context.Context, restoreConf *extension.Restore
// To work around that, we treat non-root container in start/restore
// failure state as stopped.
if !s.p.Sandbox {
s.p.io.Close()
// p.io is nil when the process was created with a terminal.
if s.p.io != nil {
s.p.io.Close()
}
s.p.setExited(internalErrorCode)
s.transition(stopped)
}
+105
View File
@@ -0,0 +1,105 @@
// Copyright 2026 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package proc
import (
"context"
"os/exec"
"sync"
"testing"
"github.com/containerd/console"
"github.com/containerd/containerd/v2/pkg/stdio"
runc "github.com/containerd/go-runc"
"gvisor.dev/gvisor/pkg/shim/v1/runsccmd"
)
// fakePlatform is a no-op stdio.Platform.
type fakePlatform struct{}
func (fakePlatform) CopyConsole(ctx context.Context, con console.Console, id, stdin, stdout, stderr string, wg *sync.WaitGroup) (console.Console, error) {
return con, nil
}
func (fakePlatform) ShutdownConsole(context.Context, console.Console) error { return nil }
func (fakePlatform) Close() error { return nil }
// closerIO records whether it was closed. The embedded interface is nil; only
// the methods below are called.
type closerIO struct {
runc.IO
closed bool
}
func (c *closerIO) Close() error {
c.closed = true
return nil
}
func (c *closerIO) Set(*exec.Cmd) {}
// TestCreatedStateStartFailureWithoutIO verifies that a failed start does not
// panic when p.io is nil, as Init.Create leaves it for a terminal.
func TestCreatedStateStartFailureWithoutIO(t *testing.T) {
p := New("id", &runsccmd.Runsc{Command: "/nonexistent-runsc"}, stdio.Stdio{Terminal: true})
p.Platform = fakePlatform{}
p.Sandbox = false
if err := p.Start(t.Context()); err == nil {
t.Fatal("Start() succeeded, want error")
}
if _, ok := p.initState.(*stoppedState); !ok {
t.Errorf("initState = %T, want *stoppedState", p.initState)
}
if got := p.ExitStatus(); got != internalErrorCode {
t.Errorf("ExitStatus() = %d, want %d", got, internalErrorCode)
}
}
// TestCreatedStateStartFailureClosesIO verifies that the failure path still
// closes the IO when it exists.
func TestCreatedStateStartFailureClosesIO(t *testing.T) {
p := New("id", &runsccmd.Runsc{Command: "/nonexistent-runsc"}, stdio.Stdio{})
p.Platform = fakePlatform{}
p.Sandbox = false
cio := &closerIO{}
p.io = cio
if err := p.Start(t.Context()); err == nil {
t.Fatal("Start() succeeded, want error")
}
if !cio.closed {
t.Error("process IO was not closed on start failure")
}
if _, ok := p.initState.(*stoppedState); !ok {
t.Errorf("initState = %T, want *stoppedState", p.initState)
}
}
// TestCreatedStateStartFailureSandbox verifies that a sandbox is left in the
// created state on start failure, so that it can still be deleted.
func TestCreatedStateStartFailureSandbox(t *testing.T) {
p := New("id", &runsccmd.Runsc{Command: "/nonexistent-runsc"}, stdio.Stdio{Terminal: true})
p.Platform = fakePlatform{}
p.Sandbox = true
if err := p.Start(t.Context()); err == nil {
t.Fatal("Start() succeeded, want error")
}
if _, ok := p.initState.(*createdState); !ok {
t.Errorf("initState = %T, want *createdState", p.initState)
}
}
+1
View File
@@ -44,6 +44,7 @@ func getLastRuntimeError(r *runsccmd.Runsc) (string, error) {
if err != nil {
return "", err
}
defer f.Close()
var (
errMsg string
+80
View File
@@ -0,0 +1,80 @@
// Copyright 2026 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package proc
import (
"os"
"path/filepath"
"testing"
"gvisor.dev/gvisor/pkg/shim/v1/runsccmd"
)
func openFDs(t *testing.T) int {
t.Helper()
entries, err := os.ReadDir("/proc/self/fd")
if err != nil {
t.Skipf("cannot read /proc/self/fd: %v", err)
}
return len(entries)
}
// TestGetLastRuntimeErrorNoFDLeak verifies that getLastRuntimeError closes the
// log file it opens. It runs on every failed runsc command, so a leak here
// exhausts the fd table of a long-lived shim.
func TestGetLastRuntimeErrorNoFDLeak(t *testing.T) {
logPath := filepath.Join(t.TempDir(), "log.json")
content := `{"level":"info","msg":"starting","time":"2026-01-01T00:00:00Z"}
{"level":"error","msg":"something failed","time":"2026-01-01T00:00:01Z"}
`
if err := os.WriteFile(logPath, []byte(content), 0600); err != nil {
t.Fatalf("WriteFile: %v", err)
}
r := &runsccmd.Runsc{Log: logPath}
// Prime lazy initialization before sampling the fd count.
if _, err := getLastRuntimeError(r); err != nil {
t.Fatalf("getLastRuntimeError: %v", err)
}
before := openFDs(t)
const iterations = 64
for i := 0; i < iterations; i++ {
msg, err := getLastRuntimeError(r)
if err != nil {
t.Fatalf("getLastRuntimeError: %v", err)
}
if want := "something failed"; msg != want {
t.Fatalf("getLastRuntimeError() = %q, want %q", msg, want)
}
}
after := openFDs(t)
// Slack for unrelated runtime activity; a per-call leak adds ~iterations.
if after-before > iterations/2 {
t.Errorf("open fds grew from %d to %d over %d calls; log file is not being closed", before, after, iterations)
}
}
// TestGetLastRuntimeErrorNoLog verifies the empty-log short circuit.
func TestGetLastRuntimeErrorNoLog(t *testing.T) {
msg, err := getLastRuntimeError(&runsccmd.Runsc{})
if err != nil {
t.Fatalf("getLastRuntimeError: %v", err)
}
if msg != "" {
t.Errorf("getLastRuntimeError() = %q, want empty", msg)
}
}
+4
View File
@@ -70,6 +70,8 @@ go_library(
go_test(
name = "runsc_test",
srcs = [
"debug_test.go",
"epoll_test.go",
"oom_v2_test.go",
"service_test.go",
"state_test.go",
@@ -77,6 +79,7 @@ go_test(
library = ":runsc",
deps = [
"//pkg/shim/v1/utils",
"@com_github_containerd_cgroups_v3//cgroup1:go_default_library",
"@com_github_containerd_cgroups_v3//cgroup2:go_default_library",
"@com_github_containerd_containerd_api//events:go_default_library",
"@com_github_containerd_containerd_api//runtime/task/v2:go_default_library",
@@ -85,5 +88,6 @@ go_test(
"@com_github_containerd_errdefs//:go_default_library",
"@com_github_google_go_cmp//cmp:go_default_library",
"@com_github_opencontainers_runtime_spec//specs-go:go_default_library",
"@org_golang_x_sys//unix:go_default_library",
],
)
+13 -9
View File
@@ -31,18 +31,22 @@ func setDebugSigHandler() {
dumpCh := make(chan os.Signal, 1)
signal.Notify(dumpCh, syscall.SIGUSR2)
go func() {
buf := make([]byte, 10240)
for range dumpCh {
for {
n := runtime.Stack(buf, true)
if n >= len(buf) {
buf = make([]byte, 2*len(buf))
continue
}
log.L.Debugf("User requested stack trace:\n%s", buf[:n])
}
log.L.Debugf("User requested stack trace:\n%s", dumpStacks())
}
}()
log.L.Debugf("For full process dump run: kill -%d %d", syscall.SIGUSR2, os.Getpid())
})
}
// dumpStacks returns the stack traces of all goroutines.
func dumpStacks() []byte {
buf := make([]byte, 10240)
for {
n := runtime.Stack(buf, true)
if n < len(buf) {
return buf[:n]
}
buf = make([]byte, 2*len(buf))
}
}
+55
View File
@@ -0,0 +1,55 @@
// Copyright 2026 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package runsc
import (
"strings"
"testing"
"time"
)
// TestDumpStacksTerminates verifies that a stack dump returns rather than
// looping forever, which would spin a core and flood the log on every SIGUSR2.
func TestDumpStacksTerminates(t *testing.T) {
done := make(chan []byte, 1)
go func() {
done <- dumpStacks()
}()
select {
case buf := <-done:
if len(buf) == 0 {
t.Fatal("dumpStacks() returned no data")
}
if got := string(buf); !strings.Contains(got, "goroutine ") {
t.Errorf("dumpStacks() = %q, want a goroutine dump", got)
}
case <-time.After(30 * time.Second):
t.Fatal("dumpStacks() did not return; it is looping")
}
}
// TestDumpStacksRepeatable verifies that consecutive dumps do not share a
// buffer.
func TestDumpStacksRepeatable(t *testing.T) {
first := dumpStacks()
second := dumpStacks()
if len(first) == 0 || len(second) == 0 {
t.Fatalf("dumpStacks() returned empty: %d, %d bytes", len(first), len(second))
}
if &first[0] == &second[0] {
t.Error("consecutive dumpStacks() calls returned aliasing buffers")
}
}
+6 -2
View File
@@ -26,6 +26,7 @@ import (
cgroups "github.com/containerd/cgroups/v3/cgroup1"
"github.com/containerd/containerd/v2/core/events"
"github.com/containerd/containerd/v2/core/runtime"
"github.com/containerd/log"
"golang.org/x/sys/unix"
)
@@ -129,8 +130,11 @@ func (e *epoller) process(ctx context.Context, fd uintptr) {
if err := e.publisher.Publish(ctx, runtime.TaskOOMEventTopic, &TaskOOM{
ContainerID: i.id,
}); err != nil {
// Should not happen.
panic(fmt.Errorf("publish OOM event: %w", err))
if publishFailureIsFatal() {
// Should not happen when an event sink is configured.
panic(fmt.Errorf("publish OOM event: %w", err))
}
log.L.Warningf("Failed to publish OOM event (no containerd event sink): %v", err)
}
}
+100
View File
@@ -0,0 +1,100 @@
// Copyright 2026 The gVisor Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// https://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//go:build linux
// +build linux
package runsc
import (
"context"
"testing"
cgroups "github.com/containerd/cgroups/v3/cgroup1"
"golang.org/x/sys/unix"
)
// fakeCgroup reports a fixed state. The embedded interface is nil; only State
// is called.
type fakeCgroup struct {
cgroups.Cgroup
state cgroups.State
}
func (f *fakeCgroup) State() cgroups.State { return f.state }
// newTestEpoller returns an epoller with one live container registered on an
// eventfd, along with that fd. The epoll fd is invalid: these tests never wait
// on it.
func newTestEpoller(t *testing.T, state cgroups.State) (*epoller, uintptr) {
t.Helper()
efd, err := unix.Eventfd(0, unix.EFD_CLOEXEC|unix.EFD_NONBLOCK)
if err != nil {
t.Skipf("eventfd unavailable: %v", err)
}
fd := uintptr(efd)
e := &epoller{
fd: -1,
publisher: &errorPublisher{},
set: map[uintptr]*item{
fd: {id: "test", cg: &fakeCgroup{state: state}},
},
}
return e, fd
}
// TestEpollerProcessDoesNotPanicOnPublishError verifies that an OOM publish
// failure is logged rather than fatal when no event sink is configured,
// matching runscService.forward. Under CRI-O the first OOM used to kill the
// shim.
func TestEpollerProcessDoesNotPanicOnPublishError(t *testing.T) {
t.Setenv("TTRPC_ADDRESS", "")
e, fd := newTestEpoller(t, cgroups.Thawed)
t.Cleanup(func() { unix.Close(int(fd)) })
defer func() {
if r := recover(); r != nil {
t.Errorf("process() panicked without an event sink: %v", r)
}
}()
e.process(context.Background(), fd)
}
// TestEpollerProcessPanicsOnPublishErrorUnderContainerd verifies that a publish
// failure remains fatal when an event sink is configured.
func TestEpollerProcessPanicsOnPublishErrorUnderContainerd(t *testing.T) {
t.Setenv("TTRPC_ADDRESS", "/run/containerd/containerd.sock.ttrpc")
e, fd := newTestEpoller(t, cgroups.Thawed)
t.Cleanup(func() { unix.Close(int(fd)) })
defer func() {
if r := recover(); r == nil {
t.Error("process() did not panic on publish error under containerd")
}
}()
e.process(context.Background(), fd)
}
// TestEpollerProcessDeletedCgroup verifies that a deleted cgroup is
// unregistered without publishing.
func TestEpollerProcessDeletedCgroup(t *testing.T) {
t.Setenv("TTRPC_ADDRESS", "/run/containerd/containerd.sock.ttrpc")
// No cleanup: process() closes the fd itself on this path.
e, fd := newTestEpoller(t, cgroups.Deleted)
e.process(context.Background(), fd)
if _, ok := e.set[fd]; ok {
t.Error("deleted cgroup was not removed from the epoller set")
}
}
+12 -12
View File
@@ -654,22 +654,22 @@ func (s *runscService) getContainerPids(ctx context.Context, c *Container) ([]ui
return pids, nil
}
// publishFailureIsFatal reports whether a failure to publish an event is fatal.
// An empty TTRPC_ADDRESS means no event sink is configured (as under CRI-O), so
// the publisher can never connect and publish errors are expected.
func publishFailureIsFatal() bool {
return os.Getenv("TTRPC_ADDRESS") != ""
}
func (s *runscService) forward(ctx context.Context, publisher shim.Publisher) {
// An empty TTRPC_ADDRESS means no containerd event sink is configured, so
// the publisher can never connect and publish errors are expected and
// non-fatal. This is how CRI-O launches the shim, but the check is not
// CRI-O specific: any caller that omits TTRPC_ADDRESS gets the same lenient
// handling. When TTRPC_ADDRESS is set (e.g. under containerd) a publish
// failure is unexpected and remains fatal.
isEmptyTTRPCAddress := os.Getenv("TTRPC_ADDRESS") == ""
isFatal := publishFailureIsFatal()
for e := range s.events {
if err := publisher.Publish(ctx, getTopic(e), e); err != nil {
if isEmptyTTRPCAddress {
log.L.Warningf("Failed to post event (no containerd event sink): %v", err)
continue
if isFatal {
// Should not happen when an event sink is configured.
panic(fmt.Errorf("post event: %w", err))
}
// Should not happen when an event sink is configured.
panic(fmt.Errorf("post event: %w", err))
log.L.Warningf("Failed to post event (no containerd event sink): %v", err)
}
}
}
+11 -2
View File
@@ -788,8 +788,17 @@ func cmdOutput(cmd *exec.Cmd, combined bool) ([]byte, []byte, error) {
if err == nil && status != 0 {
err = fmt.Errorf("%q did not terminate successfully", cmd.Args[0])
}
// The buffers go back to the pool on return, so copy their contents out.
if stderr == nil {
return stdout.Bytes(), nil, err
return cloneBuf(stdout), nil, err
}
return stdout.Bytes(), stderr.Bytes(), err
return cloneBuf(stdout), cloneBuf(stderr), err
}
// cloneBuf returns a copy of b's contents that does not alias b.
func cloneBuf(b *bytes.Buffer) []byte {
if b == nil {
return nil
}
return append([]byte(nil), b.Bytes()...)
}
+62
View File
@@ -85,3 +85,65 @@ exit 0
t.Fatalf("stdin resources: got %+v, want memory limit %d", got.Memory, limit)
}
}
// TestCmdOutputDoesNotAliasPooledBuffer verifies that cmdOutput copies its
// output before returning. Its buffers come from a sync.Pool and are released
// on return, so an alias would be overwritten by the next runsc invocation
// while the caller is still unmarshaling it.
func TestCmdOutputDoesNotAliasPooledBuffer(t *testing.T) {
dir := t.TempDir()
script := filepath.Join(dir, "fake-runsc")
const want = "original output"
scriptBody := fmt.Sprintf("#!/bin/sh\nprintf '%%s' %q\n", want)
if err := os.WriteFile(script, []byte(scriptBody), 0o755); err != nil {
t.Fatal(err)
}
r := &Runsc{Command: script, Root: filepath.Join(dir, "root")}
out, _, err := cmdOutput(r.command(t.Context(), "state", "cid-1"), true)
if err != nil {
t.Fatalf("cmdOutput: %v", err)
}
if string(out) != want {
t.Fatalf("cmdOutput() = %q, want %q", out, want)
}
// Scribble over the pooled buffers, as a subsequent command would.
for i := 0; i < 8; i++ {
b := getBuf()
b.WriteString(strings.Repeat("X", len(want)*4))
putBuf(b)
}
if string(out) != want {
t.Errorf("cmdOutput() result was corrupted to %q; it aliases a pooled buffer", out)
}
}
// TestCmdOutputSeparateStderr verifies the non-combined path also copies.
func TestCmdOutputSeparateStderr(t *testing.T) {
dir := t.TempDir()
script := filepath.Join(dir, "fake-runsc")
if err := os.WriteFile(script, []byte("#!/bin/sh\nprintf 'out'\nprintf 'err' >&2\n"), 0o755); err != nil {
t.Fatal(err)
}
r := &Runsc{Command: script, Root: filepath.Join(dir, "root")}
stdout, stderr, err := cmdOutput(r.command(t.Context(), "state", "cid-1"), false)
if err != nil {
t.Fatalf("cmdOutput: %v", err)
}
for i := 0; i < 8; i++ {
b := getBuf()
b.WriteString(strings.Repeat("X", 64))
putBuf(b)
}
if string(stdout) != "out" {
t.Errorf("stdout = %q, want %q", stdout, "out")
}
if string(stderr) != "err" {
t.Errorf("stderr = %q, want %q", stderr, "err")
}
}
+4
View File
@@ -330,6 +330,10 @@ func configureShm(s *specs.Spec) (bool, error) {
m := &s.Mounts[i]
if m.Destination == shmPath && m.Type == "bind" {
if IsSandbox(s) {
// IsSandbox is true for a spec without annotations.
if s.Annotations == nil {
s.Annotations = make(map[string]string)
}
s.Annotations[volumeKeyPrefix+devshmName+".source"] = m.Source
s.Annotations[volumeKeyPrefix+devshmName+".type"] = devshmType
s.Annotations[volumeKeyPrefix+devshmName+".share"] = "pod"
+32
View File
@@ -427,6 +427,38 @@ func TestUpdateVolumeAnnotations(t *testing.T) {
},
},
},
{
// A spec without a container-type annotation is a sandbox, so the
// map must be created before the /dev/shm hints are added.
name: "shm-sandbox-nil-annotations",
spec: &specs.Spec{
Annotations: nil,
Mounts: []specs.Mount{
{
Destination: "/dev/shm",
Type: "bind",
Source: testVolumePath,
Options: []string{"ro", "foo"},
},
},
},
expected: &specs.Spec{
Annotations: map[string]string{
volumeKeyPrefix + devshmName + ".share": "pod",
volumeKeyPrefix + devshmName + ".type": "tmpfs",
volumeKeyPrefix + devshmName + ".options": "rw",
volumeKeyPrefix + devshmName + ".source": testVolumePath,
},
Mounts: []specs.Mount{
{
Destination: "/dev/shm",
Type: "tmpfs",
Source: testVolumePath,
Options: []string{"ro", "foo"},
},
},
},
},
{
name: "shm-container",
spec: &specs.Spec{