From 1ec1ed6b35794e0b0b17eb084b333dfab8240064 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 19:01:58 -0500 Subject: [PATCH 1/3] fix(exec/shim): stop a late SIGTERM killing a clean shutdown The parent and the child arm a timer off the same req.Deadline, so when it fires the executor SIGTERMs the process group at the same instant the child's own context deadline cancels the handler. A cooperative handler returns ctx.Err(), the shim writes its Result frame and settles on exit 0, and then mainExitCode's deferred signal.Stop puts SIGTERM back on its default disposition while the process is still alive and still a target. A signal landing in that gap kills a child that had already reported, and the parent reads back Signal 15 for a handler that shut down cleanly. Ignoring SIGTERM for the rest of the exit path closes it. The defer is registered before the Stop so that it runs after it, and that ordering is the whole point: get it backwards and Stop has the last word and nothing changes. Only the real child does this. An in-process caller is not about to exit and would carry an ignored SIGTERM for the life of the test binary, so Main passes true and the tests pass false. TestKillLadderClassifiesACooperativeTimeoutCorrectly failed 8 times in 120 runs under -race with the CPU loaded, and 0 in 120 after. Putting the same call in Main rather than mainExitCode only got it to 2 in 120, because the remaining defers and the return are themselves wide enough to lose the race. --- exec/shim/internal_test.go | 2 +- exec/shim/main.go | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/exec/shim/internal_test.go b/exec/shim/internal_test.go index c9dcc50..f304415 100644 --- a/exec/shim/internal_test.go +++ b/exec/shim/internal_test.go @@ -112,7 +112,7 @@ func callMainExitCode(t *testing.T, defs []job.Registrable, req *exec.Request) i t.Setenv(EnvRequestFD, strconv.Itoa(reqDup)) t.Setenv(EnvResultFD, strconv.Itoa(resDup)) - return mainExitCode(defs) + return mainExitCode(defs, false) } // TestMainExitCode pins the exit-code contract mainExitCode must satisfy: diff --git a/exec/shim/main.go b/exec/shim/main.go index f6290b9..7ac5897 100644 --- a/exec/shim/main.go +++ b/exec/shim/main.go @@ -88,7 +88,11 @@ const ( // clean run without having to inspect the frame first: the exit code // corroborates what the Result already says. func Main(defs ...job.Registrable) { - os.Exit(mainExitCode(defs)) + // true: this is the real child, so mainExitCode's signal teardown + // must leave SIGTERM ignored rather than restore the default. See the + // exiting parameter's own comment for why that matters here and not + // for the in-process callers that pass false. + os.Exit(mainExitCode(defs, true)) } // mainExitCode does the real work of Main and returns the process exit @@ -104,7 +108,7 @@ func Main(defs ...job.Registrable) { // err == nil alone cannot tell mainExitCode apart from a clean success. // Only StatusHandlerError keeps exit 0; every other non-OK status, // including one Run reported without an error, is a nonzero exit. -func mainExitCode(defs []job.Registrable) int { +func mainExitCode(defs []job.Registrable, exiting bool) int { // fdFromEnv guarantees a non-negative descriptor, so the uintptr // conversion cannot wrap. in := os.NewFile(uintptr(fdFromEnv(EnvRequestFD, defaultRequestFD)), "dispatch-exec-request") @@ -140,6 +144,29 @@ func mainExitCode(defs []job.Registrable) int { sigCh := make(chan os.Signal, 1) signal.Notify(sigCh, syscall.SIGTERM) + + // Registered before the Stop below so that it runs after it: defers + // unwind last-in-first-out, and the order is what this is for. + // signal.Stop puts SIGTERM back on its default disposition, which is + // fatal, while this process is still alive and still a target. The + // parent's kill ladder (subprocess.terminate) signals the whole + // process group the moment a deadline fires, and a handler that + // cooperated has returned ctx.Err() at that same instant off the + // child's own copy of the deadline, so the two events collide by + // design. Without this, a SIGTERM landing in the gap between Stop and + // the process actually exiting kills a child that had already written + // its Result frame and settled on exit 0, and the parent reports + // Signal 15 for a handler that shut down cleanly. Measured at roughly + // 7% of runs under load before this was added. + // + // Only the real child does this. An in-process caller (the tests, + // which pass exiting false) is not about to exit, and leaving SIGTERM + // ignored for the rest of the test binary's life would outlive the + // call that set it. + if exiting { + defer signal.Ignore(syscall.SIGTERM) + } + defer signal.Stop(sigCh) // done unblocks the goroutine below once mainExitCode is about to From 634bf870e2c45e3c4977206729f725df7346ced1 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 19:02:12 -0500 Subject: [PATCH 2/3] chore(exec/shim): annotate the gosec findings that are false positives Nine of them, all paths gosec cannot see are already contained. The six G703 hits in localfs.go are removes and renames of temp files that os.CreateTemp made inside a directory resolve had already confined to fs.root, and resolve does the real work: it rejects an absolute key, rejects one that cleans to the root itself, and Rel-checks the joined path so nothing climbs out. The seventh is a WalkDir over req.OutputDir, which the parent picks and hands across the request fd, never something the handler supplies. The two G115 hits convert a descriptor to uintptr, and fdFromEnv already falls back to its default on a negative, with both defaults positive. Annotated at each site with its reason instead of excluded wholesale in .golangci.yml, matching the #nosec G304 already sitting on LocalFS.Open. Suppressing the first batch surfaced more that the taint analysis had been hiding behind them, so this took three passes to reach a clean run. --- exec/shim/localfs.go | 6 ++++++ exec/shim/main.go | 3 +++ 2 files changed, 9 insertions(+) diff --git a/exec/shim/localfs.go b/exec/shim/localfs.go index 7e7c944..0fbb5b4 100644 --- a/exec/shim/localfs.go +++ b/exec/shim/localfs.go @@ -142,6 +142,7 @@ func (fs *LocalFS) Create(_ context.Context, bucket, key string) (artifact.Write // already confined to fs.root; nothing here reads attacker input. // Safe to remove: the path came from resolve's containment check, not // from a raw key. + // #nosec G703 -- os.CreateTemp generated this name inside a directory resolve already confined to fs.root. _ = os.Remove(tmp.Name()) return nil, fmt.Errorf("shim: create %s/%s: %w", bucket, key, cherr) @@ -237,12 +238,14 @@ func (w *localWriter) Commit(_ context.Context) (artifact.ObjectInfo, error) { // resolve already confined to fs.root; it is not attacker input. if err := w.file.Sync(); err != nil { _ = w.file.Close() + // #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve. _ = os.Remove(tmpName) return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err) } if err := w.file.Close(); err != nil { + // #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve. _ = os.Remove(tmpName) return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err) @@ -252,7 +255,9 @@ func (w *localWriter) Commit(_ context.Context) (artifact.ObjectInfo, error) { // (w.final at Create time; tmpName is a sibling CreateTemp made inside // that same, already-contained directory). // Both paths are confined to fs.root by resolve. + // #nosec G703 -- both operands passed resolve's containment check; see the comment above. if err := os.Rename(tmpName, w.final); err != nil { + // #nosec G703 -- tmpName is this writer's own CreateTemp file, confined to fs.root by resolve. _ = os.Remove(tmpName) return artifact.ObjectInfo{}, fmt.Errorf("shim: commit %s/%s: %w", w.bucket, w.key, err) @@ -280,6 +285,7 @@ func (w *localWriter) Abort() error { // name is this writer's own temp file, created inside the directory // resolve already confined to fs.root at Create time. // name is our own temp file under the resolved, contained directory. + // #nosec G703 -- name is this writer's own CreateTemp file, confined to fs.root by resolve. if err := os.Remove(name); err != nil && !os.IsNotExist(err) { return fmt.Errorf("shim: abort %s/%s: %w", w.bucket, w.key, err) } diff --git a/exec/shim/main.go b/exec/shim/main.go index 7ac5897..59e9e97 100644 --- a/exec/shim/main.go +++ b/exec/shim/main.go @@ -111,7 +111,9 @@ func Main(defs ...job.Registrable) { func mainExitCode(defs []job.Registrable, exiting bool) int { // fdFromEnv guarantees a non-negative descriptor, so the uintptr // conversion cannot wrap. + // #nosec G115 -- fdFromEnv rejects negatives and both defaults are positive, so neither conversion can wrap. in := os.NewFile(uintptr(fdFromEnv(EnvRequestFD, defaultRequestFD)), "dispatch-exec-request") + // #nosec G115 -- fdFromEnv rejects negatives and both defaults are positive, so neither conversion can wrap. out := os.NewFile(uintptr(fdFromEnv(EnvResultFD, defaultResultFD)), "dispatch-exec-result") // Applied before anything else touches the request: RLIMIT_CORE in @@ -394,6 +396,7 @@ func collectOutputs(dir string) ([]exec.OutputFile, error) { // attempt, never a value read out of the untrusted payload the // handler parses. // dir is the request's own OutputDir, not attacker-controlled. + // #nosec G703 -- dir is req.OutputDir, chosen by the parent and delivered over the request fd, never handler input. err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { if err != nil { return err From c1f25c5d581bf46a816240007f1d1441cfb0d737 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 20:25:51 -0500 Subject: [PATCH 3/3] fix(exec/subprocess): wait out the reap before probing the grandchild TestKillLadderKillsTheWholeProcessGroup checked the grandchild pid once, the instant Run returned, and failed on ubuntu CI with the grandchild "still alive". It was not alive. terminate sends the group its SIGKILL and returns without waiting for anything to actually die, so Run can come back while the grandchild is a zombie: killed, but not yet reaped. syscall.Kill reports success rather than ESRCH for that state, because the process table entry is still sitting there. The grandchild is forked by the fixture rather than by the test, so once the fixture dies it gets reparented and reaped on whatever schedule the subreaper feels like, which is nothing this test synchronises with. Polling for up to two seconds is the same treatment TestKillLadderReapsAHelperAfterACooperativeLeaderExits already got, in this file, for the identical flake. longSleep is five minutes, so the bound is nowhere near it and a grandchild that genuinely survived the group kill still fails this exactly as before. Only reproduces on Linux. 200 runs of the old assertion under load on darwin came back clean on both this branch and the commit it forks from, which is also why the first CI run caught it and local runs did not. --- exec/subprocess/kill_unix_test.go | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/exec/subprocess/kill_unix_test.go b/exec/subprocess/kill_unix_test.go index edda3ea..6a1c77c 100644 --- a/exec/subprocess/kill_unix_test.go +++ b/exec/subprocess/kill_unix_test.go @@ -103,7 +103,34 @@ func TestKillLadderKillsTheWholeProcessGroup(t *testing.T) { // that surviving would show up as "still alive," not as "happened to // exit on its own around the same time," see envLongSleep's doc // comment (main_test.go). - if kerr := syscall.Kill(pid, 0); kerr != syscall.ESRCH { + // + // Polled rather than probed once, for the same reason + // TestKillLadderReapsAHelperAfterACooperativeLeaderExits polls its own + // helper: the grandchild is forked by the fixture, not by this test, + // so once the fixture is killed the grandchild is reparented and + // reaped by whatever subreaper the OS hands it to, on no schedule + // this test is synchronised with. terminate sends the group its + // SIGKILL and returns without waiting, so Run can return while the + // grandchild is a zombie that has died but not yet been reaped, and + // syscall.Kill reports success rather than ESRCH for exactly that + // state. A single check right after Run raced the reap on Linux CI + // under parallel-package load. Two seconds is nowhere near longSleep, + // so a grandchild that genuinely survived the group kill still fails + // this the same way it always did. + deadline := time.Now().Add(2 * time.Second) + + var kerr error + + for { + kerr = syscall.Kill(pid, 0) + if errors.Is(kerr, syscall.ESRCH) || time.Now().After(deadline) { + break + } + + time.Sleep(10 * time.Millisecond) + } + + if !errors.Is(kerr, syscall.ESRCH) { t.Errorf("syscall.Kill(%d, 0) = %v, want ESRCH — grandchild pid %d is still alive", pid, kerr, pid) } }