Merge pull request #14422 from milantracy:shim

PiperOrigin-RevId: 971739193

Author: gvisor-bot

pkg/shim/v1/proc/BUILD

@@ -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",
     ],
 )

pkg/shim/v1/proc/init_state.go

@@ -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)
 		}

pkg/shim/v1/proc/init_state_test.go

@@ -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)
+	}
+}

pkg/shim/v1/proc/utils.go

@@ -44,6 +44,7 @@ func getLastRuntimeError(r *runsccmd.Runsc) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	defer f.Close()
 
 	var (
 		errMsg string

pkg/shim/v1/proc/utils_test.go

@@ -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)
+	}
+}

pkg/shim/v1/runsc/BUILD

@@ -70,13 +70,16 @@ go_library(
 go_test(
     name = "runsc_test",
     srcs = [
+        "debug_test.go",
+        "epoll_test.go",
         "oom_v2_test.go",
         "service_test.go",
         "state_test.go",
     ],
     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",
     ],
 )

pkg/shim/v1/runsc/debug.go

@@ -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))
+	}
+}

pkg/shim/v1/runsc/debug_test.go

@@ -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")
+	}
+}

pkg/shim/v1/runsc/epoll.go

@@ -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)
 	}
 }