carlos / platform Public

cgroup: the shim's tenant keeps the agent's ignored signals (#272)

Whole-branch review round 1, finding 2. A direct exec.Cmd.Start child
keeps every signal the agent ignores at the fork (the runtime's
clearSignalHandlers resets only the ones it handles). The shim's own Go
runtime replaces most inherited ignores with its handlers (all but SIGHUP
and SIGINT), so the tenant's execve reset them to default: an agent that
ignored SIGTERM or SIGUSR1 started tenants that did not.

The parent now reads its SigIgn from /proc/self/status at each spawn and
carries it in the spec (new field, magic carlos-cgexec-2). The shim sets
exactly those to SIG_IGN through os/signal immediately before exec, after
LockOSThread and the empty mask, and checks that the kernel reads them
back ignored; a mismatch fails before R, as a placement failure.

The inherited-state kernel test now ignores SIGTERM and SIGUSR1 as well as
SIGHUP and compares shim and direct. Against a mutant that restores
nothing it fails with SigIgn 0x1 against 0x4201; with the fix both read
0x4201, with and without RestrictNamespaces=yes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 1 week ago 24887f97859eb3000bb47c12e0e0e7a2b6a275b0 parent 306aa76

4 files changed, +129 −10

internal/cgroup/shim.go +19 −2
internal/cgroup/shim_kernel_linux_test.go +25 −4
internal/cgroup/shim_linux.go +63 −3
internal/cgroup/shim_test.go +22 −1

diff --git a/internal/cgroup/shim.go b/internal/cgroup/shim.go

index ce7efb1aa..1bf9d2f30 100644

--- a/internal/cgroup/shim.go

+++ b/internal/cgroup/shim.go

@@ -92,10 +92,19 @@ type shimSpec struct {

Expected string // "/..." as /proc/self/cgroup shows it after "0::"

KeepFDs int // fds 0..KeepFDs-1 are the tenant's

TestHold int // test binaries only: 0 none, 1 hold before step 4, 2 hold after step 10 (before R)

+ // SigIgn is the set of signals the agent ignored when it started the

+ // shim, as /proc/self/status's SigIgn shows it: bit n-1 is signal n.

+ // A direct start's child keeps exactly these ignored through its exec

+ // (the runtime resets only the signals it handles, clearSignalHandlers),

+ // but the shim's own Go runtime replaces most inherited ignores with its

+ // handlers, which the tenant's execve then resets to default. So the

+ // shim ignores these again immediately before it execs (whole-branch

+ // review, finding 2).

+ SigIgn uint64

}

const (

- shimSpecMagic = "carlos-cgexec-1"

+ shimSpecMagic = "carlos-cgexec-2"

// shimSpecMax bounds the encoded spec the shim will read.

shimSpecMax = 16 << 20

// shimSpecMaxCount bounds each decoded count (argv, env, KeepFDs).

@@ -103,7 +112,7 @@ const (

)

// encodeShimSpec writes each field followed by a NUL, in order: the magic,

-// Path, Dir, Expected, KeepFDs, TestHold, len(Argv), each argv element,

+// Path, Dir, Expected, KeepFDs, TestHold, SigIgn, len(Argv), each argv element,

// len(Env), each env entry. Bytes pass through untouched (invalid UTF-8

// included); a string holding a NUL is refused, as execve could not carry

// it either.

@@ -123,6 +132,7 @@ func encodeShimSpec(s shimSpec) ([]byte, error) {

for _, f := range []struct{ name, v string }{

{"path", s.Path}, {"dir", s.Dir}, {"expected", s.Expected},

{"keep", strconv.Itoa(s.KeepFDs)}, {"hold", strconv.Itoa(s.TestHold)},

+ {"sigign", strconv.FormatUint(s.SigIgn, 10)},

{"argc", strconv.Itoa(len(s.Argv))},

} {

if err := put(f.name, f.v); err != nil {

@@ -188,6 +198,13 @@ func decodeShimSpec(b []byte) (shimSpec, error) {

if s.TestHold, err = count(); err != nil {

return s, err

}

+ sigign, err := next()

+ if err != nil {

+ return s, err

+ }

+ if s.SigIgn, err = strconv.ParseUint(sigign, 10, 64); err != nil {

+ return s, fmt.Errorf("cgroup: shim spec: bad ignored-signal set %q", sigign)

+ }

argc, err := count()

if err != nil {

return s, err

diff --git a/internal/cgroup/shim_kernel_linux_test.go b/internal/cgroup/shim_kernel_linux_test.go

index d64dd4037..1d81457ef 100644

--- a/internal/cgroup/shim_kernel_linux_test.go

+++ b/internal/cgroup/shim_kernel_linux_test.go

@@ -283,8 +283,19 @@ exec "$0" "$@"`

func TestShimKernelInheritedStateMatchesADirectStart(t *testing.T) {

m := shimKernelManager(t)

dir := shimKernelTarget(t, m, "state")

- signal.Ignore(syscall.SIGHUP)

- t.Cleanup(func() { signal.Reset(syscall.SIGHUP) })

+ // SIGHUP is the case Go's runtime keeps ignored by itself; SIGTERM and

+ // SIGUSR1 are ones the shim's runtime would replace with its handlers,

+ // so the tenant's exec would reset them to default unless the shim

+ // ignores them again (whole-branch review, finding 2).

+ ignored := []syscall.Signal{syscall.SIGHUP, syscall.SIGTERM, syscall.SIGUSR1}

+ for _, sig := range ignored {

+ signal.Ignore(sig)

+ }

+ t.Cleanup(func() {

+ for _, sig := range ignored {

+ signal.Reset(sig)

+ }

+ })

opened, err := syscall.Open("/dev/null", syscall.O_RDONLY, 0) // no O_CLOEXEC

if err != nil {

t.Fatal(err)

@@ -367,14 +378,24 @@ func TestShimKernelInheritedStateMatchesADirectStart(t *testing.T) {

t.Fatalf("shim state %q lacks %q", shimState, want)

}

+ sawSigIgn := false

for _, l := range shimState {

if v, ok := strings.CutPrefix(l, "SigIgn:\t"); ok {

+ sawSigIgn = true

mask, err := strconv.ParseUint(v, 16, 64)

- if err != nil || mask&(1<<(uint(syscall.SIGHUP)-1)) == 0 {

- t.Fatalf("SigIgn %q does not carry the ignored SIGHUP", v)

+ if err != nil {

+ t.Fatalf("SigIgn %q: %v", v, err)

+ }

+ for _, sig := range ignored {

+ if mask&(1<<(uint(sig)-1)) == 0 {

+ t.Fatalf("SigIgn %q does not carry the ignored %v", v, sig)

+ }

}

+ if !sawSigIgn {

+ t.Fatal("the shim tenant's state has no SigIgn line")

+ }

}

func TestShimKernelChdirFailureIsTheCommands(t *testing.T) {

diff --git a/internal/cgroup/shim_linux.go b/internal/cgroup/shim_linux.go

index a38c1b0c0..928b14d82 100644

--- a/internal/cgroup/shim_linux.go

+++ b/internal/cgroup/shim_linux.go

@@ -12,6 +12,7 @@ import (

"math"

"os"

"os/exec"

+ "os/signal"

"runtime"

"slices"

"strconv"

@@ -133,9 +134,19 @@ func (m *Manager) startShimCore(dir string, dirFile *os.File, expected string, c

dirFile.Close()

return false, errors.New("exec: environment variable contains NUL")

}

+ // The signals this process ignores now, which a direct start's child

+ // would keep ignored (the shim restores them before its exec). Unknown

+ // is a placement failure: nothing has started, and the flat start's

+ // fallback is a direct start, which keeps them by itself.

+ sigIgn, err := ignoredSignals()

+ if err != nil {

+ dirFile.Close()

+ return false, &placementError{fmt.Errorf("read the ignored signals: %w", err)}

+ }

spec, err := encodeShimSpec(shimSpec{

Path: path, Dir: wd, Argv: argv, Env: cmd.Environ(),

Expected: expected, KeepFDs: keep, TestHold: shimTestHold,

+ SigIgn: sigIgn,

})

if err != nil {

// A NUL execve cannot carry: what execve answers for it.

@@ -431,13 +442,19 @@ func runShim(args []string) int {

}

// The exec state a direct start under systemd would have: an empty

- // signal mask on the thread that execs. execve resets every handled

- // signal to default and keeps ignored ones ignored, and syscall.Exec

- // restores the RLIMIT_NOFILE this process started with.

+ // signal mask on the thread that execs, and the agent's ignored signals

+ // ignored. execve resets every handled signal to default and keeps

+ // ignored ones ignored, and syscall.Exec restores the RLIMIT_NOFILE

+ // this process started with.

runtime.LockOSThread()

if err := unix.PthreadSigmask(unix.SIG_SETMASK, &unix.Sigset_t{}, nil); err != nil {

return 125

}

+ // A failure here, before R, is "the shim exited before exec": a

+ // placement failure, with no tenant code run.

+ if err := ignoreSignals(spec.SigIgn); err != nil {

+ return 125

+ }

// R immediately before execve, and never earlier: without it the parent

// may report the start failed, so no exec may follow a failed write.

if !shimWrite(statusFD, "R\n") {

@@ -448,6 +465,49 @@ func runShim(args []string) int {

return 126

}

+// ignoredSignals reads this process's ignored-signal set from

+// /proc/self/status (SigIgn, hexadecimal; bit n-1 is signal n). Signal

+// dispositions are process-wide, so any thread's view is the process's.

+func ignoredSignals() (uint64, error) {

+ raw, err := os.ReadFile("/proc/self/status")

+ if err != nil {

+ return 0, err

+ }

+ for _, line := range strings.Split(string(raw), "\n") {

+ if v, ok := strings.CutPrefix(line, "SigIgn:"); ok {

+ return strconv.ParseUint(strings.TrimSpace(v), 16, 64)

+ }

+ return 0, errors.New("no SigIgn line in /proc/self/status")

+// ignoreSignals sets every signal in set (bit n-1 is signal n) to SIG_IGN

+// through os/signal, which keeps the shim's runtime in step, and then

+// checks the kernel's view: each one must read back ignored. os/signal

+// cannot ignore a signal its runtime keeps for itself (SIGPROF, and the

+// few without a Notify slot), but the agent's runtime keeps those too, so

+// the agent cannot have ignored one unless it inherited the ignore, and then

+// so did the shim, whose runtime leaves it alone as the agent's did. The

+// check turns any case this reasoning misses into a failed start rather

+// than a tenant with a handled signal it should have ignored. Signals the

+// shim inherited ignored beyond set are left as they are: they come from the

+// same agent, so a direct start at that moment would have kept them too.

+func ignoreSignals(set uint64) error {

+ for n := 1; n <= 64; n++ {

+ if set&(1<<(n-1)) != 0 {

+ signal.Ignore(syscall.Signal(n))

+ }

+ got, err := ignoredSignals()

+ if err != nil {

+ return err

+ }

+ if missing := set &^ got; missing != 0 {

+ return fmt.Errorf("signals %#x did not read back ignored", missing)

+ }

+ return nil

// runProbe prints this process's cgroup as /proc/self/cgroup shows it.

func runProbe() int {

raw, err := os.ReadFile("/proc/self/cgroup")

diff --git a/internal/cgroup/shim_test.go b/internal/cgroup/shim_test.go

index 191bd64c9..1fe34b243 100644

--- a/internal/cgroup/shim_test.go

+++ b/internal/cgroup/shim_test.go

@@ -15,6 +15,7 @@ func TestShimSpecRoundTripsExactBytes(t *testing.T) {

Expected: "/system.slice/carlos-edge.service/carlos-exec@a.example/g3",

KeepFDs: 5,

TestHold: 2,

+ SigIgn: 1<<0 | 1<<14 | 1<<63,

}

b, err := encodeShimSpec(in)

if err != nil {

@@ -67,8 +68,28 @@ func TestShimSpecRefusesTruncatedAndTrailing(t *testing.T) {

}

func TestShimSpecRefusesHugeCounts(t *testing.T) {

- bad := []byte("carlos-cgexec-1\x00/x\x00\x00/g\x003\x000\x001048577\x00")

+ bad := []byte("carlos-cgexec-2\x00/x\x00\x00/g\x003\x000\x000\x001048577\x00")

if _, err := decodeShimSpec(bad); err == nil {

t.Fatal("decoded an argv count above 1<<20")

}

+ // The control: the same spec with a count in bounds decodes.

+ ok := []byte("carlos-cgexec-2\x00/x\x00\x00/g\x003\x000\x000\x000\x000\x00")

+ if _, err := decodeShimSpec(ok); err != nil {

+ t.Fatalf("the control spec: %v", err)

+ }

+// The ignored-signal set is a 64-bit number: a bad one, and a spec in the

+// encoding before the set was added (magic carlos-cgexec-1), are refused.

+func TestShimSpecRefusesBadSigIgnAndTheOldEncoding(t *testing.T) {

+ for _, bad := range []string{

+ "carlos-cgexec-2\x00/x\x00\x00/g\x003\x000\x00x\x000\x000\x00",

+ "carlos-cgexec-2\x00/x\x00\x00/g\x003\x000\x0018446744073709551616\x000\x000\x00",

+ "carlos-cgexec-2\x00/x\x00\x00/g\x003\x000\x00-1\x000\x000\x00",

+ "carlos-cgexec-1\x00/x\x00\x00/g\x003\x000\x000\x000\x00",

+ } {

+ if _, err := decodeShimSpec([]byte(bad)); err == nil {

+ t.Fatalf("decoded a bad spec %q", bad)

+ }

}