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/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 f6290b9..59e9e97 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,10 +108,12 @@ 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. + // #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 @@ -140,6 +146,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 @@ -367,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 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) } }